Bug 30170 - Qt Plugins : Remove usage of winId()
: Qt Plugins : Remove usage of winId()
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: PC All
: P2 Normal
Assigned To:
:
: Qt
: 20081
:
  Show dependency treegraph
 
Reported: 2009-10-07 09:20 PST by
Modified: 2009-10-13 07:27 PST (History)


Attachments
Approach 1 - windowId (6.40 KB, patch)
2009-10-07 09:21 PST, Girish Ramakrishnan
no flags Review Patch | Details | Formatted Diff | Diff
Approach 1 - use pluginWidget (5.23 KB, patch)
2009-10-07 09:22 PST, Girish Ramakrishnan
no flags Review Patch | Details | Formatted Diff | Diff
Approach 2 - Use pluginWiget (8.01 KB, patch)
2009-10-08 04:09 PST, Girish Ramakrishnan
no flags Review Patch | Details | Formatted Diff | Diff
Qt bug (702 bytes, text/x-c++)
2009-10-08 22:49 PST, Girish Ramakrishnan
no flags Details
ownerWidget (9.75 KB, patch)
2009-10-12 04:13 PST, Girish Ramakrishnan
hausmann: review+
commit-queue: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
ownerWidget (2) (9.88 KB, patch)
2009-10-13 00:25 PST, Girish Ramakrishnan
no flags Review Patch | Details | Formatted Diff | Diff
ownerWidget (3) (9.90 KB, patch)
2009-10-13 00:53 PST, Girish Ramakrishnan
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-07 09:20:02 PST
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 From 2009-10-07 09:21:19 PST -------
Created an attachment (id=40792) [details]
Approach 1 - windowId
------- Comment #2 From 2009-10-07 09:22:10 PST -------
Created an attachment (id=40793) [details]
Approach 1 - use pluginWidget
------- Comment #3 From 2009-10-08 01:29:55 PST -------
This patch fixes running Qt with ARGB visuals, yay!
------- Comment #4 From 2009-10-08 04:09:45 PST -------
Created an attachment (id=40863) [details]
Approach 2 - Use pluginWiget

Has the approval of tronical (simon). Awaiting, kenneth's blessing :-)
------- Comment #5 From 2009-10-08 09:28:48 PST -------
(From update of attachment 40863 [details])
This looks good to me, but I know nothing about Qt or x11.
------- Comment #6 From 2009-10-08 22:49:26 PST -------
Created an attachment (id=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 From 2009-10-10 05:58:20 PST -------
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 From 2009-10-12 04:12:51 PST -------
(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 From 2009-10-12 04:13:54 PST -------
Created an attachment (id=41031) [details]
ownerWidget
------- Comment #10 From 2009-10-12 22:36:52 PST -------
(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.
------- Comment #11 From 2009-10-12 22:40:40 PST -------
(From update of attachment 41031 [details])
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 From 2009-10-13 00:24:39 PST -------
(In reply to comment #10)
> (From update of attachment 41031 [details] [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 From 2009-10-13 00:25:35 PST -------
Created an attachment (id=41091) [details]
ownerWidget (2)
------- Comment #14 From 2009-10-13 00:53:06 PST -------
Created an attachment (id=41092) [details]
ownerWidget (3)

Oops, I uploaded the wrong patch last time.
------- Comment #15 From 2009-10-13 07:27:20 PST -------
(From update of attachment 41092 [details])
Clearing flags on attachment: 41092

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