[Qt][WK2] Plugin initialization

• Component: Plug-ins
• Status: RESOLVED FIXED

[Balazs Kelemen]

We need to implement plugin initialization on UNIX (non-mac) and furthermore
we need to apply the Qt specific quirks (especially for flash) as well as we do in WebCore.

[Balazs Kelemen]

Created attachment 71764 [details]
proposed patch

[Balazs Kelemen]

*** Bug 46262 has been marked as a duplicate of this bug. ***

[Early Warning System Bot]

Attachment 71764 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4623086

[Anders Carlsson]

Do we really need a NetscapePluginQt?

I was talking to Adam Roben about this and maybe we could have a special PLUGIN_PLATFORM or PLUGIN_ARCHITECTURE macro that could have one of three values: X11, MAC or WINDOWS. That way Qt could use the right plug-in architecture based on which platform it's being compiled on.

If we need any platform specific code we should try to put it below the plug-in implementation itself. Adam, do you agree?

[Adam Roben (:aroben)]

(In reply to comment #4)
> Do we really need a NetscapePluginQt?
> 
> I was talking to Adam Roben about this and maybe we could have a special PLUGIN_PLATFORM or PLUGIN_ARCHITECTURE macro that could have one of three values: X11, MAC or WINDOWS. That way Qt could use the right plug-in architecture based on which platform it's being compiled on.

That sounds good to me.

> If we need any platform specific code we should try to put it below the plug-in implementation itself. Adam, do you agree?

Obviously some platform-specific code will be in the plug-in implementation (e.g., NetscapePluginMac and NetscapePluginWin).

[Eric Seidel (no email)]

Attachment 71764 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4715089

[Kenneth Rohde Christiansen]

Sounds OK with me. I'm adding Girish as he is the guy who have been working on the Qt Linux related plugin code lately, and knows about the GTK+ implementation as well.

[Anders Carlsson]

(In reply to comment #7)
> Sounds OK with me. I'm adding Girish as he is the guy who have been working on the Qt Linux related plugin code lately, and knows about the GTK+ implementation as well.

Great! I'd really like the WebKit2 plug-in code to stay sane and not become a mess of #ifdefs and different codepaths like the one in WebCore :)

[Balazs Kelemen]

(In reply to comment #8)
> (In reply to comment #7)
> > Sounds OK with me. I'm adding Girish as he is the guy who have been working on the Qt Linux related plugin code lately, and knows about the GTK+ implementation as well.
> 
> Great! I'd really like the WebKit2 plug-in code to stay sane and not become a mess of #ifdefs and different codepaths like the one in WebCore :)

Me too. I tried to avoid ifdefs in common code and code duplication as well.
What do you mean about "the plugin implementation itself"? As I see
NetscapePlugin is the plugin implementation. The Qt specific code I have added
is for qt-linux-x11. On Windows and Mac Qt should reuse the existing
implementation as it does in WebCore. Please taking into account that event
handling and painting is port specific everywhere in WebKit so I think we cannot
omit NetscapePluginQt.

[Balazs Kelemen]

Created attachment 71853 [details]
build fixed patch

Same as the previous one with build fix.

[Eric Seidel (no email)]

Attachment 71853 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4748016

[Balazs Kelemen]

Created attachment 71897 [details]
fixed Mac build

[Anders Carlsson]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Sounds OK with me. I'm adding Girish as he is the guy who have been working on the Qt Linux related plugin code lately, and knows about the GTK+ implementation as well.
> > 
> > Great! I'd really like the WebKit2 plug-in code to stay sane and not become a mess of #ifdefs and different codepaths like the one in WebCore :)
> 
> Me too. I tried to avoid ifdefs in common code and code duplication as well.
> What do you mean about "the plugin implementation itself"? As I see
> NetscapePlugin is the plugin implementation. The Qt specific code I have added
> is for qt-linux-x11. On Windows and Mac Qt should reuse the existing
> implementation as it does in WebCore. Please taking into account that event
> handling and painting is port specific everywhere in WebKit so I think we cannot
> omit NetscapePluginQt.

Could we have a NetscapePluginX11 instead of a NetscapePluginQt?

[Balazs Kelemen]

> Could we have a NetscapePluginX11 instead of a NetscapePluginQt?

I hardly think that for example Qt and GTK can use the same
implementation even on X11. My argument is again the fact that painting and event handling is always port specific and not "just" platform
specific.

[WebKit Review Bot]

Attachment 71897 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4807035

[Balazs Kelemen]

Created attachment 72084 [details]
GTK build fix

[Kenneth Rohde Christiansen]

Comment on attachment 72084 [details]
GTK build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=72084&action=review

> WebCore/plugins/unix/UNIXPluginQuirks.cpp:139
> +#if defined(Q_WS_MAEMO_5) || defined(Q_WS_MAEMO_6)
> +    // Passing a 32-bit depth pixmap to NPAPI plugins is too inefficient. Instead, pass a X Pixmap
> +    // that has same depth as the screen depth since graphics operations are optimized
> +    // for this depth.
> +    quirks->add(PluginQuirkRequiresDefaultScreenDepth);
> +#endif

This seems qt specific and it is not inside #if PLATFORM(QT). Maybe that would make it more clean that it is a Qt workaround?

> WebCore/plugins/unix/UNIXPluginQuirks.h:51
> +class UNIXPluginQuirks {

Remember Mac is UNIX as well.

> WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:86
> +void NetscapePlugin::platformPreInitialize(Parameters*)
> +{
> +}

No comment to why this is empty?

[Kenneth Rohde Christiansen]

(In reply to comment #14)
> > Could we have a NetscapePluginX11 instead of a NetscapePluginQt?
> 
> I hardly think that for example Qt and GTK can use the same
> implementation even on X11. My argument is again the fact that painting and event handling is always port specific and not "just" platform
> specific.

But well, Qt uses the Windows impl on Windows and the mac impl on mac, so why not? but maybe it will become an ifdef hell.

[Kenneth Rohde Christiansen]

Are these not X11 quirks? That would be a better name than UNIX.

[Kenneth Rohde Christiansen]

Comment on attachment 72084 [details]
GTK build fix

View in context: https://bugs.webkit.org/attachment.cgi?id=72084&action=review

> WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp:404
> +        loadURL("GET", effectivePararmeters.url.string(), String(), HTTPHeaderMap(), Vector<uint8_t>(), false, 0);

Spelling mistake here. It is parameters and not para_R_meters

[Anders Carlsson]

I don't think it will become an ifdef hell - we'll have something like

#if PLATFORM(MAC)
#define PLUGIN_ARCHITECTURE_MAC 1
#elif PLATFORM(WIN)
#define PLUGIN_ARCHITECTURE_WIN 1
#elif PLATFORM(QT)
#if (qt for mac)
#define PLUGIN_ARCHITECTURE_MAC 1
#elif (qt for x11)
#define PLUGIN_ARCHITECTURE_X11
#endif
#endif

I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.

[Kenneth Rohde Christiansen]

(In reply to comment #21)
> I don't think it will become an ifdef hell - we'll have something like
> 
> #if PLATFORM(MAC)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif PLATFORM(WIN)
> #define PLUGIN_ARCHITECTURE_WIN 1
> #elif PLATFORM(QT)
> #if (qt for mac)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif (qt for x11)
> #define PLUGIN_ARCHITECTURE_X11
> #endif
> #endif
> 
> I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.

Yes, this is a good suggestion. I also agree that the patch needs to be split up.

[Balazs Kelemen]

(In reply to comment #17)
> (From update of attachment 72084 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72084&action=review
> 
> > WebCore/plugins/unix/UNIXPluginQuirks.cpp:139
> > +#if defined(Q_WS_MAEMO_5) || defined(Q_WS_MAEMO_6)
> > +    // Passing a 32-bit depth pixmap to NPAPI plugins is too inefficient. Instead, pass a X Pixmap
> > +    // that has same depth as the screen depth since graphics operations are optimized
> > +    // for this depth.
> > +    quirks->add(PluginQuirkRequiresDefaultScreenDepth);
> > +#endif
> 
> This seems qt specific and it is not inside #if PLATFORM(QT). Maybe that would make it more clean that it is a Qt workaround?

Right.

> 
> > WebCore/plugins/unix/UNIXPluginQuirks.h:51
> > +class UNIXPluginQuirks {
> 
> Remember Mac is UNIX as well.

True.

>
> 
> > WebKit2/WebProcess/Plugins/Netscape/mac/NetscapePluginMac.mm:86
> > +void NetscapePlugin::platformPreInitialize(Parameters*)
> > +{
> > +}
> 
> No comment to why this is empty?

I think this is self documenting. A platform method that does nothing
=> there is nothing to do on that platform at that point :)
I can add a comment however.

[Balazs Kelemen]

(In reply to comment #19)
> Are these not X11 quirks? That would be a better name than UNIX.

Right.

[Balazs Kelemen]

(In reply to comment #21)
> I don't think it will become an ifdef hell - we'll have something like
> 
> #if PLATFORM(MAC)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif PLATFORM(WIN)
> #define PLUGIN_ARCHITECTURE_WIN 1
> #elif PLATFORM(QT)
> #if (qt for mac)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif (qt for x11)
> #define PLUGIN_ARCHITECTURE_X11
> #endif
> #endif
> 
> I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.

What would it solve? The ifdef hell would be in NetscapePluginX11.cpp
with a lot of code guarded by #if PLATFORM(QT), #if PLATFORM(GTK), ...
Unfortunately the plugin implementation is depends on the arch and on
the port as well.

[Balazs Kelemen]

> > I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.
 
True. However, another approach that had came to my mind is to
determining the quirks in the UI process and send more information
about the plugin to the web process than just the plugin path. Do you
think it would be better? This would help to solve the issue in
https://bugs.webkit.org/show_bug.cgi?id=46076 as well.

[Anders Carlsson]

(In reply to comment #25)
> (In reply to comment #21)
> > I don't think it will become an ifdef hell - we'll have something like
> > 
> > #if PLATFORM(MAC)
> > #define PLUGIN_ARCHITECTURE_MAC 1
> > #elif PLATFORM(WIN)
> > #define PLUGIN_ARCHITECTURE_WIN 1
> > #elif PLATFORM(QT)
> > #if (qt for mac)
> > #define PLUGIN_ARCHITECTURE_MAC 1
> > #elif (qt for x11)
> > #define PLUGIN_ARCHITECTURE_X11
> > #endif
> > #endif
> > 
> > I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.
> 
> What would it solve? The ifdef hell would be in NetscapePluginX11.cpp
> with a lot of code guarded by #if PLATFORM(QT), #if PLATFORM(GTK), ...
> Unfortunately the plugin implementation is depends on the arch and on
> the port as well.

What are the things you would guard by Qt and GTK specific ifdefs?

[Anders Carlsson]

(In reply to comment #26)
> > > I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.
> 
> True. However, another approach that had came to my mind is to
> determining the quirks in the UI process and send more information
> about the plugin to the web process than just the plugin path. Do you
> think it would be better? This would help to solve the issue in
> https://bugs.webkit.org/show_bug.cgi?id=46076 as well.

I don't think doing that would buy us anything. 46076 is all about site-specific quirks and not plug-in quirks.

[Tor Arne Vestbø]

(In reply to comment #25)
> (In reply to comment #21)
> > I don't think it will become an ifdef hell - we'll have something like
> > 
> > #if PLATFORM(MAC)
> > #define PLUGIN_ARCHITECTURE_MAC 1
> > #elif PLATFORM(WIN)
> > #define PLUGIN_ARCHITECTURE_WIN 1
> > #elif PLATFORM(QT)
> > #if (qt for mac)
> > #define PLUGIN_ARCHITECTURE_MAC 1
> > #elif (qt for x11)
> > #define PLUGIN_ARCHITECTURE_X11
> > #endif
> > #endif
> > 
> > I also think that this patch should be split up. The quirk refactoring could definitely be done as a separate patch.
> 
> What would it solve? The ifdef hell would be in NetscapePluginX11.cpp
> with a lot of code guarded by #if PLATFORM(QT), #if PLATFORM(GTK), ...
> Unfortunately the plugin implementation is depends on the arch and on
> the port as well.

I agree with Anders here, we should have a NetscapePluginX11.cpp with #if PLATFORM(QT), #if PLATFORM(GTK) guards for the port-specific bits. Even if there will be a few of those guards, it's certainly better than the approach we have in WebCore now with PluginViewQt and PluginViewGtk. 

PluginViewMac in WebCore shows how the Qt and Wx port shares most stuff, and many of those port-specific bits could be put into widget or hidden somewhere closer to where the difference really is.

Regarding the name X11 is preferable, it would clash a bit with XP_UNIX, but I don't mind that. 

I'd also split up this patch as others say, the renaming etc can be done separately from sharing the quirks between WebKit and WebKit2.

[Tor Arne Vestbø]

(In reply to comment #21)
> I don't think it will become an ifdef hell - we'll have something like
> 
> #if PLATFORM(MAC)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif PLATFORM(WIN)
> #define PLUGIN_ARCHITECTURE_WIN 1
> #elif PLATFORM(QT)
> #if (qt for mac)
> #define PLUGIN_ARCHITECTURE_MAC 1
> #elif (qt for x11)
> #define PLUGIN_ARCHITECTURE_X11
> #endif
> #endif

Would it make sense to just use the XP-defines from npapi? Apart from XP_UNIX being really XP_X11 they cover what we want? 

The alternative is to add another layer of abstraction/indirection by going for PLUGIN_ARCHITECTURE_FOO like you say. But then what do we base those on?

a) We have the choices made in npapi.h to enable the XP-defines: XP_OS2, XP_WIN, XP_SYMBIAN, XP_MACOSX (for some reason XP_UNIX is not defined there).

b) We have the OS() macros, which work for MAC_OS_X and WINDOWS, but not for X11 (LINUX would not be correct). 

c) Or we have PLATFORM()-macros.

If we go for the latter two we'll have two different definitions of "plugin architecture", XP_FOO and PLUGIN_ARCHITECTURE_FOO.

I'd prefer to stay with the XP-defines or add another level to make it clearer what they mean but to map them to the XP-defines. 

The downside to this is of course that we have platform-detection code both in npapi.h and Platform.h. Would it make sense to have npapi.h use the macros from Platform.h?

[Antonio Gomes]

> Would it make sense to just use the XP-defines from npapi? Apart from XP_UNIX being really XP_X11 they cover what we want? 

We are wrongly assuming in webcore that unix == x11 for plugins. It is not always true.

[Adam Roben (:aroben)]

(In reply to comment #30)
> I'd prefer to stay with the XP-defines or add another level to make it clearer what they mean but to map them to the XP-defines.

That makes sense to me.
 
> The downside to this is of course that we have platform-detection code both in npapi.h and Platform.h. Would it make sense to have npapi.h use the macros from Platform.h?

npapi.h can't use Platform.h. npapi.h is an API header that plugins include, and plugins don't have access to Platform.h.

[Tor Arne Vestbø]

(In reply to comment #32)
> (In reply to comment #30)
> > I'd prefer to stay with the XP-defines or add another level to make it clearer what they mean but to map them to the XP-defines.
> 
> That makes sense to me.

So we could have:

#define NESCAPE_PLUGIN_PLATFORM(WTF_FEATURE) (defined XP_##WTF_FEATURE && XP_##WTF_FEATURE)

And then either use NESCAPE_PLUGIN_PLATFORM(UNIX) all around, or #define XP_X11 XP_UNIX and use the clearer NESCAPE_PLUGIN_PLATFORM(X11)

> 
> > The downside to this is of course that we have platform-detection code both in npapi.h and Platform.h. Would it make sense to have npapi.h use the macros from Platform.h?
> 
> npapi.h can't use Platform.h. npapi.h is an API header that plugins include, and plugins don't have access to Platform.h.

Right

[Anders Carlsson]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #30)
> > > I'd prefer to stay with the XP-defines or add another level to make it clearer what they mean but to map them to the XP-defines.
> > 
> > That makes sense to me.
> 
> So we could have:
> 
> #define NESCAPE_PLUGIN_PLATFORM(WTF_FEATURE) (defined XP_##WTF_FEATURE && XP_##WTF_FEATURE)
> 

I like this, but it would be far more elegant if we didn't have to #define XP_X11. I suppose we could do something like

#ifdef XP_MACOSX
#define WTF_XP_MACOSX XP_MACOSX
#endif

#ifdef XP_WIN
#define WTF_XP_WIN XP_WIN
#endif

#ifdef XP_UNIX
#define WTF_XP_X11 XP_UNIX
#endif

[Anders Carlsson]

Or we could just change the global npapi.h!

[Balazs Kelemen]

Comment on attachment 72084 [details]
GTK build fix

Remove r? since this approached had
been rejected.

[Balazs Kelemen]

Created attachment 83298 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 83298 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83298&action=review

Looks ok, but I have a few comments

> Source/WebCore/plugins/PluginPackage.cpp:42
> +#if XP_UNIX
> +#include "x11/X11PluginQuirks.h"
> +#endif

for Qt we could check for X11. Q_WS_X11

> Source/WebCore/plugins/x11/X11PluginQuirks.cpp:5
> + * Copyright (C) 2006, 2007, 2008 Apple Inc.  All rights reserved.
> + * Copyright (C) 2008 Collabora Ltd. All rights reserved.
> + * Copyright (C) 2009 Holger Hans Peter Freyther
> + * Copyright (C) 2010 University of Szeged. All rights reserved.

Strange that there is no Nokia copyright

[Balazs Kelemen]

> > > WebCore/plugins/unix/UNIXPluginQuirks.cpp:139
> > > +#if defined(Q_WS_MAEMO_5) || defined(Q_WS_MAEMO_6)
> > > +    // Passing a 32-bit depth pixmap to NPAPI plugins is too inefficient. Instead, pass a X Pixmap
> > > +    // that has same depth as the screen depth since graphics operations are optimized
> > > +    // for this depth.
> > > +    quirks->add(PluginQuirkRequiresDefaultScreenDepth);
> > > +#endif
> > 
> > This seems qt specific and it is not inside #if PLATFORM(QT). Maybe that would make it more clean that it is a Qt workaround?
> 

I have forgot this in the new patch. Can be handled before landing if the patch
is good otherwise.

[Balazs Kelemen]

(In reply to comment #38)
> (From update of attachment 83298 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83298&action=review
> 
> Looks ok, but I have a few comments
> 
> > Source/WebCore/plugins/PluginPackage.cpp:42
> > +#if XP_UNIX
> > +#include "x11/X11PluginQuirks.h"
> > +#endif
> 
> for Qt we could check for X11. Q_WS_X11

I don't mind using that but we need to check XP_UNIX for GTK and others.
Some grep output:

GNUmakefile.am
168:# For the Gtk port we want to use XP_UNIX both in X11 and Mac
169-if !TARGET_WIN32
170-global_cppflags += \
171:    -DXP_UNIX
172-endif

Source/cmake/OptionsCommon.cmake-11-IF (WTF_OS_UNIX)
Source/cmake/OptionsCommon.cmake:12:    ADD_DEFINITIONS(-DXP_UNIX)
Source/cmake/OptionsCommon.cmake-13-ENDIF (WTF_OS_UNIX)

> 
> > Source/WebCore/plugins/x11/X11PluginQuirks.cpp:5
> > + * Copyright (C) 2006, 2007, 2008 Apple Inc.  All rights reserved.
> > + * Copyright (C) 2008 Collabora Ltd. All rights reserved.
> > + * Copyright (C) 2009 Holger Hans Peter Freyther
> > + * Copyright (C) 2010 University of Szeged. All rights reserved.
> 
> Strange that there is no Nokia copyright

It is mostly copy-paste so I just copied the copyrights. Should I add Nokia? (Sounds strange to me.)

[Kenneth Rohde Christiansen]

> 
> It is mostly copy-paste so I just copied the copyrights. Should I add Nokia? (Sounds strange to me.)

Why? Most of that code was done by Girish under Nokia contract. I even think that I did parts of it, if I'm not mistaken.

[Csaba Osztrogonác]

(In reply to comment #41)
> > 
> > It is mostly copy-paste so I just copied the copyrights. Should I add Nokia? (Sounds strange to me.)
> 
> Why? Most of that code was done by Girish under Nokia contract. I even think that I did parts of it, if I'm not mistaken.


I don't understand why should Balázs add Nokia copyright in this patch.

If Girishr missed to add this copyright before, we should add it 
in a separate patch with reference to the original changesets.

[Balazs Kelemen]

(In reply to comment #42)
> (In reply to comment #41)
> > > 
> > > It is mostly copy-paste so I just copied the copyrights. Should I add Nokia? (Sounds strange to me.)
> > 
> > Why? Most of that code was done by Girish under Nokia contract. I even think that I did parts of it, if I'm not mistaken.
> 
> 
> I don't understand why should Balázs add Nokia copyright in this patch.
> 
> If Girishr missed to add this copyright before, we should add it 
> in a separate patch with reference to the original changesets.

I don't care about copyrights, please guys make a compromise about this and tell me what to do.

[Kenneth Rohde Christiansen]

(In reply to comment #42)
> (In reply to comment #41)
> > > 
> > > It is mostly copy-paste so I just copied the copyrights. Should I add Nokia? (Sounds strange to me.)
> > 
> > Why? Most of that code was done by Girish under Nokia contract. I even think that I did parts of it, if I'm not mistaken.
> 
> 
> I don't understand why should Balázs add Nokia copyright in this patch.
> 
> If Girishr missed to add this copyright before, we should add it 
> in a separate patch with reference to the original changesets.

Yes that is fine, it could be added separately, but I thought it had gone missing during the moving around of files.

[Balazs Kelemen]

Comment on attachment 83298 [details]
Patch

While hacking on the rest of this I have became dubious about that this will be good.

[Balazs Kelemen]

Let me summarize the problems. For making flash work we need to determine some quirks
even before calling NP_Initialize. At that point we only have the plugin path but don't
know exactly the supported mime types, description, etc., so we just guess that this is
that "wonderful" piece of software. Unfortunately there is no way on linux to query those
information without actually loading and initializing the plugin. After we have initialized
the plugin we can call NP_GetMIMEDescription and NP_GetValue and determine the rest of plugins.

I see two possibility for setting up things correctly:
1. add two X11 specific helper to NetscapePlugin, like determineQuirksByPluginPath and
determineQuirksByQueryingPlugin and call them before and after calling NP_Initialize in
NetscapePlugin::initialize. Also we need a quirk set member (likely WebCore::PluginQuirkSet)
but this seems necessary anyway. Pro: somewhat straightforward. Contra: #ifdef's.
2. determine quirks at the UI side - where we need to load the plugin lib anyway to get the mime
types - by reusing logic from WebCore (PluginPackage) and send them (in the case of X11). 
Pro: less #ifdef. Contra: (even more) different behavior on X11.

So, which one do you like better, or is there a better approach that I don't see?

[Balazs Kelemen]

Created attachment 83667 [details]
Patch

[Balazs Kelemen]

At least flash returns with NPERR_NO_ERROR from NP_Initialize with this.
Avoid duplicating the hacks for flash seems to be overkill so I had give
up with the refactoring.

[WebKit Review Bot]

Attachment 83667 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7983679

[Balazs Kelemen]

Created attachment 83794 [details]
Patch

Fix build

[Balazs Kelemen]

Created attachment 83803 [details]
Patch

Fix handling of the NPNVToolkit case in NPN_GetValue - return with error if the callback is not expected.

[Balazs Kelemen]

Created attachment 84074 [details]
Patch - partI

Part I. PLUGIN_ARCHITECTURE macros and X11 way of initialization.

[Balazs Kelemen]

The change will need a follow-up that replaces PLATFORM(x) macros with
PLUGIN_ARCHITECTURE(x).

[Balazs Kelemen]

Created attachment 84079 [details]
Patch - partII

Part II. X11 quirks (for flash).

[Balazs Kelemen]

Comment on attachment 84074 [details]
Patch - partI

Didn't meant to obsolete it.

[Build Bot]

Attachment 84079 [details] did not build on win:
Build output: http://queues.webkit.org/results/8071367

[Anders Carlsson]

Comment on attachment 84079 [details]
Patch - partII

View in context: https://bugs.webkit.org/attachment.cgi?id=84079&action=review

I'm going to go ahead and mark this as r- because it didn't build on Windows. Please upload a new patch and I'll r+ it.

> Source/WebKit2/Shared/Plugins/PluginQuirks.h:42
> +        RequiresGTKToolKit,

Please add a comment to this quirk that explain what it's for and why it's necessary.

> Source/WebKit2/WebProcess/Plugins/Netscape/NetscapeBrowserFuncs.cpp:490
> +               *reinterpret_cast<uint32_t*>(value) = 2;

Please add a comment explaining what "2" means here, or maybe put it in a constant.

[Early Warning System Bot]

Attachment 84079 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8070438

[WebKit Review Bot]

Attachment 84079 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8073433

[Balazs Kelemen]

Comment on attachment 84074 [details]
Patch - partI

Clearing flags on attachment: 84074

Committed r79925: <http://trac.webkit.org/changeset/79925>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Balazs Kelemen]

Reopen for the second patch (I should use webkit-patch more carefully).

[Balazs Kelemen]

The second patch depends on the first one, that caused the build failures.

[Balazs Kelemen]

Created attachment 84140 [details]
Patch - part II v2

Incorporated review comments.

[Csaba Osztrogonác]

(In reply to comment #60)
> (From update of attachment 84074 [details])
> Clearing flags on attachment: 84074
> 
> Committed r79925: <http://trac.webkit.org/changeset/79925>

Unfortunately it broke Qt Linux Release Minimal buildbot. :(
(Clean build with r79924 works, but clean build with r79925 doesn't work.)

The error message:
../../WebCore/release/libwebcore.a(PluginPackageNone.o): In function `WebCore::PluginPackage::determineQuirks(WTF::String const&)':
PluginPackageNone.cpp:(.text._ZN7WebCore13PluginPackage15determineQuirksERKN3WTF6StringE+0x0): multiple definition of `WebCore::PluginPackage::determineQuirks(WTF::String const&)'
../../WebCore/release/libwebcore.a(PluginPackage.o):PluginPackage.cpp:(.text._ZN7WebCore13PluginPackage15determineQuirksERKN3WTF6StringE+0x0): first defined here
collect2: ld returned 1 exit status
make[1]: *** [../../lib/libQtWebKit.so.4.9.0] Error 1

It seems that PluginPackage::determineQuirks is multiple defined by 
PluginPackage.cpp and PluginPackageNone.cpp.

I think PluginPackage::determineQuirks from PluginPackageNone.cpp
wasn't build previously and the conflict was revieled by your change.
(moving XP_UNIX definition)

[WebKit Review Bot]

http://trac.webkit.org/changeset/79925 might have broken Qt Linux Release minimal

[Andras Becsi]

(In reply to comment #65)
> I think PluginPackage::determineQuirks from PluginPackageNone.cpp
> wasn't build previously and the conflict was revieled by your change.
> (moving XP_UNIX definition)

The minimal build was fixed in http://trac.webkit.org/changeset/79982.

[Balazs Kelemen]

Comment on attachment 84140 [details]
Patch - part II v2

Clearing flags on attachment: 84140

Committed r80007: <http://trac.webkit.org/changeset/80007>

[Balazs Kelemen]

We are still far from working plugins in Qt/GTK WebKit2 but we should continue the work in new bugs.