|
Description
marcoil
2008-07-17 10:04:43 PDT
Created attachment 22367 [details]
Zattoo's modified PluginViewWin.cpp file
Created attachment 22368 [details]
Zattoo's modified PluginView.h file
Sorry marcoil, I meant my attachments to go against https://bugs.webkit.org/show_bug.cgi?id=20095, which was open in another tab. I'm not sure how to delete them. Me neither, but don't worry about it. I have two not-fully-working implementations of this: - At http://git.collabora.co.uk/?p=user/marcoil/webkit.git;a=shortlog;h=refs/heads/marcoil/plugins-wmode-qt there's a branch that makes the plugin draw directly on the window. This produces flickering artifacts that I think are the result of bad interaction betweeen XRender and X normal drawing. - The branch at http://git.collabora.co.uk/?p=user/marcoil/webkit.git;a=shortlog;h=refs/heads/marcoil/plugins-wmode-qt-offscreen uses an off-screen pixmap and makes the plugin render into it. The problem is that uninitalized pixmaps have no alpha (thus no transparency). If alpha is added to the pixmap, then Gtk (from the plugins) complains of non-compatible colormaps between the pixmap and the screen. I have no idea on how to solve these issues, so I'm publishing these in the hope someone else will be able to improve on it. I'm no longer working on this, so taking me out. Created attachment 33666 [details]
patch, not for review
This patch is not final. It contains 2 ways of passing the X11 drawable to the plugin. At this point, none of them fully work. I will keep working on it after my vacation.
Hi Yael, I cleaned up some of your event handling code. Please consult my upcoming patch. Created attachment 34003 [details]
Implement keyboard event forwarding for windowless plugins
Implement keyboard event forwarding for windowless plugins.
Windowless plugins are not there yet, but this is one small step of the way.
Created attachment 34004 [details]
Implement keyboard event forwarding for windowless plugins
ChangeLog fixes
Comment on attachment 34004 [details] Implement keyboard event forwarding for windowless plugins r=me > > Based on work my Yael Aharon. I guess that should become a "by" when landing ;-) > +bool PluginView::dispatchNPEvent(NPEvent& event) A ### TODO would be nice here to point out that we should unify this function across all ports. Comment on attachment 34004 [details]
Implement keyboard event forwarding for windowless plugins
Landed in 46733
I am working on this bug. Quick status - I am currently working outside webkit and trying to fix the problem faced by Marc. When backing store enabled, I see two approaches: 1. Provide plugin with a Drawable that already has content of Qt's backing store. So, we just grab the backing store contents into a Drawable before the plugin's paint event, give it to the plugin and after the plugin paint we put the Drawable back in the backing store. I think, this will work well even if use ARGB visuals but this involves copying back and forth will make it slow. 2. Make plugin paint directly to Qt's backing store. Will be much faster. When backing store disabled, we can assume paint on screen. And make the plugin draw directly to the window. This will flicker but they asked for it (as in marc's patch). I have been testing with the diamondx plugin (with a windowless patch from bug 137189 of mozilla). You can find the git repo here - http://git.forwardbias.in/?p=diamondx.git;a=shortlog;h=refs/heads/windowless. I have managed to get approach 1 to work outside webkit, remains to be seen if it will work with webkit :) It's flicker free. I wrote a pure qt4 'host' that loads a netscape plugin. http://git.forwardbias.in/?p=npploader.git;a=shortlog;h=refs/heads/grab_bs. It's _very_ hacky. Ideally, we should use QWindowSurface (which is exported but private). Also, the gdk code and Qt code use different X11 displays. So to sync the painting in the drawable, I have to end up calling XSync :/ I am working on approach 2, but I am getting all sorts of gdk errors atm. You should notice that some plugins are using the glib refcounting which can lead to problems (like with the Flash plugin). The problem is that the GtkSocket takes a ref on the GtkPlug, which never happens when using a QX11EmbedContainer, thus we are always left with 1 ref less, which means that the plugin is sometimes destroying parts of itself during resize etc. Ok, approach 2 works too. http://git.forwardbias.in/?p=npploader.git;a=shortlog;h=refs/heads/direct_bs. I will try integrating the approach into qt/webkit now. Before integrating to WebKit, I thought I should first test with flash instead of diamondx. I had to make a lot of changes to my npploader. After a lot of pain, transparent flash works in npploader! http://git.forwardbias.in/?p=npploader.git;a=shortlog;h=refs/heads/master Now to integrate it into Qt/WebKit. (In reply to comment #16) > Before integrating to WebKit, I thought I should first test with flash instead > of diamondx. I had to make a lot of changes to my npploader. After a lot of > pain, transparent flash works in npploader! > > http://git.forwardbias.in/?p=npploader.git;a=shortlog;h=refs/heads/master > > Now to integrate it into Qt/WebKit. Cool news! thanks for the great work Girish! Created attachment 40102 [details]
Windowless patch - makes painting work
Painting works with windowless plugins with no flicker. Does not work when the view is scrolled, I need to get the scroll offsets right.
My initial version drew directly to the backing store. But it has a couple of problems - the backing store offset changes when new widgets appear _over_ the QWebView. This means that we need to detect this (which is very hard) and also do a setwindow call. The code says that calling setwindow multiple times crashes plugin (doesn't say which plugin). In general, I thought it was safer to just use a offscreen drawable.
Builds over yael's patch, the main fixes are how the drawable is grabbed, painted by plugin.
I need comment on the below and on the patch in general:
1. The gdkDisplay is a part of the PluginPackage, I am not sure if this is the right place. I was hoping that after a plugin is loaded (but no new plugin instance created), we use RTLD_NOLOAD using dlopen to detect if gdk-x11 is already open. There seems to be no mechanism to achieve this using QLibrary (besides QLibrary will load gdk-x11 even if the plugin does not need it).
One option is to load gdk-x11 *always* on startup and make Qt use the gdk default display. However, this does not work (Qt bug). Note that, if I do a XOpenDisplay manually and provide that display to Qt, it works. Just not with gdk's display, I haven't debugged this problem. (In reply to comment #19) > One option is to load gdk-x11 *always* on startup and make Qt use the gdk > default display. However, this does not work (Qt bug). Note that, if I do a > XOpenDisplay manually and provide that display to Qt, it works. Just not with > gdk's display, I haven't debugged this problem. I think always loading gdk-x11 is the best option. Flushing the gdk display shouldn't be too bad. But why do you have to flush the Qt display? Can't you simply use QPainter::drawPixmap to paint the pixmap? We cannot change the way people construct their QApplication or change the existing Qt connection. Ah, of course, We cannot change the way people construct their QApplication or change the existing Qt connection. What was I thinking... Flushing Qt connection is required so that changes to the m_drawable are reflected to the plugin. Created attachment 40107 [details]
Windowless patch - makes painting work (2)
* Moved gdk display detection code to PluginViewQt (Simon's suggestion)
* Fixed coding style
(In reply to comment #18) > QWebView. This means that we need to detect this (which is very hard) and also > do a setwindow call. The code says that calling setwindow multiple times > crashes plugin (doesn't say which plugin). This is due to Flash crashing when setWindow is called with a different size. Flash destroys its internals and reconstructs them, but as it has one ref less (due to us not using GtkSocket) it becomes 0 and some internals die and result in a crash. As you are dealing with windowless you might not have this problem. Created attachment 40216 [details]
Windowless patch - makes painting work (3)
Scrolling works now. The rendering is now ready for review.
Here's my todo list for this feature to be 'complete': 1. Make it work with QWebGraphicsItem. We most likely cannot support transparent mode but opaque mode should work. 2. Make it work with printing 3. Test grabWidget 4. Test Keyboard and Focus (some code already exists) 5. Implement Mouse handling (Click, Wheel) 6. Check if dialogs and widgets displayed by plugins work (flash shows gtk popup menus and dialogs) 7. Optimize opaque mode. Need not XCopyArea from the backing store to drawable. Let me know, if I am missed something. Comment on attachment 40216 [details]
Windowless patch - makes painting work (3)
This is a great start!
After a quick discussion with Girish on IRC we agreed that makes more sense to fix point 4/5 and 6 in the TODO before landing. Otherwise I don't see any issues with the existing patch, except that it would be good to replace the unconditional enabling of the supportswindowless variable with something slightly more conditional depending on the availability of the backing store for example.
Created attachment 40294 [details]
Windowless patch - painting and event handling (4)
4,5,7 now work. So painting, mouse, focus, keyboard handling should all work.
6, I am not entirely sure. The diamondx plugin manages to show popups and the gtk file dialog. However, the flash plugin fails to show the context menu. This seems to be the case with chrome/windowless too. So, maybe flash decides not to show the context menu for some reason. No idea how to fix this/proceed. Is this a blocker for landing?
Also fixes the case of not having a backing store, we just use the pixmap and painter (no content propagation).
check-webkit-style reports no errors :)
Created attachment 40299 [details]
Windowless patch - painting and event handling (5)
New patch after review by kenneth on irc.
Changes:
1. Create static funcs for setting the XEvent (follow the trend for setting keyboard event).
2. Add better comment for why setNPWindowRect is not called multiple times for windowed/full mode.
3. Add comment for -offset
4. s/hasBackingStore/hasValidBackingStore
5. Display detection code moved to getPluginDisplay()
6. Use QX11Info * in platformStart()
Created attachment 40345 [details]
Windowless patch - painting and event handling (6)
Some updates.
Context menu problem - This is a gtk/qt integration issue and is not a problem that can be solved by this patch. If you use gtk inside Qt application, we are unable to show a gtk context menu. I am not 100% sure why but this appears to be because of gtk failing to grab mouse since Qt has grabbed it. Problem reproducible with diamondx plugin.
GrabWidget - works, with no changes - yay, we can see flash in screenshots :)
Also, I ended up modifying the original patch with the following changes (after reading mozilla code)
1. Implement enter/leave notifications
2. Implement focusin/out.
3. I also made the setX... functions pointer based - I couldn't find any coding policy on this for WebKit but the Qt way atleast is to pass a pointer (instead of a ref) when the object gets modified.
Here's the bug list, so it can help us decide if we want to land it: 1. Focusing the plugin does not defocus webview. The focus handling is correct and the keyevents are sent correctly. But the lineedit/input cursor keeps blinking. (Side note: There seems to be a general focus problem where the cursor does not blink at all when used with plugins - unrelated to this patch) 2. Flash get confused when the mouse release happens outside the plugin (because we don't generate a release). 3. When a messagebox pops up, right when the flash gets focus (because of the onblur handler of input) it will start selecting text in lineedit/input even when no mouse button is pressed. Strangely, this happens in chrome too. 4. QGraphicsWebView does not work 5. Printing crashes 6. Using ARGB visuals crashes (In reply to comment #30) > Here's the bug list, so it can help us decide if we want to land it: > 1. Focusing the plugin does not defocus webview. The focus handling is correct > and the keyevents are sent correctly. But the lineedit/input cursor keeps > blinking. (Side note: There seems to be a general focus problem where the > cursor does not blink at all when used with plugins - unrelated to this patch) With windowed plugins this works because the focus is transferred on X level and QWebView/QWebPage receive a FocusOut event, that will deactivate the focuscontroller. We may have to emulate that behaviour. > 6. Using ARGB visuals crashes Oops, given the popularity of composition managers these days that's a problem. Would a backingstore-less painting help here? > With windowed plugins this works because the focus is transferred on X level > and QWebView/QWebPage receive a FocusOut event, that will deactivate the > focuscontroller. We may have to emulate that behaviour. > I have tried to do this, but doesn't help. See the initial lines of PluginView::handleMouseEvent. Do I need to do anything else? > > 6. Using ARGB visuals crashes > > Oops, given the popularity of composition managers these days that's a problem. > > Would a backingstore-less painting help here? To clarify, the existence of a compmgr does not crash webkit. It crashes only when Qt uses ARGB visuals (which I think is not so common) i.e when Qt::WA_TranslucentWindow is used. (In reply to comment #32) > > With windowed plugins this works because the focus is transferred on X level > > and QWebView/QWebPage receive a FocusOut event, that will deactivate the > > focuscontroller. We may have to emulate that behaviour. > > > I have tried to do this, but doesn't help. See the initial lines of > PluginView::handleMouseEvent. Do I need to do anything else? Ahh, I found the potential solution: Try using PluginView::focusPluginElement(). > > > 6. Using ARGB visuals crashes > > > > Oops, given the popularity of composition managers these days that's a problem. > > > > Would a backingstore-less painting help here? > > To clarify, the existence of a compmgr does not crash webkit. It crashes only > when Qt uses ARGB visuals (which I think is not so common) i.e when > Qt::WA_TranslucentWindow is used. Hmm, I see... you're right, that makes it slightly less vulnerable. Not a showstopper then IMHO. But would be great to at least avoid the crash if it's easy to detect :) (maybe by comparing the default depths of qt and gtk pixmaps?) Created attachment 40422 [details]
Windowless patch - painting and event handling (7)
Can we try to land this and get some blackteam testing?
Changes since last patch:
1. Don't crash when printing. We just skip painting for the time being.
2. We provide a 'white' background for transparent plugins when we have no backing store.
At this point, we are feature par with the windowed mode plugin :)
Some good news and some depressing news. The good news - I had initially tried to get the plugin to paint to a 32-bit QPixmap. I could not succeed because it was giving me X match errors. I have now figured the problem and solved it. I am able to get DiamondX to paint, even! The beauty of this approach is that it's very clean and we can get it to work in qwebview, graphicsview, printing all in a single shot. The depressing news - Flash does not paint. No errors, nothing. :| Anyone has friends at Adobe, I can get in touch with :) ? Otherwise, it's back to 24-bit QPixmap and all sort of hackery to make it work with different paint devices. Comment on attachment 40422 [details] Windowless patch - painting and event handling (7) > +#include <QLabel> I guess this isn't really needed ;-) > + XCopyArea(QX11Info::display(), backingStorePixmap->handle(), m_drawable.handle(), gc, > + offset.x() + m_windowRect.x() + m_clipRect.x(), offset.y() + m_windowRect.y() + m_clipRect.y(), There's a trailing whitespace after the above two lines!!! (OMG, I would've never thought I'd get that anal in a patch review ;-) > setCallingPlugin(true); > - bool accepted = m_plugin->pluginFuncs(); > + bool accepted = m_plugin->pluginFuncs()->event(m_instance, &event); This is my favourite part of the patch. > + const QX11Info *x11Info = 0; Coding style :) Regardless of my silly style picks this patch is a great piece of work! Created attachment 40500 [details]
Windowless patch - painting and event handling (8)
Changes since last patch
* Simon's last review (coding style, whitespace)
* Fixes crash reported by Simon in some sites (e.g, facebook/biotronic game)
* Don't paint anything when the x11 graphics system is not being used i.e check if pixmap handle is valid.
- This can be fixed by using plain X Pixmap. However, I will do this later.
So what is left on the TODO now, before this can be considered for inclusion? Does it work with zoom? does it just zoom it as it was an image or actually set another size on the plugin? What does firefox do here? in firefox, the associated layout element for a given plugin (<object> or <embed> tags) will be zoomed and the new size will bubbled until it reaches the embeded xwindow plugin (so flash would be resized accordingly to page zoom)
> Does it work with zoom? does it just zoom it as it was an image or actually set
> another size on the plugin? What does firefox do here?
Kenneth, thanks for reminding me about zooming :) I forgot to test it with that. Important history is being lost, so I put up the patch in http://gitorious.org/~girish/qtwebkit/girishs-clone/commits/windowless. Changes include: 1. Use X Pixmaps (93c3aaf) 2. Add PluginQuirkUsesScreenDefaultVisual for Flash (654645). Inspired by Kenneth's suggestion on irc. 3. Make zoom work (06e79ab) I think it's ready for landing. Here are the pending bugs: 1. Printing does not work (but no crash). 2. -graphicssystem opengl crashes. We cannot paint X11 Pixmap using QPainter. I will disable this in a separate patch (since we need this history) 3. QGVLauncher crashes on exit - WebCore::Chrome::repaint uses the client. The client is alreay gone by then. 4. QGraphicsWebView works, yay! But I need a way to detect painting on qgv and turn off transparency. Any suggestions? 5. When using ARGB visuals on tlw, mouse over crashes. In the process of debugging 5, I think I have found a bug. It appears that svn commit 48604 (0292c6) has introduced a bug - it calls the view's winId(). This actually ends up creating a native window. I don't think is what we want. For the windowless case, we want the window's winId (and not the views). Even in the windowed case, only the container needs to have a winId and not the view itself. Simon, Kenneth, is that correct? Patches with the change log are at http://gitorious.org/~girish/qtwebkit/girishs-clone/commits/windowless_20081 Created attachment 40625 [details]
Patch 1/4 - [Qt] Add support for windowless NPAPI plugins
Created attachment 40626 [details]
Patch 2/4 - Use X Pixmap instead of QPixmap.
Created attachment 40627 [details]
Patch 3/4 - Add PluginQuirkUsesScreenDefaultVisual for Flash.
Created attachment 40628 [details]
Patch 4/4 - Make painting and events work when page is zoomed.
Comment on attachment 40626 [details]
Patch 2/4 - Use X Pixmap instead of QPixmap.
tools like git-send-bugzilla or bugzilla-tool post-diff/post-commits can take care of setting the correct mime type and patch checkbox for you.
Comment on attachment 40626 [details]
Patch 2/4 - Use X Pixmap instead of QPixmap.
The ChangeLog needs a little tweak though to mention that this is in the Qt windowless plugin support. I'll fix that during landing.
Comment on attachment 40627 [details] Patch 3/4 - Add PluginQuirkUsesScreenDefaultVisual for Flash. There are a few coding style issues left and one detail: > + > + No new tests. (OOPS!) Please remove this line :) > + ((NPSetWindowCallbackStruct *)m_npWindow.ws_info)->depth); Coding style :) > + const int drawableDepth = ((NPSetWindowCallbackStruct *)m_npWindow.ws_info)->depth; Same here. > +#ifndef QT_NO_XRENDER > + static const bool useXrender = qgetenv("QT_X11_NO_XRENDER").isNull(); // Should also check for XRender >= 0.5 > +#else > + static const bool useXrender = false; > +#endif Kenneth suggested on chat that this should perhaps be useXRender? Created attachment 40709 [details]
Patch 3/4 - Add PluginQuirkUsesScreenDefaultVisual for Flash (Take 2)
Renamed quirk as discussed on irc.
Fixed coding style.
Renamed to useXRender
Added "[Qt] Windowless plugins: " prefix
Created attachment 40710 [details]
Patch 4/4 - Make painting and events work when page is zoomed. (Take 2)
Added "[Qt] Windowless plugins: " prefix
Created attachment 40711 [details]
Enable printing
Bug in Qt has been fixed by Samuel. It is already part of 4.6 branch.
Comment on attachment 40709 [details] Patch 3/4 - Add PluginQuirkUsesScreenDefaultVisual for Flash (Take 2) Minor coding style issue left, but I'll fix that when landing. > + QPixmap* backingStorePixmap = static_cast<QPixmap *>(backingStoreDevice); All patches landed in 49158, 49159, 49169, 49170 and 49171 Here's the TODO list: 1. -graphicssystem opengl crashes. UPDATE: It turns out that it crashes only on my machine. This is supported by Qt. 2. QGVLauncher crashes on exit - WebCore::Chrome::repaint uses the client. The client is alreay gone by then. 3. QGraphicsWebView works, yay! But I need a way to detect painting on qgv and turn off transparency. Any suggestions? UPDATE: Will use pluginParent() and do a ugly qobject_cast 4. When using ARGB visuals on tlw, mouse over crashes. UPDATE: Should be solved with the introduction of pluginParent() and removing winId(). 5. Remove the non-transparent sync for opaque mode. 6. Context menu. UPDATE: This requires integration of context menus of gtk within Qt. The toolkits have to sync up on mouse/keyboard grabs. Qt has to be fixed, the Qt developers are aware of this problem (but have no solution). No fix is required in WebKit. Simon and I agreed that this bug is exhausted. So, I will create separate bugs for the above TODO list items. The QGVLauncher has support for flipping the view upside down. Doing this with a windowless plugin results in the page being painted all wrong :-) Can be tested with http://www.popup-killer-review.com/windowless-swf.htm (In reply to comment #55) > The QGVLauncher has support for flipping the view upside down. > > Doing this with a windowless plugin results in the page being painted all wrong > :-) > > Can be tested with http://www.popup-killer-review.com/windowless-swf.htm Indeed, it's TODO item 3 :) I have a patch at https://bugs.webkit.org/show_bug.cgi?id=30149 A note on the quirk mode added to flash. The problem is that flash appears to be using gdk_visual_get_system() to draw to the Drawable instead of the visual provided by us. As a result, when provided with transparent pixmap (which requires a 32-bit visual) flash fails because it uses the system default visual (for example, 24-bit). To verify that is indeed the case, I shadowed the above gdk symbol (gdk_visual_get_system) to return a 32-bit visual. And it works with transparent pixmaps. This 'hack' was found unsuitable for Qt/WebKit since many libraries have started using -Bsymbolic-functions and probably not possible with all platforms/compilers. Comparing the calls to plugins in PluginView.cpp and in PluginViewQt.cpp I noticed 2 differences and I was wondering if they are intentional
In PluginView.cpp:
JSC::JSLock::DropAllLocks dropAllLocks(<b>JSC::SilenceAssertionsOnly</b>);
...
PluginView::setCurrentPluginView(0);
In PluginViewQt.cpp:
JSC::JSLock::DropAllLocks dropAllLocks(<b>false</b>);
and we never call
PluginView::setCurrentPluginView(0);
(In reply to comment #58) > Comparing the calls to plugins in PluginView.cpp and in PluginViewQt.cpp I > noticed 2 differences and I was wondering if they are intentional > > In PluginView.cpp: > JSC::JSLock::DropAllLocks dropAllLocks(<b>JSC::SilenceAssertionsOnly</b>); > ... > PluginView::setCurrentPluginView(0); > > In PluginViewQt.cpp: > JSC::JSLock::DropAllLocks dropAllLocks(<b>false</b>); > and we never call > PluginView::setCurrentPluginView(0); It's not intentional. I don't know anything about the above code other than it locks js code for mulithread access. There is no bool argument to dropAllLocks, so it appears that it is wrong. And setCurrentPluginView(0) is also appears to be needed (I can't see what it is). I will create patch. *** Bug 260998 has been marked as a duplicate of this bug. *** Seen from the domain http://volichat.com Page where seen: http://volichat.com/adult-chat-rooms Marked for reference. Resolved as fixed @bugzilla. |