RESOLVED FIXED Bug 37478
Update PluginViewNone and PluginPackageNone to the last Plugin API
https://bugs.webkit.org/show_bug.cgi?id=37478
Summary Update PluginViewNone and PluginPackageNone to the last Plugin API
Gustavo Sverzut Barbieri
Reported 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.
Attachments
Update WebCore/plugins/Plugin*None.cpp to new API (1.34 KB, patch)
2010-04-12 17:18 PDT, Gustavo Sverzut Barbieri
no flags
Update WebCore/plugins/Plugin*None.cpp to new API, isolated to XP_UNIX and NETSCAPE_PLUGIN_API (1.40 KB, patch)
2010-04-12 17:52 PDT, Gustavo Sverzut Barbieri
no flags
Update PluginViewNone and PluginPackageNone to the last Plugin API (1.44 KB, patch)
2010-04-13 06:55 PDT, Leandro Pereira
no flags
Update PluginViewNone and PluginPackageNone to the last Plugin API (1.50 KB, patch)
2010-04-14 07:33 PDT, Leandro Pereira
no flags
Update PluginViewNone and PluginPackageNone to the last Plugin API (1.63 KB, patch)
2010-04-15 13:43 PDT, Leandro Pereira
no flags
WebKit Review Bot
Comment 1 2010-04-12 17:22:52 PDT
Gustavo Sverzut Barbieri
Comment 2 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).
Leandro Pereira
Comment 3 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.
Dave Hyatt
Comment 4 2010-04-13 11:55:48 PDT
Comment on attachment 53241 [details] Update PluginViewNone and PluginPackageNone to the last Plugin API r=me
WebKit Commit Bot
Comment 5 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).
Eric Seidel (no email)
Comment 6 2010-04-13 13:21:02 PDT
I'm not sure what went wrong for the commit-queue, I'll investigate.
Eric Seidel (no email)
Comment 7 2010-04-13 13:28:55 PDT
Comment on attachment 53241 [details] Update PluginViewNone and PluginPackageNone to the last Plugin API Trying again?
WebKit Commit Bot
Comment 8 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).
Eric Seidel (no email)
Comment 9 2010-04-13 13:44:54 PDT
I'll figure out what's going wrong and fix it this afternoon. Sorry again!
Eric Seidel (no email)
Comment 10 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.
Chris Jerdonek
Comment 11 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 )
Leandro Pereira
Comment 12 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.
Eric Seidel (no email)
Comment 13 2010-04-14 09:07:34 PDT
Comment on attachment 53330 [details] Update PluginViewNone and PluginPackageNone to the last Plugin API OK.
WebKit Commit Bot
Comment 14 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
Eric Seidel (no email)
Comment 15 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
Joseph Pecoraro
Comment 16 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.
Leandro Pereira
Comment 17 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.
Eric Seidel (no email)
Comment 18 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.
Eric Seidel (no email)
Comment 19 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.
WebKit Commit Bot
Comment 20 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>
WebKit Commit Bot
Comment 21 2010-04-19 23:30:18 PDT
All reviewed patches have been landed. Closing bug.
Gustavo Sverzut Barbieri
Comment 22 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.
Adam Roben (:aroben)
Comment 23 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
Gustavo Sverzut Barbieri
Comment 24 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.
Note You need to log in before you can comment on or make changes to this bug.