Bug 27706 – WebCore NPAPI implementations should share more code

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

[Tor Arne Vestbø]

This is a meta-bug for collecting patches that try to merge some of the code in the various PluginView implementations.

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Anders Carlsson]

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.

[Anders Carlsson]

Comment on attachment 33539 [details]
Share implementation of PluginView::userAgent() and userAgentStatic()

Looks great, r=me.

[Anders Carlsson]

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 :)

[Anders Carlsson]

Comment on attachment 33537 [details]
Unregister plugin from PluginMainThreadScheduler if NPP_New fails

r=me

[Tor Arne Vestbø]

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

[Eric Seidel (no email)]

Yay!  This is awesome!  Aren't some of these already landed?  In which case they should have their flags cleared...

[Kenneth Rohde Christiansen]

Really cool patches Tor Arne! Great work!

[Tor Arne Vestbø]

Comment on attachment 33536 [details]
Join the various versions of PluginView::stop() into one shared implementation

Landed in r46697

[Tor Arne Vestbø]

Comment on attachment 33537 [details]
Unregister plugin from PluginMainThreadScheduler if NPP_New fails

Landed in r46698

[Tor Arne Vestbø]

Comment on attachment 33539 [details]
Share implementation of PluginView::userAgent() and userAgentStatic()

Landed in r46699

[Tor Arne Vestbø]

I'll rework the last two patches to match the changes in r46649

[Tor Arne Vestbø]

Created attachment 38812 [details]
Share PluginView::init() between ports

Patch by Tor Arne Vestbø <[e-mail]> 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(-)

[Tor Arne Vestbø]

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(-)

[Eric Seidel (no email)]

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.

[Tor Arne Vestbø]

(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.

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Eric Seidel (no email)]

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!

[Eric Seidel (no email)]

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.

[Tor Arne Vestbø]

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(-)

[Tor Arne Vestbø]

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(-)

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

Comment on attachment 38921 [details]
Share PluginView::init() between ports

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

Otherwise looks good.

[Tor Arne Vestbø]

Thanks Eric! I'll fix the style and commit myself :)

[Tor Arne Vestbø]

Closing, as I don't have any more patches for this at the moment.