Bug 30979

Created attachment 42276 [details] Patch This patch includes only the functionality of exposing QWebPageClient and non of the required documentation. QWebPageClient is now a QObject, so that we can extend it without breaking binary compatibility.

We introduced QWebPageClient to decouple WebCore from the widgets or graphics items in the WebKit layer on top. It is designed as private interface, it is not designed to be part of the public API. Making it public means we cannot change it anymore, we wouldn't be able to add new virtual functions or rename existing ones. We should extend the existing public API (QWebPage, QWebView, QGraphicsWebView) instead of introducing yet another public interface for developers to learn.

(In reply to comment #2) > We introduced QWebPageClient to decouple WebCore from the widgets or graphics > items in the WebKit layer on top. It is designed as private interface, it is > not designed to be part of the public API. I am more than happy to discuss an alternative solution, that will not involve exposing QWebPageClient. > Making it public means we cannot > change it anymore, we wouldn't be able to add new virtual functions or rename > existing ones. The modified QWebPageClient is inheriting QObject. We will not be able to add new virtual methods, but we will be able to add new signals and slots instead. > We should extend the existing public API (QWebPage, QWebView, QGraphicsWebView) > instead of introducing yet another public interface for developers to learn. Qt 4.5 allows replacing QWebView with another QWidget, and there are no side effects. In Qt 4.6, applications that replace QWebView with their own QWidget will crash when loading plugins. IMO, this is regression.

(In reply to comment #3) > > We should extend the existing public API (QWebPage, QWebView, QGraphicsWebView) > > instead of introducing yet another public interface for developers to learn. > Qt 4.5 allows replacing QWebView with another QWidget, and there are no side > effects. In Qt 4.6, applications that replace QWebView with their own QWidget > will crash when loading plugins. IMO, this is regression. That is true, it is a regression. Even though it's a use-case that we shouldn't encourage we shouldn't break it either, especially as it looks like it's easy to fix: QWebViewPrivate is the implementation of the QWebPageClient interface that operates on the QWebView. However at closer inspection it appears that there's nothing in that implementation that makes it really depend on QWebView. AFAICS all the calls and queries to the view would work on any QWidget. It appears the fix would be to rip out the QWebPageClient implementation from QWebViewPrivate, turn into into a private standalone class (say QWebPageWidgetClient) and use it independently from QWebView. I guess QWebPage should own/allocate the instance when QWebPage::setView() is called.

The solution here would not be exposing QWebPageClient, hence the title change.

Created attachment 42333 [details] Patch (In reply to comment #4) > It appears the fix would be to rip out the QWebPageClient implementation from > QWebViewPrivate, turn into into a private standalone class (say > QWebPageWidgetClient) and use it independently from QWebView. I guess QWebPage > should own/allocate the instance when QWebPage::setView() is called. This patch does what you suggested.

Instead of +#if defined(Q_WS_X11) +#include <QX11Info> +#endif +int QWebPageWidgetClient::screenNumber() const +{ +#if defined(Q_WS_X11) + if (view) + return view->x11Info().screen(); +#endif + + return 0; +} Could use QDesktopWidget::screenNumber() which is cross-platform? Simon also suggest to recycle the client instead of throwing it all the time: + delete d->client; + d->client = view ? new QWebPageWidgetClient(view) : 0;

(In reply to comment #7) > Instead of > +#if defined(Q_WS_X11) > +#include <QX11Info> > +#endif > > +int QWebPageWidgetClient::screenNumber() const > +{ > +#if defined(Q_WS_X11) > + if (view) > + return view->x11Info().screen(); > +#endif > + > + return 0; > +} > > Could use QDesktopWidget::screenNumber() which is cross-platform? > The patch as it is, is only moving code from one place to another, to address the regression. If we want to change the existing functionality, I think that should be a separate bug.

Created attachment 42374 [details] Patch This patch is recycling the client in the rare case that the application switchs view at runtime.

Created attachment 42384 [details] Patch Replace NULL with 0.

Comment on attachment 42384 [details] Patch Clearing flags on attachment: 42384 Committed r50503: <http://trac.webkit.org/changeset/50503>

All reviewed patches have been landed. Closing bug.