Summary: | Qt Plugins : Remove usage of winId() | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Girish Ramakrishnan <girish> | ||||||||||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, girish, kenneth | ||||||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | PC | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 20081 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Girish Ramakrishnan
2009-10-07 09:20:02 PDT
Created attachment 40792 [details]
Approach 1 - windowId
Created attachment 40793 [details]
Approach 1 - use pluginWidget
This patch fixes running Qt with ARGB visuals, yay! Created attachment 40863 [details]
Approach 2 - Use pluginWiget
Has the approval of tronical (simon). Awaiting, kenneth's blessing :-)
Comment on attachment 40863 [details]
Approach 2 - Use pluginWiget
This looks good to me, but I know nothing about Qt or x11.
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.
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? (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. Created attachment 41031 [details]
ownerWidget
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 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. (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. Created attachment 41091 [details]
ownerWidget (2)
Created attachment 41092 [details]
ownerWidget (3)
Oops, I uploaded the wrong patch last time.
Comment on attachment 41092 [details] ownerWidget (3) Clearing flags on attachment: 41092 Committed r49502: <http://trac.webkit.org/changeset/49502> All reviewed patches have been landed. Closing bug. |