Bug 30170 – Qt Plugins : Remove usage of winId()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Girish Ramakrishnan]

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?

[Girish Ramakrishnan]

Created attachment 40792 [details]
Approach 1 - windowId

[Girish Ramakrishnan]

Created attachment 40793 [details]
Approach 1 - use pluginWidget

[Girish Ramakrishnan]

This patch fixes running Qt with ARGB visuals, yay!

[Girish Ramakrishnan]

Created attachment 40863 [details]
Approach 2 - Use pluginWiget

Has the approval of tronical (simon). Awaiting, kenneth's blessing :-)

[Eric Seidel (no email)]

Comment on attachment 40863 [details]
Approach 2 - Use pluginWiget

This looks good to me, but I know nothing about Qt or x11.

[Girish Ramakrishnan]

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.

[Kenneth Rohde Christiansen]

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?

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

Created attachment 41031 [details]
ownerWidget

[Simon Hausmann]

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.

[WebKit Commit Bot]

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.

[Girish Ramakrishnan]

(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.

[Girish Ramakrishnan]

Created attachment 41091 [details]
ownerWidget (2)

[Girish Ramakrishnan]

Created attachment 41092 [details]
ownerWidget (3)

Oops, I uploaded the wrong patch last time.

[WebKit Commit Bot]

Comment on attachment 41092 [details]
ownerWidget (3)

Clearing flags on attachment: 41092

Committed r49502: <http://trac.webkit.org/changeset/49502>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.