Bug 14750 – [gtk] Implement plugin support in GTK backend

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Bastien Nocera]

The GTK+ backend currently doesn't implement any plug-ins support. It should do so probably supporting the old NPAPI with XEmbed and GTK2 support, as well as using the more native API used by other ports.

[Kai Hendry]

Jean-Charles VERDIE from Pleyo (they are doing some mobile Webkit port) mentioned that he might be working on plugin support. I wonder if there is any overlap.

[Jean-Charles Verdié]

To be clearer: We are trying to understand what has been done on the windows port, split the common part and the specific part, then to understand how it works and give this code back to webkit to make it possible to implement using different backend while we won't do more than re-implementing Windows (for testing purposes) and our own "owBal" implementation (ours at sand-labs.org)

[Rodney Dawes]

(In reply to comment #2)
> To be clearer: We are trying to understand what has been done on the windows
> port, split the common part and the specific part, then to understand how it
> works and give this code back to webkit to make it possible to implement using
> different backend while we won't do more than re-implementing Windows (for
> testing purposes) and our own "owBal" implementation (ours at sand-labs.org)

Is this plug-in code viewable somewhere?

[Rodney Dawes]

Created attachment 16998 [details]
Start at plug-ins patch

Here is my patch which mostly implements everything needed for plug-ins to work. It's got some debug printfs() lying around, and the code isn't perfectly matching the style guide yet. If flash is found, it will crash the browser on pages that use it. Flash requires use of X toolkit bits still, and without these, it simply just crashes whatever loads it. Implementing this requires a custom widget to integrate the necessary bits with GTK+. Unfortunately, the GtkSocket I'm trying to use isn't getting sized properly or rendered on screen. It seems to get parented, mapped, and the plug-in seems to get embedded. But, the size never gets allocated to anything > 1x1, with a position of -1,-1. I need to get this widget to embed and render properly before I can really do anything else, unfortunately. Feel free to try the patch though. It would be great if someone could figure out why it's not working right. I still don't understand exactly how the WebKitPage parenting and stuff is supposed to work.

[Mark Rowe (bdash)]

Alp helped Rodney work out the issue with the widget sizing/positioning, and I think he's back underway on his plugin patch.

[Rodney Dawes]

Created attachment 17239 [details]
Update plug-ins patch

Here is an updated version of the plug-ins patch. This version only supports Xt windowed plug-ins. Next version of the patch will support both. Adobe Flash 9 works with this version. You can watch the videos on http://homestarrunner.com/ . Youtube and similar flash video player sites will load the flash, but only stream the first N bytes of the actual video to the player. This is because the PluginStreamGtk code is not fully complete. Hopefully I should have it fixed in the next version also. I'm also having trouble getting the Xt plug-ins to scroll correctly. It doesn't seem to update the position properly. But all in all it's nice to see flash working. :)

[Rodney Dawes]

Created attachment 17273 [details]
Updated patch for the Shared type removal

[Nigel Tao]

Just with the patch, please replace tabs by spaces.  http://webkit.org/coding/coding-style.html

[Rodney Dawes]

Created attachment 17297 [details]
Updated patch to handle XEmbed and Xt plug-ins both

Ok. Here is an updated version of the patch. It handles both XEmbed and Xt plug-ins the same way gecko does, by checking the value of NPPVneedsXEmbed with the plug-in's GetValue implementation. I'd like to start splitting up the patch logically now and getting some pieces in. Perhaps the PluginDatabaseGtk impl and the ability to build with XP_UNIX enabled (but only when building for X11). Are there standard locations for plug-ins on win32 and osx that we should be checking? I'd like to get that support in PluginDatabaseGtk also.

[Kai Hendry]

Built and tested dobey's "Updated patch to handle XEmbed and Xt plug-ins both" patch with a r27911 WebKit checkout.

Macromedia Flash playback on YouTube worked. =)

Be good to start merging this patch in. I hope to help dobey to improve the NPAPI implementation with some scriptability tests.

[Rodney Dawes]

Created attachment 17851 [details]
Updated to build against trunk

[Rodney Dawes]

Created attachment 17869 [details]
Updated patch for KURL class changes and without other patches included

This is an updated version of the patch for the changes to the KURL class. This version of the patch also does not include changes from other patches, as previous versions did. You will need the patches in the bugs I'm adding as dependencies as well for a full experience.

[Rodney Dawes]

I can't change the DependsOn field it seems, so you will need the patches from bugs #16389 #15669 and #16071 as well.

[Rodney Dawes]

Created attachment 17880 [details]
Updated Giant Patch With Everything

There are still some conflicts between patches, so here is another huge patch with all the changes needed.

[Rodney Dawes]

Created attachment 17963 [details]
Updated giant patch for trunk

[Rodney Dawes]

Created attachment 17973 [details]
Updated patch for trunk today

[Kai Hendry]

Created attachment 17974 [details]
NPAPI scriptability test

http://wiki.webvm.net/ is the site where the Aplix Corporation is co-ordinating their NPAPI plugin efforts.

[Rodney Dawes]

Created attachment 18017 [details]
Update patch against r28911

[Rodney Dawes]

Created attachment 18094 [details]
Updated plug-ins patch

This is just a quick update to fix some warnings about variable initialization ordering in plugins/gtk/ and use of char* instead of const char* for string constants in PluginDebug.h.

[Rodney Dawes]

Created attachment 18096 [details]
Updated patch to fix scrolling too

[Rodney Dawes]

Created attachment 18108 [details]
Update to fix a Gtk-CRITICAL when destroying the widget

[Rodney Dawes]

Created attachment 18239 [details]
Updated patch to build with autotools also, against r29084

[Mark Rowe (bdash)]

Comment on attachment 18239 [details]
Updated patch to build with autotools also, against r29084

Sorry that this has been relatively untouched for so long.

There are a few relatively serious issues here:  PluginViewGtk has a large amount of duplicated code with PluginViewWin, some of which could trivially be factored out (handlePost, parseRFC822HeaderFields and friends).  It also contains many chunks of commented-out code, unused variables (kWebPluginViewWindowClassName, kWebPluginViewProperty) and plugin quirks which are probably not needed on non-Windows platforms.  I'm not sure that PluginMessageThrottlerGtk would be needed on non-Windows platforms as I believe the Windows equivalent exists due to peculiarities with paint events and the Windows message queue, but I suspect Anders would know more about that.  I suspect the references to XID might need to be conditional on X11 being available, both in PluginViewGtk.cpp and PluginViewGtk.h.  PluginViewGtk seems to contain a bunch of references to "wndProc" still which is a Windows concept that seems out of place in this Gtk code.

PluginStreamGtk seems to have a large amount of duplicate code with PluginStreamWin, only the Gtk version looks to be broken in the case of NP_ASFILE/NP_ASFILEONLY streams.  It should be possible to share the majority of this code with Windows, and eventually share with the Mac when that code is refactored.

With a bit more polish we should be able to get this landed soon.

[Rodney Dawes]

Created attachment 18331 [details]
Updated patch against r29305

[Rodney Dawes]

Created attachment 18356 [details]
Updated patch against r29338

This patch also resolves several of the issues mentioned in Mark's quick review. Curl also seems to be crashing quite a bit for me now as well. I'm not sure why.

[Michael Emmel]

(In reply to comment #25)
> Created an attachment (id=18356) [edit]
> Updated patch against r29338
> 
> This patch also resolves several of the issues mentioned in Mark's quick
> review. Curl also seems to be crashing quite a bit for me now as well. I'm not
> sure why.
> 

If your using files instead of http the version in the head probably will crash.
Its got quite a few nasty bugs that are not obvious.
I've got another version that uses the latest curl with file handles handed to the OS for select works much better than polling.

[Rodney Dawes]

(In reply to comment #26)
> If your using files instead of http the version in the head probably will
> crash.
> Its got quite a few nasty bugs that are not obvious.
> I've got another version that uses the latest curl with file handles handed to
> the OS for select works much better than polling.

No, it's crashing when trying to stream data for flash. Most notably it occurs with http://burststudio.com/kitten.html after clicking "start game" or when trying to seek past the buffered data in a flash video player, such as on youtube.

[Michael Emmel]

(In reply to comment #27)
> (In reply to comment #26)
> > If your using files instead of http the version in the head probably will
> > crash.
> > Its got quite a few nasty bugs that are not obvious.
> > I've got another version that uses the latest curl with file handles handed to
> > the OS for select works much better than polling.
> 
> No, it's crashing when trying to stream data for flash. Most notably it occurs
> with http://burststudio.com/kitten.html after clicking "start game" or when
> trying to seek past the buffered data in a flash video player, such as on
> youtube.
> 

Yeah I could see streaming being a problem also.
Anything but a reasonably a http connection that closes fast enough will probably crash.

I'm meeting with Alp Toker in a few weeks and I'll get a patch out soon after. Right now I've got to get a demo going.

The new version is doing stuff like this.
    curl_multi_setopt(d->m_curlMultiHandle, CURLMOPT_SOCKETFUNCTION, socketCallback);
    curl_multi_setopt(d->m_curlMultiHandle, CURLMOPT_SOCKETDATA,this);
    curl_multi_setopt(d->m_curlMultiHandle, CURLMOPT_TIMERFUNCTION,timerCallback);
    curl_multi_setopt(d->m_curlMultiHandle, CURLMOPT_TIMERDATA,this);

Which is a lot better and has not crashed yet for me.
Curl still has a few bogus timer callbacks but this can be fixed a bit later.
The big issue left is msg's to the multi handle should be possible from a callback. But I'll need to send a patch in for curl itself to fix this.

[Rodney Dawes]

Created attachment 18473 [details]
Updated patch against r29531

[Rodney Dawes]

Created attachment 18606 [details]
Updated patch against r29725

[Alp Toker]

(In reply to comment #30)
> Created an attachment (id=18606) [edit]
> Updated patch against r29725
> 

Could you add a comment about gtk2xtbin.c and any other imported sources in the ChangeLog entry?

We should try to keep track of all "bundled" source code from other projects so we can keep up to date with their bugfixes, and maybe even send back our fixes to the code.

If the intention is not to bundle the code but to adopt it, then it needs to be modified to follow WebKit coding style. Bundled code should keep upstream coding style -- i think in this case it's better to treat the code as bundled to avoid needless changes. What do you think?

[Rodney Dawes]

Created attachment 18623 [details]
Updated patch against r29742

[Rodney Dawes]

Created attachment 18691 [details]
Updated patch against r29792

This is an updated patch to deal with the new PluginQuirkSet class introduced in recent builds.

[Rodney Dawes]

> Could you add a comment about gtk2xtbin.c and any other imported sources in the
> ChangeLog entry?

That's the plan, yes.

[Rodney Dawes]

Created attachment 18849 [details]
Updated patch against r29914

[Alp Toker]

(In reply to comment #35)
> Created an attachment (id=18849) [edit]
> Updated patch against r29914
> 

Is this patch intended for review? It won't be looked at unless it's set r? and has ChangeLog entries.

I noticed a few tabs where they shouldn't be. I'm still interested in finding out what the origin of the bundled code is. Having some kind of trail is important.

[Rodney Dawes]

(In reply to comment #36)
> Is this patch intended for review? It won't be looked at unless it's set r? and
> has ChangeLog entries.

Not entirely yet. Due to the need to refactor the code, this patch will have to be updated when the refactoring of the win32 plug-in code to be shared, to use the shared implementation for most things.

> I noticed a few tabs where they shouldn't be. I'm still interested in finding
> out what the origin of the bundled code is. Having some kind of trail is
> important.

The "original author" is in the code's license block, and as I said before, in comment 34, the origin of the code will also be noted in the ChangeLog when it gets written (after all the refactoring work).

[Rodney Dawes]

Created attachment 18918 [details]
Updated patch against r29975

[Rodney Dawes]

Created attachment 18936 [details]
Updated patch against r30005

[Gwenole Beauchesne]

Created attachment 19342 [details]
64-bit fixes

This is a patch to fix 64-bit issues brought by the NPAPI/gtk giant patch. I have only tested with Flash 9.0.115 and webkit safari-3.1-branch.

[Rodney Dawes]

Created attachment 19936 [details]
Updated patch against r31217

Here's an updated patch against r31217 which works with the refactored plugin api code.

[Jasper Bryant-Greene]

I think the MPL licensing needs to be deleted in that patch so that the GPL applies instead?

If it's now ready for review then please add a ChangeLog entry and mark it r?

[Mark Rowe (bdash)]

(In reply to comment #42)
> I think the MPL licensing needs to be deleted in that patch so that the GPL
> applies instead?

The GPL is definitely not acceptable (Perhaps you meant LGPL?).  I think it may be preferable to leave the original license header on the code to indicate its origin and the license under which we have adopted it.  I'm not an expert on these sorts of things though.

[Rodney Dawes]

Created attachment 20011 [details]
Updated patch

[Jasper Bryant-Greene]

(In reply to comment #43)
> The GPL is definitely not acceptable (Perhaps you meant LGPL?).  I think it may
> be preferable to leave the original license header on the code to indicate its
> origin and the license under which we have adopted it.  I'm not an expert on
> these sorts of things though.

Sorry, I did mean LGPL. The reason I say that the MPL should be deleted is that the MPL specifically says this:

* Alternatively, the contents of this file may be used under the terms of
* either the GNU General Public License Version 2 or later (the "GPL"), or
* the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
* in which case the provisions of the GPL or the LGPL are applicable instead
* of those above. If you wish to allow use of your version of this file only
* under the terms of either the GPL or the LGPL, and not to allow others to
* use your version of this file under the terms of the MPL, indicate your
* decision by deleting the provisions above and replace them with the notice
* and other provisions required by the GPL or the LGPL. If you do not delete
* the provisions above, a recipient may use your version of this file under
* the terms of any one of the MPL, the GPL or the LGPL.

[Mike Hommey]

(In reply to comment #45)
> Sorry, I did mean LGPL. The reason I say that the MPL should be deleted is that
> the MPL specifically says this:
(...)

But why would you want that ? It would be better for everyont if you kept the original license, so that modifications made here could be taken back under the original tri-license terms.

[Jasper Bryant-Greene]

(In reply to comment #46)
> (In reply to comment #45)
> > Sorry, I did mean LGPL. The reason I say that the MPL should be deleted is that
> > the MPL specifically says this:
> (...)
> 
> But why would you want that ? It would be better for everyont if you kept the
> original license, so that modifications made here could be taken back under the
> original tri-license terms.

The reason I think it would be necessary is that code contributed to WebKit needs to be licensed under either LGPL or new BSD. If the license gives the user the option to take it under the GPL or MPL, I would think that is incompatible.

However there may have been some confusion here. Is this code merely being bundled, and not actually being contributed to form part of WebKit? If that's the case then of course there's no problem with keeping the original license.

[Mike Hommey]

(In reply to comment #47)
> The reason I think it would be necessary is that code contributed to WebKit
> needs to be licensed under either LGPL or new BSD. If the license gives the
> user the option to take it under the GPL or MPL, I would think that is
> incompatible.

There is no incompatibility with having more licensing terms for a snippet of code than for the rest of the tree, as far as licenses are compatible. And obviously, a code snippet that is licensed under the terms of MPL, GPL *and* LGPL can be linked to LGPL or BSD licensed code.

(BTW, code contributed to WebKit is usually licensed under LGPL, 3-clause BSD *or* 2-clause BSD, which is actually quite annoying, because each file has a different license. Why not have *all* dual-licensed under 3-clause BSD and LGPL ?)

[Rodney Dawes]

Created attachment 20098 [details]
Updated patch against r31326

Here is an updated patch which removes a couple extra lines of debugging outut, and fixes a crash when calling refresh() with an already populated PluginDatabase, exposed by http://www.java.com/en/download/installed.jsp .

[Adam Williamson]

I'm trying to add this patch to the Mandriva webkit package. I had to rediff it a bit for current SVN webkit, but finally got it to compile clean. However, it doesn't seem to actually work...I'm attaching my rediffed version of the patch. (It also has the ChangeLog bits taken out as it has to apply against the snapshot tarballs for us, not against the tree). a) hope some of the rediffing is useful and b) does anyone know what might be wrong? When I try to use, say, YouTube - in Epiphany or Midori - with the patched webkitgtk, I just get the same 'no Flash or JavaScript disabled' error.

[Adam Williamson]

Created attachment 20604 [details]
alternative updated version of patch (against 31893, despite the filename)

this is an amateur attempt at a rediff of the 31326 patch against 31893. as noted in my comment, it doesn't seem to work for me, for some reason. all ChangeLog changes have been taken out, so it applies against the snapshot tarball. if any of my changes are useful they can happily be used under any licensing terms at all.

[Sriram Neelakandan]

Created attachment 20617 [details]
patch for Plugin updated to r31990

Older Plugin patch has been updated to r31990
- Modified original patch to use Directory instead of Path
- Implemented getFileModificationTime for WebCore/platform/gtk/FileSystemGtk.cpp

The getFileModificationTime is necessary for plugins to load as per the new changes in the trunk.

[Sriram Neelakandan]

Comment on attachment 20617 [details]
patch for Plugin updated to r31990

NOTE: the earlier patch 
https://bugs.webkit.org/attachment.cgi?id=20098
has been adopted for r31990

[Adam Williamson]

Sriram, your updated patch still refers to PluginDataGtk.cpp in GNUmakefile.am , even though you removed that file from the patch, so it doesn't build. That line should be taken out of GNUmakefile.am , I guess.

[Adam Williamson]

Same thing with PluginPackageGtk.cpp .

Is this patch just broken? Or is it meant to be a serial patch together with 31326, in which case it's still wrong as it still contains some of the same things as 31326?

[Sriram Neelakandan]

(In reply to comment #55)
> Same thing with PluginPackageGtk.cpp .
> 
> Is this patch just broken? Or is it meant to be a serial patch together with
> 31326, in which case it's still wrong as it still contains some of the same
> things as 31326?
> 
Thanks Adam.
I found the problem .. i did svn-create-patch .. and some files were not added to snv. 
I will fix that and upload a new patch 

[Sriram Neelakandan]

Created attachment 20621 [details]
Updated patch against r31990 - fixed missing files

Fixed my previous patch, that had a few missing files.
This one should compile fine

[Adam Williamson]

This version works (great!), but breaks the Qt build. The 31326 patch had the same problem. I'm attaching a slightly modified version of sri's patch which fixes that issue.

Tested with this patch, Midori and Epiphany-webkit can both do Youtube fine. I had to remove my Java plugin though (I use the IcedTea one), as I was getting this error:

[adamw@lenovo SPECS]$ epiphany 
Init plugins called 
Module Load Failed :/usr/lib/mozilla/plugins/libjavaplugin.so, Error:libxpcom.so: cannot open shared object file: No such file or directory

which caused the browser to crash (same in Midori). Not sure if this is a problem with the patch, the plugin, or just down to packaging on our (Mandriva's) side. With the Java plugin removed, Flash seems to work fine.

[Adam Williamson]

Created attachment 20639 [details]
fixes sri's latest patch: this one doesn't break the Qt build

[Adam Williamson]

yeah, with either Sun or IcedTea Java plugin, Epiphany with this patch crashes on startup for me, and Midori crashes as soon as I go to any site which would require a plugin to be loaded. Anyone else seeing this?

With Sun Java I don't get the libxpcom.so error, but the browser still crashes.

[Mike Hommey]

is -lnspr4 really needed in GNUmakefile.am ?

[Alp Toker]

I landed the xtbin bundled sources in r32287. I hope this makes the plugin patch smaller and easier to review.

[Alp Toker]

The changes to .pro files should be dropped from this patch as well as the already-landed xtbin files.

[Sriram Neelakandan]

XtBin changes landed - 
http://trac.webkit.org/projects/webkit/changeset/32287
FileSystemGtk landed -
http://trac.webkit.org/projects/webkit/changeset/32288

[Shen Chuan-Hsing]

(In reply to comment #59)
> Created an attachment (id=20639) [edit]
> fixes sri's latest patch: this one doesn't break the Qt build
> 
PluginView::init()
{
    ....
    if (m_isWindowed) {
#if defined(GDK_WINDOWING_X11)
        NPSetWindowCallbackStruct ws;
        ....
        m_npWindow.ws_info = &ws;
#endif
    }
    ....
    if (!(m_plugin->quirks().contains(PluginQuirkDeferFirstSetWindowCall)))
        setNPWindowRect(frameGeometry());
}

void PluginView::setNPWindowRect(const IntRect& rect)
{
    // ((NPSetWindowCallbackStruct*)m_npWindow.ws_info)->display becomes null and crash.
}

[marcoil]

Created attachment 20769 [details]
Adds NPAPI plugin support for Gtk and Qt-x11.

This patch adds NPAPI plugin support for both Gtk (XEmbed, Xt and some support for window-less plugins at the moment) and Qt-x11 (only XEmbed plugins at the moment). It's based on Rodney Dawes' previous work.

It also moves some code from win to platform-independent as it's used in all plugin support code.

I have tested this patch with both the diamondx plugin (works on both platforms) and Adobe Flash Player, which works but has problems with scrolling on Gtk.  Some other plugins don't load because they depend on XPCOM. If anyone knows of an Xt plugin I could test with, please send it my way.

[Adam Williamson]

for the record, marcoil's patch is against rev 32430. I will test it shortly...

[Gwenole Beauchesne]

Hi, you can try Acrobat Reader 5 or Flash 7 plugins which use Xt IIRC.

[Simon Hausmann]

First of all, great job!!

I have a few small comments:

+#elif PLATFORM(QT) && PLATFORM(UNIX)
+#define ENABLE_NETSCAPE_PLUGIN_API 1

In order to not break the Qt/Mac and the Qt/Embedded build I suggest to make this even stricter by perhaps simply using

#elif PLATFORM(QT) && defined(Q_WS_X11)


In PluginPackageQt.cpp, PluginPackage::load() if loading the module fails I think the QLibrary should be deleted and m_module be set back to a null pointer. Also it seems that unloadModule() in FileSystemQt.cpp does not delete the QLibrary itself, it just calls unload(), which it seems the caller in PluginPackage::freeLibraryTimerFired() expects the function to free any allocated resources.

In PluginViewQt.cpp in the PluginView destructor:
+    if (m_window)
+        delete m_window;

I think the if() is safe to remove :)

Other than that I looks good to me on the Qt side.

[Alp Toker]

(In reply to comment #66)
> Created an attachment (id=20769) [edit]
> Adds NPAPI plugin support for Gtk and Qt-x11.
> 
> This patch adds NPAPI plugin support for both Gtk (XEmbed, Xt and some support
> for window-less plugins at the moment) and Qt-x11 (only XEmbed plugins at the
> moment). It's based on Rodney Dawes' previous work.
> 
> It also moves some code from win to platform-independent as it's used in all
> plugin support code.
> 
> I have tested this patch with both the diamondx plugin (works on both
> platforms) and Adobe Flash Player, which works but has problems with scrolling
> on Gtk.  Some other plugins don't load because they depend on XPCOM. If anyone
> knows of an Xt plugin I could test with, please send it my way.
> 


Looks OK on the GTK+ side.

What's -lnspr4 there for? We can't start introducing a dependency on packages and libraries from another browser.

Once the Qt issues are addressed I can look into landing this.

[marcoil]

(In reply to comment #69)
> First of all, great job!!
> 
Thanks!

> +#elif PLATFORM(QT) && PLATFORM(UNIX)
> +#define ENABLE_NETSCAPE_PLUGIN_API 1
> 
> In order to not break the Qt/Mac and the Qt/Embedded build I suggest to make
> this even stricter by perhaps simply using
> 
> #elif PLATFORM(QT) && defined(Q_WS_X11)
> 
That's what I did in the first place, but it doesn't work because at that time no Qt header has been included yet, so Q_WS_X11 is not defined. Should I define ENABLE_NETSCAPE_PLUGIN_API in WebCore.pro instead?

I'll make a new patch for the other comments.

[Adam Williamson]

I can confirm that the new patch from marcoil builds clean against the latest nightly (32416). It also basically works, and having a Java plugin installed no longer causes the browser to crash.

However, it seems basically flaky. Java plugin still doesn't actually work (though this may be known). Sometimes playing video at Youtube works, but quite often, Midori just crashes. I can also get a reproducible crash simply by running Acid3 ( http://acid3.acidtests.org/ )with the patch applied: it crashes around 76/100. Without the patch, the test completes fine.

Have not yet tested with Ephy, I need to rebuild it.

[Adam Williamson]

Epiphany results are basically identical to Midori. Also flaky with Flash, nothing with Java, crashes running Acid3.

[Shen Chuan-Hsing]

WebCore/plugins/gtk/PluginViewGtk.cpp

	NPSetWindowCallbackStruct ws;

	ws.type = 0;

	if (m_needsXEmbed) {
	    ws.display = GDK_WINDOW_XDISPLAY(m_window->window);
	    ws.visual = GDK_VISUAL_XVISUAL(gdk_drawable_get_visual(GDK_DRAWABLE(m_window->window)));
	    ws.depth = gdk_drawable_get_visual(GDK_DRAWABLE(m_window->window))->depth;
	    ws.colormap = GDK_COLORMAP_XCOLORMAP(gdk_drawable_get_colormap(GDK_DRAWABLE(m_window->window)));
	} else {
	    ws.display = GTK_XTBIN(m_window)->xtdisplay;
	    ws.visual = GTK_XTBIN(m_window)->xtclient.xtvisual;
	    ws.depth = GTK_XTBIN(m_window)->xtclient.xtdepth;
	    ws.colormap = GTK_XTBIN(m_window)->xtclient.xtcolormap;

	    XFlush (ws.display);
	}

	m_npWindow.ws_info = &ws;

should be:

	NPSetWindowCallbackStruct *ws = new NPSetWindowCallbackStruct();

	ws->type = 0;

	if (m_needsXEmbed) {
	    ws->display = GDK_WINDOW_XDISPLAY(m_window->window);
	    ws->visual = GDK_VISUAL_XVISUAL(gdk_drawable_get_visual(GDK_DRAWABLE(m_window->window)));
	    ws->depth = gdk_drawable_get_visual(GDK_DRAWABLE(m_window->window))->depth;
	    ws->colormap = GDK_COLORMAP_XCOLORMAP(gdk_drawable_get_colormap(GDK_DRAWABLE(m_window->window)));
	} else {
	    ws->display = GTK_XTBIN(m_window)->xtdisplay;
	    ws->visual = GTK_XTBIN(m_window)->xtclient.xtvisual;
	    ws->depth = GTK_XTBIN(m_window)->xtclient.xtdepth;
	    ws->colormap = GTK_XTBIN(m_window)->xtclient.xtcolormap;

	    XFlush (ws->display);
	}

	m_npWindow.ws_info = ws;

[Sriram Neelakandan]

> 
>         NPSetWindowCallbackStruct ws;
> 
> should be:
> 
>         NPSetWindowCallbackStruct *ws = new NPSetWindowCallbackStruct();


I agree we cannot alloc on stack and store in a class member.. 
but the new struct is a leak per plugin

so we need this also :

PluginView::stop()
{

...
..

    if (m_isWindowed && m_npWindow.ws_info) 
           delete m_npWindow.ws_info
}

[marcoil]

Created attachment 20794 [details]
Adds NPAPI plugin support for Gtk and Qt-x11, r32489

This new patch addresses comments made here:
- ENABLE_NETSCAPE_PLUGIN_API on Qt is defined in WebCore.pro, as it was difficult to do it in Platform.h.
- QtLibraries are deleted if correctly unloaded.
- I removed the dependency on libnspr4, I don't know where that came from.

Right now (on my setup), testing with GtkLauncher and QtLauncher:

- Acid3 gets to 94/100 on both platforms without crashing, although it gets "linktest failed" on Qt, don't know if this is due to the patch.
- Flash semi-works in a very flacky way. It has problems with scrolling on Gtk and YouTube videos stutter on Qt (other videos sites work, though). XEmbed implementations are different and I've sent a patch for Qt that solves this a little.
- Java doesn't load and I'm currently investigating why.
- Some other plugins don't load because they depend on XPCOM, i.e., totem video plugins. Others don't work on Qt because of Gtk dependencies, like Gnash.
- Adobe Reader doesn't load because it needs display info before any PluginView has been created, which will be difficult to solve right now.

[Adam Williamson]

will incorporate the patch into our build and do my own tests later. Thanks. I thought current Webkit was supposed to hit 100/100 on acid3?

[marcoil]

(In reply to comment #77)
> will incorporate the patch into our build and do my own tests later. Thanks. I
> thought current Webkit was supposed to hit 100/100 on acid3?
> 

Now that I've tested it, I only get 94/100 and the same "linktest failed" with QtLauncher without the patch.

[marcoil]

(In reply to comment #76)
> - Java doesn't load and I'm currently investigating why.

The Java plugin does not work because it depends on OJI, an older interface and not on NPAPI, and this will not be fixed until JRE's next version. It will possibly need extensions to NPAPI, so it should take a while.

[Mike Hommey]

> Now that I've tested it, I only get 94/100 and the same "linktest failed" with
> QtLauncher without the patch.

You probably forgot to enable svg support

[Adam Williamson]

Results with the new marcoil patch:

acid3 doesn't cause a crash any more. test completes with a score of 98/100 (I'll track down that missing 2 elsewhere, likely nothing to do with the patch).

as he noted, scrolling is troublesome. Basically, if you go to Youtube and start playing a video, it'll be fine. If you then scroll the page, the video will freeze on the frame that was being displayed when you scrolled. Audio keeps playing. Thereafter, the video only updates each time you scroll the page: each time you scroll the page the video will update to the 'current' frame, then freeze again.

[Adam Williamson]

Forgot to mention: with the patch, if you just run a browser, go to a 'normal' site and then quit, all is fine. If you run a browser, go to Youtube (probably any other site that causes Flash plugin to be loaded, not tested though) and then quit the browser, it segfaults. Midori just segfaults, Epiphany brings up the GNOME crash window.

[marcoil]

(In reply to comment #82)
> Forgot to mention: with the patch, if you just run a browser, go to a 'normal'
> site and then quit, all is fine. If you run a browser, go to Youtube (probably
> any other site that causes Flash plugin to be loaded, not tested though) and
> then quit the browser, it segfaults. Midori just segfaults, Epiphany brings up
> the GNOME crash window.
> 

I've tried it on other sites and it only happens to me on YouTube, curiously.

The scrolling bug appeared recently, as I remember it not happening with the original patch.

[Sylvain Petreolle]

(In reply to comment #83)
> 
> I've tried it on other sites and it only happens to me on YouTube, curiously.
> 
> The scrolling bug appeared recently, as I remember it not happening with the
> original patch.
> 
Works beautifully here, nice work.

The crash is reproducible on prizee.com,
and you can trigger it too when closing the site's tab.

[Andrew Badr]

I'm running WebKit headless (under Xvfb) and experiencing a segfault when I try to load a page with Flash using the latest version of this patch (against r32489). Pages without Flash work fine, but a Youtube video for example will crash right away. 

I'm using the python bindings (pywebkitgtk). Here's the program that segfaults:
import webkit, gtk
v = webkit.WebView()
v.open("http://www.youtube.com/watch?v=Yz7FFlFy8eM")
gtk.main()

Using another URL, such as http://www.google.com/, works fine (as youtube did before I installed flash), though there is this warning:  GtkWarning: gdk_window_invalidate_rect: assertion `window != NULL' failed
(this warning was also present before I patched & installed flash)

My uninformed guess is that something is trying to draw to the null window. That code could well be in the Flash plugin. Even if it's in WebKit, I don't expect that this use case is a high priority, but I thought to let you all know anyway.

[Gwenole Beauchesne]

Created attachment 20887 [details]
64-bit fixes

Re-add 64-bit fixes that got lost in recent patches.

[Gwenole Beauchesne]

Created attachment 20888 [details]
Specialize NPN_GetValue()

NPN_GetValue() should be valid for NULL instance and NPNVToolkit or NPNVSupportsXEmbedBool variables. This is the case for Gnash, nspluginwrapper and probably Acrobat Reader (not tested but the comment in npapi.cpp might indicate that).

[Gwenole Beauchesne]

Sorry, individual patches only without respecting proper rules because I am building from the WebKit/Clutter git branch.

[marcoil]

Created attachment 20901 [details]
Adds NPAPI plugin support for Gtk and Qt-x11, r32726

This new, updated and improved patch gets youtube videos working without crashes on my setup, on both GtkLauncher and QtLauncher.

(In reply to comment #86)
> Re-add 64-bit fixes that got lost in recent patches.
> 

I have incorporated this fix, but not the one separating NPN_GetValue as I'd like to tackle this issue separately. Thanks for the tip!

[Adam Williamson]

With the updated marcoil patch, Youtube scrolling and segfault on exit bugs are both solved. Haven't tested Gwenole's other suggested change yet.

[Alp Toker]

Landed in r32766.

I had to make a few hundred lines of coding style fixes. Please check the commit to make sure everything is in order.

This is going to need some stabilisation work now, still crashes from time to time.

We'll probably want it turned off by default in the GTK+ port with a configurable runtime option to enable it, or we're going to be innundated with bug reports.

[Maciej Stachowiak]

Comment on attachment 20621 [details]
Updated patch against r31990 - fixed missing files

Obsoleting this patch to take the bug out of the review queue.

[Gwenole Beauchesne]

Hi, I have not generated a patch yet, but I got by this one while working on the Clutter version. In WebCore/plugins/gtk/PluginDatabaseGtk.cpp (PluginDatabase::isPreferredPluginDirectory), please use NULL not 0 as the varargs terminator.

[Adam Williamson]

Gwenole, now the support is merged, I think it's better to open new bug reports for issues in the merged code rather than keep adding to this - they'll likely get addressed faster that way.

[Gwenole Beauchesne]

This bug should not have been closed in the first place as per comment #89, the NPN_GetValue() regression is not fixed yet. Anyhow, I will open a new bug for this one too. The g_build_filename() thing is in bug #18906.

[marcoil]

(In reply to comment #95)
> This bug should not have been closed in the first place as per comment #89, the
> NPN_GetValue() regression is not fixed yet. Anyhow, I will open a new bug for
> this one too. The g_build_filename() thing is in bug #18906.
> 

I opened a new bug for comment #89, bug #18892