Bug 14750 - [gtk] Implement plugin support in GTK backend
Summary: [gtk] Implement plugin support in GTK backend
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 523.x (Safari 3)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2007-07-24 10:27 PDT by Bastien Nocera
Modified: 2008-05-06 04:28 PDT (History)
21 users (show)

See Also:


Attachments
Start at plug-ins patch (123.46 KB, patch)
2007-11-02 14:41 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Update plug-ins patch (171.25 KB, patch)
2007-11-13 13:40 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch for the Shared type removal (171.31 KB, patch)
2007-11-14 10:55 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to handle XEmbed and Xt plug-ins both (172.63 KB, patch)
2007-11-15 09:45 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated to build against trunk (177.53 KB, patch)
2007-12-11 10:44 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch for KURL class changes and without other patches included (165.66 KB, patch)
2007-12-12 14:01 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated Giant Patch With Everything (177.33 KB, patch)
2007-12-13 13:56 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated giant patch for trunk (174.48 KB, patch)
2007-12-17 07:58 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch for trunk today (174.02 KB, patch)
2007-12-18 09:16 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
NPAPI scriptability test (64.23 KB, application/x-tar)
2007-12-18 09:26 PST, Kai Hendry
no flags Details
Update patch against r28911 (166.93 KB, patch)
2007-12-20 15:07 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated plug-ins patch (166.90 KB, patch)
2007-12-24 13:02 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to fix scrolling too (165.94 KB, patch)
2007-12-24 14:46 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Update to fix a Gtk-CRITICAL when destroying the widget (165.96 KB, patch)
2007-12-25 09:16 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to build with autotools also, against r29084 (167.96 KB, patch)
2008-01-02 09:13 PST, Rodney Dawes
mrowe: review-
Details | Formatted Diff | Diff
Updated patch against r29305 (167.97 KB, patch)
2008-01-08 11:55 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29338 (163.99 KB, patch)
2008-01-09 16:14 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29531 (1.00 KB, patch)
2008-01-16 09:43 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29725 (134.55 KB, patch)
2008-01-22 15:48 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29742 (134.35 KB, patch)
2008-01-23 12:42 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29792 (134.03 KB, patch)
2008-01-25 09:24 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29914 (133.10 KB, patch)
2008-02-01 10:10 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r29975 (136.53 KB, patch)
2008-02-04 14:05 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r30005 (133.83 KB, patch)
2008-02-05 10:51 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
64-bit fixes (2.12 KB, patch)
2008-02-25 03:51 PST, Gwenole Beauchesne
no flags Details | Formatted Diff | Diff
Updated patch against r31217 (93.30 KB, patch)
2008-03-21 13:58 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch (94.44 KB, patch)
2008-03-24 11:59 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch against r31326 (95.37 KB, patch)
2008-03-26 13:48 PDT, Rodney Dawes
no flags Details | Formatted Diff | Diff
alternative updated version of patch (against 31893, despite the filename) (93.22 KB, patch)
2008-04-16 15:15 PDT, Adam Williamson
no flags Details | Formatted Diff | Diff
patch for Plugin updated to r31990 (25.58 KB, patch)
2008-04-17 01:22 PDT, Sriram Neelakandan
no flags Details | Formatted Diff | Diff
Updated patch against r31990 - fixed missing files (94.89 KB, patch)
2008-04-17 06:00 PDT, Sriram Neelakandan
no flags Details | Formatted Diff | Diff
fixes sri's latest patch: this one doesn't break the Qt build (95.79 KB, patch)
2008-04-17 14:48 PDT, Adam Williamson
no flags Details | Formatted Diff | Diff
Adds NPAPI plugin support for Gtk and Qt-x11. (123.05 KB, patch)
2008-04-23 03:28 PDT, marcoil
no flags Details | Formatted Diff | Diff
Adds NPAPI plugin support for Gtk and Qt-x11, r32489 (123.88 KB, patch)
2008-04-24 09:01 PDT, marcoil
no flags Details | Formatted Diff | Diff
64-bit fixes (1.13 KB, patch)
2008-04-29 04:42 PDT, Gwenole Beauchesne
no flags Details | Formatted Diff | Diff
Specialize NPN_GetValue() (2.32 KB, patch)
2008-04-29 04:45 PDT, Gwenole Beauchesne
no flags Details | Formatted Diff | Diff
Adds NPAPI plugin support for Gtk and Qt-x11, r32726 (127.02 KB, patch)
2008-04-30 04:28 PDT, marcoil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Bastien Nocera 2007-07-24 10:27:35 PDT
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.
Comment 1 Kai Hendry 2007-10-31 08:02:43 PDT
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.
Comment 2 Jean-Charles Verdié 2007-10-31 08:15:46 PDT
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)
Comment 3 Rodney Dawes 2007-10-31 14:07:25 PDT
(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?

Comment 4 Rodney Dawes 2007-11-02 14:41:22 PDT
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.
Comment 5 Mark Rowe (bdash) 2007-11-05 13:22:58 PST
Alp helped Rodney work out the issue with the widget sizing/positioning, and I think he's back underway on his plugin patch.
Comment 6 Rodney Dawes 2007-11-13 13:40:15 PST
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. :)
Comment 7 Rodney Dawes 2007-11-14 10:55:35 PST
Created attachment 17273 [details]
Updated patch for the Shared type removal
Comment 8 Nigel Tao 2007-11-14 15:24:34 PST
Just with the patch, please replace tabs by spaces.  http://webkit.org/coding/coding-style.html
Comment 9 Rodney Dawes 2007-11-15 09:45:41 PST
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.
Comment 10 Kai Hendry 2007-11-27 15:25:10 PST
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.
Comment 11 Rodney Dawes 2007-12-11 10:44:03 PST
Created attachment 17851 [details]
Updated to build against trunk
Comment 12 Rodney Dawes 2007-12-12 14:01:35 PST
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.
Comment 13 Rodney Dawes 2007-12-12 14:03:36 PST
I can't change the DependsOn field it seems, so you will need the patches from bugs #16389 #15669 and #16071 as well.
Comment 14 Rodney Dawes 2007-12-13 13:56:33 PST
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.
Comment 15 Rodney Dawes 2007-12-17 07:58:30 PST
Created attachment 17963 [details]
Updated giant patch for trunk
Comment 16 Rodney Dawes 2007-12-18 09:16:21 PST
Created attachment 17973 [details]
Updated patch for trunk today
Comment 17 Kai Hendry 2007-12-18 09:26:32 PST
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.
Comment 18 Rodney Dawes 2007-12-20 15:07:15 PST
Created attachment 18017 [details]
Update patch against r28911
Comment 19 Rodney Dawes 2007-12-24 13:02:20 PST
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.
Comment 20 Rodney Dawes 2007-12-24 14:46:24 PST
Created attachment 18096 [details]
Updated patch to fix scrolling too
Comment 21 Rodney Dawes 2007-12-25 09:16:35 PST
Created attachment 18108 [details]
Update to fix a Gtk-CRITICAL when destroying the widget
Comment 22 Rodney Dawes 2008-01-02 09:13:48 PST
Created attachment 18239 [details]
Updated patch to build with autotools also, against r29084
Comment 23 Mark Rowe (bdash) 2008-01-02 09:45:26 PST
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.
Comment 24 Rodney Dawes 2008-01-08 11:55:38 PST
Created attachment 18331 [details]
Updated patch against r29305
Comment 25 Rodney Dawes 2008-01-09 16:14:52 PST
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.
Comment 26 Michael Emmel 2008-01-09 18:08:06 PST
(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.

Comment 27 Rodney Dawes 2008-01-09 18:45:51 PST
(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.

Comment 28 Michael Emmel 2008-01-09 19:15:25 PST
(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.


Comment 29 Rodney Dawes 2008-01-16 09:43:55 PST
Created attachment 18473 [details]
Updated patch against r29531
Comment 30 Rodney Dawes 2008-01-22 15:48:00 PST
Created attachment 18606 [details]
Updated patch against r29725
Comment 31 Alp Toker 2008-01-23 04:03:04 PST
(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?
Comment 32 Rodney Dawes 2008-01-23 12:42:57 PST
Created attachment 18623 [details]
Updated patch against r29742
Comment 33 Rodney Dawes 2008-01-25 09:24:02 PST
Created attachment 18691 [details]
Updated patch against r29792

This is an updated patch to deal with the new PluginQuirkSet class introduced in recent builds.
Comment 34 Rodney Dawes 2008-01-25 09:55:23 PST
> Could you add a comment about gtk2xtbin.c and any other imported sources in the
> ChangeLog entry?

That's the plan, yes.
Comment 35 Rodney Dawes 2008-02-01 10:10:47 PST
Created attachment 18849 [details]
Updated patch against r29914
Comment 36 Alp Toker 2008-02-03 23:39:03 PST
(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.
Comment 37 Rodney Dawes 2008-02-04 09:02:14 PST
(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).
Comment 38 Rodney Dawes 2008-02-04 14:05:14 PST
Created attachment 18918 [details]
Updated patch against r29975
Comment 39 Rodney Dawes 2008-02-05 10:51:02 PST
Created attachment 18936 [details]
Updated patch against r30005
Comment 40 Gwenole Beauchesne 2008-02-25 03:51:44 PST
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.
Comment 41 Rodney Dawes 2008-03-21 13:58:48 PDT
Created attachment 19936 [details]
Updated patch against r31217

Here's an updated patch against r31217 which works with the refactored plugin api code.
Comment 42 Jasper Bryant-Greene 2008-03-24 05:47:19 PDT
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?
Comment 43 Mark Rowe (bdash) 2008-03-24 09:54:29 PDT
(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.
Comment 44 Rodney Dawes 2008-03-24 11:59:04 PDT
Created attachment 20011 [details]
Updated patch
Comment 45 Jasper Bryant-Greene 2008-03-24 13:12:20 PDT
(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.
Comment 46 Mike Hommey 2008-03-24 23:10:59 PDT
(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.
Comment 47 Jasper Bryant-Greene 2008-03-24 23:14:45 PDT
(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.
Comment 48 Mike Hommey 2008-03-24 23:52:02 PDT
(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 ?)
Comment 49 Rodney Dawes 2008-03-26 13:48:04 PDT
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 .
Comment 50 Adam Williamson 2008-04-16 15:13:18 PDT
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.
Comment 51 Adam Williamson 2008-04-16 15:15:30 PDT
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.
Comment 52 Sriram Neelakandan 2008-04-17 01:22:17 PDT
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.
Comment 53 Sriram Neelakandan 2008-04-17 01:27:03 PDT
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
Comment 54 Adam Williamson 2008-04-17 03:50:34 PDT
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.
Comment 55 Adam Williamson 2008-04-17 03:59:16 PDT
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?
Comment 56 Sriram Neelakandan 2008-04-17 05:51:57 PDT
(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 

Comment 57 Sriram Neelakandan 2008-04-17 06:00:19 PDT
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
Comment 58 Adam Williamson 2008-04-17 14:46:43 PDT
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.
Comment 59 Adam Williamson 2008-04-17 14:48:41 PDT
Created attachment 20639 [details]
fixes sri's latest patch: this one doesn't break the Qt build
Comment 60 Adam Williamson 2008-04-17 14:50:35 PDT
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.
Comment 61 Mike Hommey 2008-04-18 08:54:44 PDT
is -lnspr4 really needed in GNUmakefile.am ?
Comment 62 Alp Toker 2008-04-20 23:15:23 PDT
I landed the xtbin bundled sources in r32287. I hope this makes the plugin patch smaller and easier to review.
Comment 63 Alp Toker 2008-04-20 23:28:05 PDT
The changes to .pro files should be dropped from this patch as well as the already-landed xtbin files.
Comment 64 Sriram Neelakandan 2008-04-21 22:29:15 PDT
XtBin changes landed - 
http://trac.webkit.org/projects/webkit/changeset/32287
FileSystemGtk landed -
http://trac.webkit.org/projects/webkit/changeset/32288
Comment 65 Shen Chuan-Hsing 2008-04-22 17:41:37 PDT
(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.
}
Comment 66 marcoil 2008-04-23 03:28:37 PDT
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.
Comment 67 Adam Williamson 2008-04-23 05:35:27 PDT
for the record, marcoil's patch is against rev 32430. I will test it shortly...
Comment 68 Gwenole Beauchesne 2008-04-23 06:08:27 PDT
Hi, you can try Acrobat Reader 5 or Flash 7 plugins which use Xt IIRC.
Comment 69 Simon Hausmann 2008-04-23 06:41:33 PDT
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.
Comment 70 Alp Toker 2008-04-23 06:48:47 PDT
(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.
Comment 71 marcoil 2008-04-23 08:44:56 PDT
(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.
Comment 72 Adam Williamson 2008-04-23 09:55:42 PDT
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.
Comment 73 Adam Williamson 2008-04-23 15:58:42 PDT
Epiphany results are basically identical to Midori. Also flaky with Flash, nothing with Java, crashes running Acid3.
Comment 74 Shen Chuan-Hsing 2008-04-23 18:55:15 PDT
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;
Comment 75 Sriram Neelakandan 2008-04-23 22:57:54 PDT
> 
>         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
}

Comment 76 marcoil 2008-04-24 09:01:42 PDT
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.
Comment 77 Adam Williamson 2008-04-24 09:04:31 PDT
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?
Comment 78 marcoil 2008-04-24 09:56:57 PDT
(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.
Comment 79 marcoil 2008-04-24 11:09:13 PDT
(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.
Comment 80 Mike Hommey 2008-04-24 11:44:41 PDT
> 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
Comment 81 Adam Williamson 2008-04-25 05:15:59 PDT
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.
Comment 82 Adam Williamson 2008-04-25 05:17:59 PDT
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.
Comment 83 marcoil 2008-04-25 05:25:06 PDT
(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.
Comment 84 Sylvain Petreolle 2008-04-25 11:01:36 PDT
(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.
Comment 85 Andrew Badr 2008-04-25 11:38:41 PDT
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.
Comment 86 Gwenole Beauchesne 2008-04-29 04:42:23 PDT
Created attachment 20887 [details]
64-bit fixes

Re-add 64-bit fixes that got lost in recent patches.
Comment 87 Gwenole Beauchesne 2008-04-29 04:45:18 PDT
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).
Comment 88 Gwenole Beauchesne 2008-04-29 04:47:03 PDT
Sorry, individual patches only without respecting proper rules because I am building from the WebKit/Clutter git branch.
Comment 89 marcoil 2008-04-30 04:28:36 PDT
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!
Comment 90 Adam Williamson 2008-05-01 11:14:04 PDT
With the updated marcoil patch, Youtube scrolling and segfault on exit bugs are both solved. Haven't tested Gwenole's other suggested change yet.
Comment 91 Alp Toker 2008-05-01 11:21:48 PDT
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.
Comment 92 Maciej Stachowiak 2008-05-02 04:04:27 PDT
Comment on attachment 20621 [details]
Updated patch against r31990 - fixed missing files

Obsoleting this patch to take the bug out of the review queue.
Comment 93 Gwenole Beauchesne 2008-05-05 00:38:08 PDT
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.
Comment 94 Adam Williamson 2008-05-05 07:18:25 PDT
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.
Comment 95 Gwenole Beauchesne 2008-05-06 04:23:00 PDT
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.
Comment 96 marcoil 2008-05-06 04:28:36 PDT
(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