Bug 37478

Summary: Update PluginViewNone and PluginPackageNone to the last Plugin API
Product: WebKit Reporter: Gustavo Sverzut Barbieri <barbieri>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, cjerdonek, commit-queue, dbates, dglazkov, eric, joepeck, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Update WebCore/plugins/Plugin*None.cpp to new API
none
Update WebCore/plugins/Plugin*None.cpp to new API, isolated to XP_UNIX and NETSCAPE_PLUGIN_API
none
Update PluginViewNone and PluginPackageNone to the last Plugin API
none
Update PluginViewNone and PluginPackageNone to the last Plugin API
none
Update PluginViewNone and PluginPackageNone to the last Plugin API none

Description Gustavo Sverzut Barbieri 2010-04-12 17:18:05 PDT
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.
Comment 1 WebKit Review Bot 2010-04-12 17:22:52 PDT
Attachment 53203 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1682241
Comment 2 Gustavo Sverzut Barbieri 2010-04-12 17:52:48 PDT
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).
Comment 3 Leandro Pereira 2010-04-13 06:55:13 PDT
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.
Comment 4 Dave Hyatt 2010-04-13 11:55:48 PDT
Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

r=me
Comment 5 WebKit Commit Bot 2010-04-13 12:32:06 PDT
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 eseidel@chromium.org...
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).
Comment 6 Eric Seidel (no email) 2010-04-13 13:21:02 PDT
I'm not sure what went wrong for the commit-queue, I'll investigate.
Comment 7 Eric Seidel (no email) 2010-04-13 13:28:55 PDT
Comment on attachment 53241 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

Trying again?
Comment 8 WebKit Commit Bot 2010-04-13 13:42:21 PDT
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 eseidel@chromium.org...
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).
Comment 9 Eric Seidel (no email) 2010-04-13 13:44:54 PDT
I'll figure out what's going wrong and fix it this afternoon.  Sorry again!
Comment 10 Eric Seidel (no email) 2010-04-13 14:07:09 PDT
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  <mnaganov@chromium.org>
+2010-04-13  Leandro Pereira  <leandro@profusion.mobi>
+
+        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.
Comment 11 Chris Jerdonek 2010-04-13 15:48:46 PDT
(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  <mnaganov@chromium.org>
> +2010-04-13  Leandro Pereira  <leandro@profusion.mobi>
> +

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 @@
+2010-04-12  Leandro Pereira  <leandro@profusion.mobi>
+
+        Reviewed by NOBODY (OOPS!).

(from https://bugs.webkit.org/attachment.cgi?id=53241 )
Comment 12 Leandro Pereira 2010-04-14 07:33:33 PDT
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.
Comment 13 Eric Seidel (no email) 2010-04-14 09:07:34 PDT
Comment on attachment 53330 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

OK.
Comment 14 WebKit Commit Bot 2010-04-14 09:41:02 PDT
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
Comment 15 Eric Seidel (no email) 2010-04-14 09:45:13 PDT
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
Comment 16 Joseph Pecoraro 2010-04-14 11:39:15 PDT
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.
Comment 17 Leandro Pereira 2010-04-15 13:43:20 PDT
Created attachment 53467 [details]
Update PluginViewNone and PluginPackageNone to the last Plugin API

This updated patch should build correctly under Mac.
Comment 18 Eric Seidel (no email) 2010-04-18 16:26:19 PDT
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.
Comment 19 Eric Seidel (no email) 2010-04-18 16:26:31 PDT
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.
Comment 20 WebKit Commit Bot 2010-04-19 23:30:10 PDT
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>
Comment 21 WebKit Commit Bot 2010-04-19 23:30:18 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Gustavo Sverzut Barbieri 2010-04-20 07:39:03 PDT
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.
Comment 23 Adam Roben (:aroben) 2010-04-20 07:48:49 PDT
(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
Comment 24 Gustavo Sverzut Barbieri 2010-04-20 10:12:36 PDT
(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.