Bug 27706 - WebCore NPAPI implementations should share more code
Summary: WebCore NPAPI implementations should share more code
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Tor Arne Vestbø
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-27 04:45 PDT by Tor Arne Vestbø
Modified: 2009-09-10 01:32 PDT (History)
7 users (show)

See Also:


Attachments
Refactor the common parts out of PluginView::init() and add platformInit() (9.48 KB, patch)
2009-07-27 06:16 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Join the various versions of PluginView::stop() into one shared implementation (11.75 KB, patch)
2009-07-27 06:16 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Unregister plugin from PluginMainThreadScheduler if NPP_New fails (1.31 KB, patch)
2009-07-27 06:16 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Join the various PluginView destructors into one shared implementation (6.24 KB, patch)
2009-07-27 06:17 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Share implementation of PluginView::userAgent() and userAgentStatic() (6.75 KB, patch)
2009-07-27 06:17 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Refactor the common parts out of PluginView::init() and add platformInit() (9.48 KB, patch)
2009-07-27 06:27 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Share PluginView::init() between ports (10.23 KB, patch)
2009-08-31 06:21 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Join the various PluginView destructors into one shared implementation (7.43 KB, patch)
2009-08-31 06:21 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Share PluginView::init() between ports (10.39 KB, patch)
2009-09-01 05:19 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Join the various PluginView destructors into one shared implementation (7.38 KB, patch)
2009-09-01 05:20 PDT, Tor Arne Vestbø
no flags Details | Formatted Diff | Diff
Join the various PluginView destructors into one shared implementation (8.21 KB, patch)
2009-09-02 03:22 PDT, Tor Arne Vestbø
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff
Share PluginView::init() between ports (10.43 KB, patch)
2009-09-02 03:23 PDT, Tor Arne Vestbø
eric: review+
eric: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tor Arne Vestbø 2009-07-27 04:45:56 PDT
This is a meta-bug for collecting patches that try to merge some of the code in the various PluginView implementations.
Comment 1 Tor Arne Vestbø 2009-07-27 06:16:07 PDT
Created attachment 33535 [details]
Refactor the common parts out of PluginView::init() and add platformInit()


Reviewed by NOBODY (OOPS!).

Refactor the common parts out of PluginView::init() and add platformInit()

https://bugs.webkit.org/show_bug.cgi?id=27706

PluginViewMac used to call stop() in some cases, but this error handling
should be unified across the different implementations, so removing for now.

m_hasPendingGeometryChange is now initialized in the initializer list
instead of in PluginViewQt's init()

* plugins/PluginView.cpp:
(WebCore::PluginView::handleEvent):
(WebCore::PluginView::init):
(WebCore::PluginView::bindingInstance):
* plugins/PluginView.h:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
(WebCore::PluginView::platformInit):
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 71 insertions(+), 100 deletions(-)
Comment 2 Tor Arne Vestbø 2009-07-27 06:16:32 PDT
Created attachment 33536 [details]
Join the various versions of PluginView::stop() into one shared implementation

Reviewed by NOBODY (OOPS!).

Join the various versions of PluginView::stop() into one shared implementation

https://bugs.webkit.org/show_bug.cgi?id=27706

The platform-dependent bits are now ifdef'ed in the shared implementation,
using the XP_ defines (XP_WIN, XP_MACOSX, XP_UNIX) from npapi.h

Differences to the original implementation are as follows:

- The Qt port unregistered the plugin before calling NP_SetWindow
with a 0 window handle. Now it's done after (similar to Win)

- The GTK and Qt ports unregistered the plugin before clearing
the m_npWindow.ws_info struct. Now it's done after.

- The Win port constructed the DropAllLocks after unsubclassing
the window. Now it's done before.

- The Mac, Qt, and GTK ports did not pass a NPSavedData struct
to NP_Destroy (like the Win port). Now all ports do.

- The Win port did not call PluginView::setCurrentPluginView()
before calling to NP-functions. Now it does.

* plugins/PluginView.cpp:
(WebCore::PluginView::start):
(WebCore::PluginView::~PluginView):
(WebCore::PluginView::stop):
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
(WebCore::PluginView::getValueStatic):
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
(WebCore::PluginView::handlePostReadFile):
---
 7 files changed, 110 insertions(+), 181 deletions(-)
Comment 3 Tor Arne Vestbø 2009-07-27 06:16:48 PDT
Created attachment 33537 [details]
Unregister plugin from PluginMainThreadScheduler if NPP_New fails

Reviewed by NOBODY (OOPS!).

Unregister plugin from PluginMainThreadScheduler if NPP_New fails

https://bugs.webkit.org/show_bug.cgi?id=27706

Since the plugin has not been started yet (m_isStarted is not set),
calling stop() will not have any affect, so we manually unregister
in start() before returning.

* plugins/PluginView.cpp:
(WebCore::PluginView::start):
---
 2 files changed, 18 insertions(+), 1 deletions(-)
Comment 4 Tor Arne Vestbø 2009-07-27 06:17:08 PDT
Created attachment 33538 [details]
Join the various PluginView destructors into one shared implementation

Reviewed by NOBODY (OOPS!).

Join the various PluginView destructors into one shared implementation

https://bugs.webkit.org/show_bug.cgi?id=27706

Differences to the original implementation are as follows:

- The Mac port does not set m_window to 0 anymore.

- The Qt port used to delete the platform plugin widget as
the last step. Now this is done before cleaning up the
script objects and unloading the plugin package (like
the Win port).

* plugins/PluginView.cpp:
(WebCore::PluginView::~PluginView):
* plugins/PluginViewNone.cpp:
(WebCore::PluginView::updatePluginWidget):
* plugins/gtk/PluginViewGtk.cpp:
(WebCore::PluginView::platformInit):
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
(WebCore::PluginView::platformInit):
* plugins/win/PluginViewWin.cpp:
(WebCore::PluginView::init):
---
 7 files changed, 59 insertions(+), 74 deletions(-)
Comment 5 Tor Arne Vestbø 2009-07-27 06:17:28 PDT
Created attachment 33539 [details]
Share implementation of PluginView::userAgent() and userAgentStatic()

Reviewed by NOBODY (OOPS!).

Share implementation of PluginView::userAgent() and userAgentStatic()

https://bugs.webkit.org/show_bug.cgi?id=27706

Differences to the original implementation are as follows:

- The Win port used to return 0 for userAgentStatic, but now
returns the quirk mode MozillaUserAgent as the other ports.

* plugins/PluginView.cpp:
(WebCore::PluginView::userAgent):
(WebCore::PluginView::userAgentStatic):
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 7 files changed, 49 insertions(+), 87 deletions(-)
Comment 6 Tor Arne Vestbø 2009-07-27 06:27:19 PDT
Created attachment 33540 [details]
Refactor the common parts out of PluginView::init() and add platformInit()

Reviewed by NOBODY (OOPS!).

Refactor the common parts out of PluginView::init() and add platformInit()

https://bugs.webkit.org/show_bug.cgi?id=27706

PluginViewMac used to call stop() in some cases, but this error handling
should be unified across the different implementations, so removing for now.

m_hasPendingGeometryChange is now initialized in the initializer list
instead of in PluginViewQt's init()

* plugins/PluginView.cpp:
(WebCore::PluginView::handleEvent):
(WebCore::PluginView::init):
(WebCore::PluginView::bindingInstance):
* plugins/PluginView.h:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
(WebCore::PluginView::platformInit):
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 71 insertions(+), 100 deletions(-)
Comment 7 Anders Carlsson 2009-07-30 12:49:08 PDT
Comment on attachment 33536 [details]
Join the various versions of PluginView::stop() into one shared implementation

This looks very good, although it should use our PLATFORM defines instead of those from npapi.h Marking r- for this reason.
Comment 8 Anders Carlsson 2009-07-30 13:56:59 PDT
Comment on attachment 33539 [details]
Share implementation of PluginView::userAgent() and userAgentStatic()

Looks great, r=me.
Comment 9 Anders Carlsson 2009-07-30 13:57:21 PDT
Comment on attachment 33536 [details]
Join the various versions of PluginView::stop() into one shared implementation

Actually, this is OK and it's certainly better than what we have now :)
Comment 10 Anders Carlsson 2009-07-30 15:16:08 PDT
Comment on attachment 33537 [details]
Unregister plugin from PluginMainThreadScheduler if NPP_New fails

r=me
Comment 11 Tor Arne Vestbø 2009-07-31 05:38:25 PDT
Thanks for the reviews Anders!

On the subject of defines, what's the exact meaning of ENABLE(NETSCAPE_PLUGIN_API)? Does it refer to the NPAPI-JS-bindings only, or NPAPI support in general? 

The reason I'm asking is that the Mac NPAPI implementation in WebKit wrap all files with #if ENABLE(NETSCAPE_PLUGIN_API), but in the WebCore NPAPI implementations it's a little more spread out, for example only around PluginView::userAgentStatic() and PluginView::getValueStatic(). What's the correct usage of this macro/define? 

Thanks :D
Comment 12 Eric Seidel (no email) 2009-07-31 16:30:36 PDT
Yay!  This is awesome!  Aren't some of these already landed?  In which case they should have their flags cleared...
Comment 13 Kenneth Rohde Christiansen 2009-08-01 09:09:21 PDT
Really cool patches Tor Arne! Great work!
Comment 14 Tor Arne Vestbø 2009-08-02 06:59:05 PDT
Comment on attachment 33536 [details]
Join the various versions of PluginView::stop() into one shared implementation

Landed in r46697
Comment 15 Tor Arne Vestbø 2009-08-02 07:00:48 PDT
Comment on attachment 33537 [details]
Unregister plugin from PluginMainThreadScheduler if NPP_New fails

Landed in r46698
Comment 16 Tor Arne Vestbø 2009-08-02 07:09:46 PDT
Comment on attachment 33539 [details]
Share implementation of PluginView::userAgent() and userAgentStatic()

Landed in r46699
Comment 17 Tor Arne Vestbø 2009-08-02 07:35:49 PDT
I'll rework the last two patches to match the changes in r46649
Comment 18 Tor Arne Vestbø 2009-08-31 06:21:18 PDT
Created attachment 38812 [details]
Share PluginView::init() between ports

Patch by Tor Arne Vestbø <tor.arne.vestbo@nokia.com> on 2009-08-31
Reviewed by NOBODY (OOPS!).

The port-spesific bits are moved to platformStart(), which now returns
a bool based on the success of the platformStart.

https://bugs.webkit.org/show_bug.cgi?id=27706

PluginViewMac used to call stop() in some cases, but this error handling
should be unified across the different implementations, so removing for now.

m_hasPendingGeometryChange is now initialized in the initializer list
instead of in PluginViewQt's init()

* plugins/PluginView.cpp:
* plugins/PluginView.h:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 83 insertions(+), 119 deletions(-)
Comment 19 Tor Arne Vestbø 2009-08-31 06:21:27 PDT
Created attachment 38813 [details]
Join the various PluginView destructors into one shared implementation

https://bugs.webkit.org/show_bug.cgi?id=27706

Differences to the original implementation are as follows:

- The Mac port does not set m_window to 0 anymore

- The Qt port used to delete the platform plugin widget as
the last step. Now this is done before cleaning up the
script objects and unloading the plugin package (like
the Win port).

* plugins/PluginView.cpp:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 7 files changed, 79 insertions(+), 101 deletions(-)
Comment 20 Eric Seidel (no email) 2009-08-31 08:11:15 PDT
Comment on attachment 38812 [details]
Share PluginView::init() between ports

Why are these "regressions" OK?

Why is it OK for example to not call stop()?  I'd rather have #ifdef MAC the common code for now than to change behavior with these patches.
Comment 21 Tor Arne Vestbø 2009-09-01 04:54:56 PDT
(In reply to comment #20)
> (From update of attachment 38812 [details])
> Why are these "regressions" OK?

Good point. I'll upload a new set of patches where the first "regression", the setting m_window to 0 is restored ifdefed. The stop() case was introduced by me for the PluginViewMac (which is currenly only used by the Qt port, not the Mac port which has the plugin code in WebKit), but was not the right thing to do.

We called stop() to make sure the plugin would not be still running and doing loading etc when it wasn't supported and visible to the user, but the right thing to do is to protect those operations that should not be allowed by guards using m_status and m_isStarted.
Comment 22 Tor Arne Vestbø 2009-09-01 05:19:59 PDT
Created attachment 38860 [details]
Share PluginView::init() between ports

https://bugs.webkit.org/show_bug.cgi?id=27706

The port-spesific bits are moved to platformStart(), which now returns
a bool based on the success of the platformStart.

m_hasPendingGeometryChange is now initialized in the initializer list
instead of in PluginViewQt's init()

PluginViewMac (currently used only by the Qt port) used to manually call
stop() in the case where an unsupported drawing or event model was detected.
This was wrong, as the m_status and m_isStarted fields should be used to
guard against doing operations on a plugin in this intermediate state.

* plugins/PluginView.cpp:
* plugins/PluginView.h:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 85 insertions(+), 119 deletions(-)
Comment 23 Tor Arne Vestbø 2009-09-01 05:20:08 PDT
Created attachment 38861 [details]
Join the various PluginView destructors into one shared implementation

https://bugs.webkit.org/show_bug.cgi?id=27706

The Qt port used to delete the platform plugin widget as the last step.
Now this is done before cleaning up the script objects and unloading the
plugin package, similar to how the Win port does it.

* plugins/PluginView.cpp:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 7 files changed, 78 insertions(+), 101 deletions(-)
Comment 24 Eric Seidel (no email) 2009-09-01 06:31:44 PDT
Comment on attachment 38860 [details]
Share PluginView::init() between ports

The return seems unnecessary now:
221         m_status = PluginStatusCanNotLoadPlugin;
 222         return false;
 223     }

This won't compile:
bool PluginView::platformStart()
123123 {
124124     notImplemented();
125125 }

Otherwise this looks good to me!
Comment 25 Eric Seidel (no email) 2009-09-01 06:34:01 PDT
Comment on attachment 38861 [details]
Join the various PluginView destructors into one shared implementation

Seems this could easily be shared in a platformStop() or destroyPlatformPluginWidget() method:
0 #if defined(XP_WIN)
 261        DestroyWindow(platformPluginWidget());
 262 #elif defined(XP_MACOSX)
 263        setPlatformPluginWidget(0);
 264 #elif defined(XP_UNIX) && PLATFORM(QT)
 265        // TODO: Should this be done for GTK too?
 266        delete platformPluginWidget();
 267 #endif

r- for the above.
Comment 26 Tor Arne Vestbø 2009-09-02 03:22:57 PDT
Created attachment 38920 [details]
Join the various PluginView destructors into one shared implementation

https://bugs.webkit.org/show_bug.cgi?id=27706

The Qt port used to delete the platform plugin widget as the last step.
Now this is done before cleaning up the script objects and unloading the
plugin package, similar to how the Win port does it.

* plugins/PluginView.cpp:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 89 insertions(+), 97 deletions(-)
Comment 27 Tor Arne Vestbø 2009-09-02 03:23:05 PDT
Created attachment 38921 [details]
Share PluginView::init() between ports

https://bugs.webkit.org/show_bug.cgi?id=27706

The port-spesific bits are moved to platformStart(), which now returns
a bool based on the success of the platformStart.

m_hasPendingGeometryChange is now initialized in the initializer list
instead of in PluginViewQt's init()

PluginViewMac (currently used only by the Qt port) used to manually call
stop() in the case where an unsupported drawing or event model was detected.
This was wrong, as the m_status and m_isStarted fields should be used to
guard against doing operations on a plugin in this intermediate state.

* plugins/PluginView.cpp:
* plugins/PluginView.h:
* plugins/PluginViewNone.cpp:
* plugins/gtk/PluginViewGtk.cpp:
* plugins/mac/PluginViewMac.cpp:
* plugins/qt/PluginViewQt.cpp:
* plugins/win/PluginViewWin.cpp:
---
 8 files changed, 86 insertions(+), 119 deletions(-)
Comment 28 Eric Seidel (no email) 2009-09-02 03:27:20 PDT
Comment on attachment 38920 [details]
Join the various PluginView destructors into one shared implementation

LGTM.  cq- the ChangeLog patch would confuse svn-apply.  Besides, you're a committer. :)
Comment 29 Eric Seidel (no email) 2009-09-02 03:28:33 PDT
Comment on attachment 38921 [details]
Share PluginView::init() between ports

Style:
     if (!platformStart()) {
 221         m_status = PluginStatusCanNotLoadPlugin;
 222     }

Otherwise looks good.
Comment 30 Tor Arne Vestbø 2009-09-02 03:31:12 PDT
Thanks Eric! I'll fix the style and commit myself :)
Comment 31 Tor Arne Vestbø 2009-09-10 01:32:50 PDT
Closing, as I don't have any more patches for this at the moment.