Bug 30979 - [Qt] REGRESSION: Allow applications to use their own QWidget bypassing QWebView.
Summary: [Qt] REGRESSION: Allow applications to use their own QWidget bypassing QWebView.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Critical
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks: Qt46
  Show dependency treegraph
 
Reported: 2009-10-31 11:27 PDT by Yael
Modified: 2009-11-04 00:50 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2009-10-31 11:27:16 PDT
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 Yael 2009-11-01 06:36:15 PST
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.
Comment 2 Simon Hausmann 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 Yael 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 Simon Hausmann 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 Yael 2009-11-02 11:44:17 PST
The solution here would not be exposing QWebPageClient, hence the title change.
Comment 6 Yael 2009-11-02 12:06:10 PST
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.
Comment 7 Benjamin Poulain 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 Yael 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 Yael 2009-11-03 06:36:24 PST
Created attachment 42374 [details]
Patch

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

Replace NULL with 0.
Comment 11 WebKit Commit Bot 2009-11-04 00:50:29 PST
Comment on attachment 42384 [details]
Patch

Clearing flags on attachment: 42384

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