Bug 30979 - [Qt] REGRESSION: Allow applications to use their own QWidget bypassing QWebView.
: [Qt] REGRESSION: Allow applications to use their own QWidget bypassing QWebView.
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Critical
Assigned To:
:
: Qt
:
: 29799
  Show dependency treegraph
 
Reported: 2009-10-31 11:27 PST by
Modified: 2009-11-04 00:50 PST (History)


Attachments
Patch (16.12 KB, patch)
2009-11-01 06:36 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.45 KB, patch)
2009-11-02 12:06 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2009-11-03 06:36 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.65 KB, patch)
2009-11-03 08:04 PST, Yael
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-31 11:27:16 PST
Users of QtWebKit might want their own QWidget to replace QWebView. Windowed plugins need access to this QWidget, but with the current design, QWebPageClient is not exposed. Applications that replace QWebView would simply crash when plugins are loaded into them.
------- Comment #1 From 2009-11-01 06:36:15 PST -------
Created an attachment (id=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.
------- Comment #2 From 2009-11-01 10:40:49 PST -------
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.
------- Comment #3 From 2009-11-01 13:45:47 PST -------
(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.
------- Comment #4 From 2009-11-01 19:34:27 PST -------
(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.
------- Comment #5 From 2009-11-02 11:44:17 PST -------
The solution here would not be exposing QWebPageClient, hence the title change.
------- Comment #6 From 2009-11-02 12:06:10 PST -------
Created an attachment (id=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.
------- Comment #7 From 2009-11-02 17:25:43 PST -------
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;
------- Comment #8 From 2009-11-02 19:49:32 PST -------
(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.
------- Comment #9 From 2009-11-03 06:36:24 PST -------
Created an attachment (id=42374) [details]
Patch

This patch is recycling the client in the rare case that the application switchs view at runtime.
------- Comment #10 From 2009-11-03 08:04:38 PST -------
Created an attachment (id=42384) [details]
Patch

Replace NULL with 0.
------- Comment #11 From 2009-11-04 00:50:29 PST -------
(From update of attachment 42384 [details])
Clearing flags on attachment: 42384

Committed r50503: <http://trac.webkit.org/changeset/50503>
------- Comment #12 From 2009-11-04 00:50:37 PST -------
All reviewed patches have been landed.  Closing bug.