Bug 30170 - Qt Plugins : Remove usage of winId()
: Qt Plugins : Remove usage of winId()
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Qt
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To: Nobody
: Qt
Depends on: 20081
Blocks:
  Show dependency treegraph
 
Reported: 2009-10-07 09:20 PDT by Girish Ramakrishnan
Modified: 2009-10-13 07:27 PDT (History)
3 users (show)

See Also:


Attachments
Approach 1 - windowId (6.40 KB, patch)
2009-10-07 09:21 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Approach 1 - use pluginWidget (5.23 KB, patch)
2009-10-07 09:22 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Approach 2 - Use pluginWiget (8.01 KB, patch)
2009-10-08 04:09 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
Qt bug (702 bytes, text/x-c++)
2009-10-08 22:49 PDT, Girish Ramakrishnan
no flags Details
ownerWidget (9.75 KB, patch)
2009-10-12 04:13 PDT, Girish Ramakrishnan
hausmann: review+
commit-queue: commit‑queue-
Details | Formatted Diff | Diff
ownerWidget (2) (9.88 KB, patch)
2009-10-13 00:25 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff
ownerWidget (3) (9.90 KB, patch)
2009-10-13 00:53 PDT, Girish Ramakrishnan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Girish Ramakrishnan 2009-10-07 09:20:02 PDT
Currently, the code uses winId() of QWebPageClient in many places. 

This is a bug for two reasons:
1. Everytime we use winId(), we end up creating a native widget (which in turn disables content propagation).
2. Neither windowed nor windowless plugins require the winId of the QWebView or QGraphicsView.

The only requirement AFAIK for plugins from the QWebPageClient are:
1. Provide the window id
2. Provide QWidget * to QGraphicsView or QWebView. This is needed for the parent of the QX11EmbedContainer (windowed mode) and in PopupMenu's parent.

I discussed this with Kenneth over irc. We have two approaches:
1. Replace winId with windowId. We always return the id of the window (i.e top level widget).
2. Replace winId with pluginWidget(). pluginWidget returns a QWidget* - either QWebView or QGraphicsView. (fn name debatable)

We decided on approach 1 on irc, because Kenneth said we were moving away from QWidget based code (I am not sure, but I took it at face value :)). However, when trying it, I have found that there are situations when we really really the QWidget* (In PlatformScreenQt.cpp and PopupMenuQt.cpp). I have marked them as "####" in attached patch. We cannot use pluginParent() in those situations. pluginParent() returns the QGraphicsWebView or the QWebView. So, to get to the window, we have to end up writing code like:

    QObject *pp = client->pluginParent();
    if (QWidget *w = qobject_cast<QWidget *>(pp)) {
        windowId = w->window()->winId();
    } else if (QGraphicsWebView *v = qobject_cast<QGraphicsWebView *>(pp)) {
        QGraphicsView *view = v->views().value(0);
        if (view)
            windowId = view->window()->winId();
    }

I have gone ahead and also tried approach 2. Seems to work, attached.

What do you think?
Comment 1 Girish Ramakrishnan 2009-10-07 09:21:19 PDT
Created attachment 40792 [details]
Approach 1 - windowId
Comment 2 Girish Ramakrishnan 2009-10-07 09:22:10 PDT
Created attachment 40793 [details]
Approach 1 - use pluginWidget
Comment 3 Girish Ramakrishnan 2009-10-08 01:29:55 PDT
This patch fixes running Qt with ARGB visuals, yay!
Comment 4 Girish Ramakrishnan 2009-10-08 04:09:45 PDT
Created attachment 40863 [details]
Approach 2 - Use pluginWiget

Has the approval of tronical (simon). Awaiting, kenneth's blessing :-)
Comment 5 Eric Seidel 2009-10-08 09:28:48 PDT
Comment on attachment 40863 [details]
Approach 2 - Use pluginWiget

This looks good to me, but I know nothing about Qt or x11.
Comment 6 Girish Ramakrishnan 2009-10-08 22:49:26 PDT
Created attachment 40931 [details]
Qt bug

Ok, I have figured out why this patch solves my ARGB issue.

The problem: 
If we have WA_TranslucentBackground set on a window and a QWebView with flash content that is inside this window, the app crashes. The attached program reproduces this problem. I will report this on the qt-labs channel.

Reason: 
There's a Qt bug that having a native window inside windows having ARGB fails (wrong visual is used to create the X Render Picture).

The X Error (BadMatch) can be 'ignored', but gdk overrides Qt's default error handler and asserts even in release mode.
Comment 7 Kenneth Rohde Christiansen 2009-10-10 05:58:20 PDT
Implementation is fine from my quick look at it, but I didn't like the name pluginWidget. It is not very clear what it does and what it is.

pluginParentWindow maybe? maybe it is not even plugin dependent, so maybe parentWindow is better? ownerWindow?
Comment 8 Girish Ramakrishnan 2009-10-12 04:12:51 PDT
(In reply to comment #7)
> Implementation is fine from my quick look at it, but I didn't like the name
> pluginWidget. It is not very clear what it does and what it is.
> 
> pluginParentWindow maybe? maybe it is not even plugin dependent, so maybe
> parentWindow is better? ownerWindow?

I like owner prefix. Since it's not a window but a widget, ownerWidget sounds good to me. 

I will create a patch.
Comment 9 Girish Ramakrishnan 2009-10-12 04:13:54 PDT
Created attachment 41031 [details]
ownerWidget
Comment 10 Simon Hausmann 2009-10-12 22:36:52 PDT
Comment on attachment 41031 [details]
ownerWidget

Great cleanup!

>      case NPNVnetscapeWindow: {
>          void* w = reinterpret_cast<void*>(value);
> -        *((XID *)w) = m_parentFrame->view()->hostWindow()->platformPageClient()->winId();
> +        QWebPageClient* client = m_parentFrame->view()->hostWindow()->platformPageClient();
> +        *((XID *)w) = client ? client->ownerWidget()->window()->winId() : 0;
>          return NPERR_NO_ERROR;

I really hope that this works as expected, especially with flash. Where we previously returned
the windowid of the QWebView here we now return the toplevel ID, which is according to spec but
may not be what the plugin expects.
Comment 11 WebKit Commit Bot 2009-10-12 22:40:40 PDT
Comment on attachment 41031 [details]
ownerWidget

Rejecting patch 41031 from commit-queue.

Patch https://bugs.webkit.org/attachment.cgi?id=41031 from bug 30170 failed to download and apply.
Comment 12 Girish Ramakrishnan 2009-10-13 00:24:39 PDT
(In reply to comment #10)
> (From update of attachment 41031 [details])
> Great cleanup!
> 
> >      case NPNVnetscapeWindow: {
> >          void* w = reinterpret_cast<void*>(value);
> > -        *((XID *)w) = m_parentFrame->view()->hostWindow()->platformPageClient()->winId();
> > +        QWebPageClient* client = m_parentFrame->view()->hostWindow()->platformPageClient();
> > +        *((XID *)w) = client ? client->ownerWidget()->window()->winId() : 0;
> >          return NPERR_NO_ERROR;
> 
> I really hope that this works as expected, especially with flash. Where we
> previously returned
> the windowid of the QWebView here we now return the toplevel ID, which is
> according to spec but
> may not be what the plugin expects.

According to https://developer.mozilla.org/En/NPN_GetValue, "NPNVnetscapeWindow  - Unix/X11: Gets the browser toplevel window in which the plug-in is displayed; returns Window"

But really, the point of this patch was not to fix that :-) It was unintentional, the new patch, which I am uploading removed the 'window()' part.

Also, I checked with both windowed and windowless. NPNVnetscapeWindow is never queried. So it's dead code, atleast for flash.
Comment 13 Girish Ramakrishnan 2009-10-13 00:25:35 PDT
Created attachment 41091 [details]
ownerWidget (2)
Comment 14 Girish Ramakrishnan 2009-10-13 00:53:06 PDT
Created attachment 41092 [details]
ownerWidget (3)

Oops, I uploaded the wrong patch last time.
Comment 15 WebKit Commit Bot 2009-10-13 07:27:20 PDT
Comment on attachment 41092 [details]
ownerWidget (3)

Clearing flags on attachment: 41092

Committed r49502: <http://trac.webkit.org/changeset/49502>
Comment 16 WebKit Commit Bot 2009-10-13 07:27:23 PDT
All reviewed patches have been landed.  Closing bug.