Bug 48127

Summary: [Qt][WK2] Plugin initialization
Product: WebKit Reporter: Balazs Kelemen <kbalazs>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, abecsi, andersca, aroben, benjamin, buildbot, christian.webkit, eric, girish, gustavo, kenneth, kling, laszlo.gombos, ossy, tonikitoo, webkit-ews, webkit.review.bot, xan.lopez
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
proposed patch
none
build fixed patch
none
fixed Mac build
none
GTK build fix
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch - partI
none
Patch - partII
none
Patch - part II v2 none

Description Balazs Kelemen 2010-10-22 06:12:08 PDT
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.
Comment 1 Balazs Kelemen 2010-10-25 09:41:00 PDT
Created attachment 71764 [details]
proposed patch
Comment 2 Balazs Kelemen 2010-10-25 09:41:34 PDT
*** Bug 46262 has been marked as a duplicate of this bug. ***
Comment 3 Early Warning System Bot 2010-10-25 10:00:24 PDT
Attachment 71764 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4623086
Comment 4 Anders Carlsson 2010-10-25 10:15:20 PDT
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?
Comment 5 Adam Roben (:aroben) 2010-10-25 11:02:08 PDT
(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).
Comment 6 Eric Seidel (no email) 2010-10-25 11:18:17 PDT
Attachment 71764 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4715089
Comment 7 Kenneth Rohde Christiansen 2010-10-25 12:02:56 PDT
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.
Comment 8 Anders Carlsson 2010-10-25 12:11:38 PDT
(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 :)
Comment 9 Balazs Kelemen 2010-10-26 01:56:52 PDT
(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.
Comment 10 Balazs Kelemen 2010-10-26 02:14:31 PDT
Created attachment 71853 [details]
build fixed patch

Same as the previous one with build fix.
Comment 11 Eric Seidel (no email) 2010-10-26 03:11:24 PDT
Attachment 71853 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4748016
Comment 12 Balazs Kelemen 2010-10-26 09:10:38 PDT
Created attachment 71897 [details]
fixed Mac build
Comment 13 Anders Carlsson 2010-10-26 10:22:50 PDT
(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?
Comment 14 Balazs Kelemen 2010-10-26 15:02:41 PDT
> 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.
Comment 15 WebKit Review Bot 2010-10-27 11:52:24 PDT
Attachment 71897 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/4807035
Comment 16 Balazs Kelemen 2010-10-27 14:16:46 PDT
Created attachment 72084 [details]
GTK build fix
Comment 17 Kenneth Rohde Christiansen 2010-10-27 14:21:47 PDT
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?
Comment 18 Kenneth Rohde Christiansen 2010-10-27 14:23:07 PDT
(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.
Comment 19 Kenneth Rohde Christiansen 2010-10-27 14:29:57 PDT
Are these not X11 quirks? That would be a better name than UNIX.
Comment 20 Kenneth Rohde Christiansen 2010-10-27 14:30:34 PDT
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
Comment 21 Anders Carlsson 2010-10-27 14:32:14 PDT
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.
Comment 22 Kenneth Rohde Christiansen 2010-10-27 14:37:53 PDT
(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.
Comment 23 Balazs Kelemen 2010-10-27 14:57:57 PDT
(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.
Comment 24 Balazs Kelemen 2010-10-27 14:58:48 PDT
(In reply to comment #19)
> Are these not X11 quirks? That would be a better name than UNIX.

Right.
Comment 25 Balazs Kelemen 2010-10-27 15:06:51 PDT
(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.
Comment 26 Balazs Kelemen 2010-10-27 15:16:47 PDT
> > 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.
Comment 27 Anders Carlsson 2010-10-27 16:10:23 PDT
(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?
Comment 28 Anders Carlsson 2010-10-27 16:11:28 PDT
(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.
Comment 29 Tor Arne Vestbø 2010-11-02 10:40:24 PDT
(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.
Comment 30 Tor Arne Vestbø 2010-11-03 06:58:47 PDT
(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?
Comment 31 Antonio Gomes 2010-11-03 07:01:28 PDT
> 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.
Comment 32 Adam Roben (:aroben) 2010-11-03 07:05:07 PDT
(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.
Comment 33 Tor Arne Vestbø 2010-11-03 09:10:59 PDT
(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
Comment 34 Anders Carlsson 2010-11-03 10:12:36 PDT
(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
Comment 35 Anders Carlsson 2010-11-03 10:14:18 PDT
Or we could just change the global npapi.h!
Comment 36 Balazs Kelemen 2010-11-06 12:03:17 PDT
Comment on attachment 72084 [details]
GTK build fix

Remove r? since this approached had
been rejected.
Comment 37 Balazs Kelemen 2011-02-22 04:30:52 PST
Created attachment 83298 [details]
Patch
Comment 38 Kenneth Rohde Christiansen 2011-02-22 04:55:15 PST
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
Comment 39 Balazs Kelemen 2011-02-22 04:57:58 PST
> > > 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.
Comment 40 Balazs Kelemen 2011-02-22 05:46:43 PST
(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.)
Comment 41 Kenneth Rohde Christiansen 2011-02-22 05:50:59 PST
> 
> 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.
Comment 42 Csaba Osztrogonác 2011-02-22 06:05:43 PST
(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.
Comment 43 Balazs Kelemen 2011-02-22 06:10:13 PST
(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.
Comment 44 Kenneth Rohde Christiansen 2011-02-22 06:10:49 PST
(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.
Comment 45 Balazs Kelemen 2011-02-22 10:31:20 PST
Comment on attachment 83298 [details]
Patch

While hacking on the rest of this I have became dubious about that this will be good.
Comment 46 Balazs Kelemen 2011-02-22 15:52:29 PST
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?
Comment 47 Balazs Kelemen 2011-02-24 09:08:31 PST
Created attachment 83667 [details]
Patch
Comment 48 Balazs Kelemen 2011-02-24 09:10:57 PST
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.
Comment 49 WebKit Review Bot 2011-02-24 18:00:08 PST
Attachment 83667 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7983679
Comment 50 Balazs Kelemen 2011-02-25 04:24:14 PST
Created attachment 83794 [details]
Patch

Fix build
Comment 51 Balazs Kelemen 2011-02-25 06:39:51 PST
Created attachment 83803 [details]
Patch

Fix handling of the NPNVToolkit case in NPN_GetValue - return with error if the callback is not expected.
Comment 52 Balazs Kelemen 2011-02-28 09:37:33 PST
Created attachment 84074 [details]
Patch - partI

Part I. PLUGIN_ARCHITECTURE macros and X11 way of initialization.
Comment 53 Balazs Kelemen 2011-02-28 10:01:54 PST
The change will need a follow-up that replaces PLATFORM(x) macros with
PLUGIN_ARCHITECTURE(x).
Comment 54 Balazs Kelemen 2011-02-28 10:24:27 PST
Created attachment 84079 [details]
Patch - partII

Part II. X11 quirks (for flash).
Comment 55 Balazs Kelemen 2011-02-28 10:25:27 PST
Comment on attachment 84074 [details]
Patch - partI

Didn't meant to obsolete it.
Comment 56 Build Bot 2011-02-28 11:04:42 PST
Attachment 84079 [details] did not build on win:
Build output: http://queues.webkit.org/results/8071367
Comment 57 Anders Carlsson 2011-02-28 11:13:34 PST
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.
Comment 58 Early Warning System Bot 2011-02-28 11:22:52 PST
Attachment 84079 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8070438
Comment 59 WebKit Review Bot 2011-02-28 14:14:55 PST
Attachment 84079 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8073433
Comment 60 Balazs Kelemen 2011-02-28 14:48:39 PST
Comment on attachment 84074 [details]
Patch - partI

Clearing flags on attachment: 84074

Committed r79925: <http://trac.webkit.org/changeset/79925>
Comment 61 Balazs Kelemen 2011-02-28 14:48:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 62 Balazs Kelemen 2011-02-28 14:50:11 PST
Reopen for the second patch (I should use webkit-patch more carefully).
Comment 63 Balazs Kelemen 2011-02-28 15:51:08 PST
The second patch depends on the first one, that caused the build failures.
Comment 64 Balazs Kelemen 2011-02-28 15:56:30 PST
Created attachment 84140 [details]
Patch - part II v2

Incorporated review comments.
Comment 65 Csaba Osztrogonác 2011-02-28 23:40:45 PST
(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)
Comment 66 WebKit Review Bot 2011-02-28 23:56:59 PST
http://trac.webkit.org/changeset/79925 might have broken Qt Linux Release minimal
Comment 67 Andras Becsi 2011-03-01 08:59:31 PST
(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.
Comment 68 Balazs Kelemen 2011-03-01 09:47:21 PST
Comment on attachment 84140 [details]
Patch - part II v2

Clearing flags on attachment: 84140

Committed r80007: <http://trac.webkit.org/changeset/80007>
Comment 69 Balazs Kelemen 2011-03-01 09:52:09 PST
We are still far from working plugins in Qt/GTK WebKit2 but we should continue the work in new bugs.