Bug 37478 – Update PluginViewNone and PluginPackageNone to the last Plugin API

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

[Gustavo Sverzut Barbieri]

Created attachment 53203 [details]
Update WebCore/plugins/Plugin*None.cpp to new API

Plugin API was updated but Plugin*None were not, leading to users of it to not link due unknown symbols.

The following patch by my co-worker Leandro Pereira fixes it. We noticed so while changing bugs #35761 and #35762 to use Plugin*None as requested by Zecke.

No runtime tests are requested.

[WebKit Review Bot]

Attachment 53203 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1682241

[Gustavo Sverzut Barbieri]

Created attachment 53206 [details]
Update WebCore/plugins/Plugin*None.cpp to new API, isolated to XP_UNIX and NETSCAPE_PLUGIN_API

Isolates handleFocusInEvent and handleFocusOutEvent inside defined(XP_UNIX) && ENABLE(NETSCAPE_PLUGIN_API).

[Leandro Pereira]

Created attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Updated patch adds a reference to this bug report on the ChangeLog entry.

[Dave Hyatt]

Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

r=me

[WebKit Commit Bot]

Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Rejecting patch 53241 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53241', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=53241&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=37478&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 53241 from bug 37478.
WARNING: Creating invalid ChangeLogEntry:

ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Eric Seidel (no email)]

I'm not sure what went wrong for the commit-queue, I'll investigate.

[Eric Seidel (no email)]

Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Trying again?

[WebKit Commit Bot]

Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Rejecting patch 53241 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', '53241', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=53241&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=37478&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 53241 from bug 37478.
WARNING: Creating invalid ChangeLogEntry:

ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Eric Seidel (no email)]

I'll figure out what's going wrong and fix it this afternoon.  Sorry again!

[Eric Seidel (no email)]

The patch is failing to apply correctly:

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 2a5dc84..1d287cb 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,4 +1,15 @@
 2010-04-13  Mikhail Naganov  <[e-mail]>
[phone]  Leandro Pereira  <[e-mail]>
+
+        Reviewed by David Hyatt.
+
+        Update WebCore/plugins/Plugin*None.cpp to conform to the
+        respective Plugin*.cpp API.
+        http://webkit.org/b/37478
+
+        * plugins/PluginPackageNone.cpp: Changed.
+        * plugins/PluginViewNone.cpp: Changed.
+
 
         Reviewed by Pavel Feldman.
 
This may be a svn-apply bug, or this may be a problem with the patch.

[Chris Jerdonek]

(In reply to comment #10)
> The patch is failing to apply correctly:
> 
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 2a5dc84..1d287cb 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,4 +1,15 @@
>  2010-04-13  Mikhail Naganov  <[e-mail]>
> [phone]  Leandro Pereira  <[e-mail]>
> +

It could have to do with the fact that the patch attachment's ChangeLog has no leading or ending context (the -1,0 means that the "before" chunk has 0 lines):

diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
index 1de8c1e..e9a8738 100644
--- a/WebCore/ChangeLog
+++ b/WebCore/ChangeLog
@@ -1,0 +1,11 @@
[phone]  Leandro Pereira  <[e-mail]>
+
+        Reviewed by NOBODY (OOPS!).

(from https://bugs.webkit.org/attachment.cgi?id=53241 )

[Leandro Pereira]

Created attachment 53330 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

This patch has been already r+'d by Hyatt, but was not created with the WebKit tools, so it didn't apply correctly. Here's an updated version that applies cleanly with svn-apply.

[Eric Seidel (no email)]

Comment on attachment 53330 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

OK.

[WebKit Commit Bot]

Comment on attachment 53330 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Rejecting patch 53330 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
ORT_DIR /Developer/Library/Xcode
    setenv XCODE_VERSION_ACTUAL 0313
    setenv XCODE_VERSION_MAJOR 0300
    setenv XCODE_VERSION_MINOR 0310
    setenv YACC /Developer/usr/bin/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Ld /Users/eseidel/Projects/CommitQueue/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal i386
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/1602469

[Eric Seidel (no email)]

Undefined symbols:
  "__ZN7WebCore10PluginView19setJavaScriptPausedEb", referenced from:
      __ZN7WebCore17ScriptDebugServer19setJavaScriptPausedEPNS_9FrameViewEb in ScriptDebugServer.o
  "__ZN7WebCore10PluginView27privateBrowsingStateChangedEb", referenced from:
      __ZN7WebCore4Page27privateBrowsingStateChangedEv in Page.o
  "__ZN7WebCore10PluginView9keepAliveEP4_NPP", referenced from:
      __NPN_Evaluate in NP_jsobject.o
ld: symbol(s) not found
collect2: ld returned 1 exit status

[Joseph Pecoraro]

You asked on IRC, so I took a brief look. For the first one issue It looks like PluginView's setJavaScriptPaused, which you change in this patch is actually used elsewhere:
http://trac.webkit.org/browser/trunk/WebCore/bindings/js/ScriptDebugServer.cpp#L395

> -void PluginView::setJavaScriptPaused(bool)
> +void PluginView::handleFocusOutEvent()

Also, you can reverse the symbol using the c++filt command. Even though its normally pretty obvious sometimes this helps:

> shell> c++filt __ZN7WebCore10PluginView19setJavaScriptPausedEb
> WebCore::PluginView::setJavaScriptPaused(bool)

> shell> c++filt __ZN7WebCore17ScriptDebugServer19setJavaScriptPausedEPNS_9FrameViewEb
> WebCore::ScriptDebugServer::setJavaScriptPaused(WebCore::FrameView*, bool)

Hopefully that gives you some leads.

[Leandro Pereira]

Created attachment 53467 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

This updated patch should build correctly under Mac.

[Eric Seidel (no email)]

Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Cleared David Hyatt's review+ from obsolete attachment 53241 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 53330 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Cleared Eric Seidel's review+ from obsolete attachment 53330 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 53467 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Clearing flags on attachment: 53467

Committed r57873: <http://trac.webkit.org/changeset/57873>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Gustavo Sverzut Barbieri]

Hi all,

The applied patch still does not fix all the issues, if one links PluginView.cpp with PluginViewNone.cpp, it will not work since the following methods are defined in both:

 * void PluginPackage::determineQuirks(const String& mimeType);
 * void PluginView::keepAlive(NPP instance);
 * void PluginView::setJavaScriptPaused(bool paused);
 * void PluginView::privateBrowsingStateChanged(bool privateBrowsingEnabled);

If we remove these from Plugin*None.cpp as we suggested before, it will not work on Mac/XCode as it will not link PluginView.cpp, just PluginViewNone.cpp and thus the error.

I see two options:
 1. Link Mac with PluginView.cpp, but needs someone from Mac to review;
 2. ifdef these functions in PluginViewNone.cpp to just provide them if platform is MAC, or not XP_UNIX...

I guess the first is more correct, but I'm not aware of the side effects.

[Adam Roben (:aroben)]

(In reply to comment #22)
> The applied patch still does not fix all the issues, if one links
> PluginView.cpp with PluginViewNone.cpp, it will not work since the following
> methods are defined in both:

What port is trying to link both PluginView.cpp and PluginViewNone.cpp? That doesn't seem right. I think all ports should either:

1) Link PluginViewNone.cpp, or
2) Link PluginView.cpp plus PluginViewYourPort.cpp

[Gustavo Sverzut Barbieri]

(In reply to comment #23)
> (In reply to comment #22)
> > The applied patch still does not fix all the issues, if one links
> > PluginView.cpp with PluginViewNone.cpp, it will not work since the following
> > methods are defined in both:
> 
> What port is trying to link both PluginView.cpp and PluginViewNone.cpp? That
> doesn't seem right. I think all ports should either:
> 
> 1) Link PluginViewNone.cpp, or
> 2) Link PluginView.cpp plus PluginViewYourPort.cpp

Well, after your help at IRC, I finally managed to fix it by using just PluginViewNone.cpp, not requiring PluginView.cpp.  However I had to manually disable some code paths that would call Plugin* code, like the PluginDatabase and so on.

Given this, I'm closing this bug. But it would be really good to have PluginView.cpp to be able to link with PluginViewNone.cpp and the methods would disable automatically.