| Summary: | Out-of-process plug-ins should support asynchronous initialization. | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||||
| Component: | WebKit2 | Assignee: | Brady Eidson <beidson> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | andersca, gustavo, philn, sam, webkit.review.bot, xan.lopez | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | All | ||||||||||||||||||
| OS: | All | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Brady Eidson
2012-08-01 16:00:18 PDT
Created attachment 155921 [details]
Part 1 - Add two related preferences to the API
Part 1 landed in http://trac.webkit.org/changeset/124393 I will be prepping the remainder into either 1 or 2 patches tomorrow. Created attachment 156446 [details]
Part 2 - More preference API and groundwork for regression testing
http://trac.webkit.org/changeset/124649 Final patch will (unfortunately) be much larger. Comment on attachment 156446 [details] Part 2 - More preference API and groundwork for regression testing Attachment 156446 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13437016 (In reply to comment #5) > (From update of attachment 156446 [details]) > Attachment 156446 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/13437016 This failure is because the Windows builder didn't pick up the WebPreferences changes and regenerate everything. I don't know what to do about that. (In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 156446 [details] [details]) > > Attachment 156446 [details] [details] did not pass win-ews (win): > > Output: http://queues.webkit.org/results/13437016 > > This failure is because the Windows builder didn't pick up the WebPreferences changes and regenerate everything. I don't know what to do about that. Actually, wait, I might have failed on a much more epic level - Might not have gotten the new pref in my patch... =/ Dealing with git right now to try and fix this. http://trac.webkit.org/changeset/124652 Sorry about that! Comment on attachment 156446 [details] Part 2 - More preference API and groundwork for regression testing Attachment 156446 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13425769 (In reply to comment #9) > (From update of attachment 156446 [details]) > Attachment 156446 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/13425769 Will be fixed by http://trac.webkit.org/changeset/124652 Created attachment 156485 [details]
Part 3 - Add the feature (with some tests)
Attachment 156485 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:64: The parameter name "pluginProcessConnectionManager" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 156485 [details] Part 3 - Add the feature (with some tests) Attachment 156485 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13424873 (In reply to comment #12) > Attachment 156485 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1 > Source/WebKit2/WebProcess/Plugins/PluginProcessConnection.h:64: The parameter name "pluginProcessConnectionManager" adds no information, so it should be removed. [readability/parameter_name] [5] > Total errors found: 1 in 38 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. Fixed locally, won't bother uploading a new patch for this. (In reply to comment #13) > (From update of attachment 156485 [details]) > Attachment 156485 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/13424873 Last 500 characters of output: xy::DidCreateWebProcessConnection&) ./DerivedSources/WebKit2/PluginProcessProxyMessages.h:55:8: note: no known conversion for argument 1 from 'CoreIPC::Attachment' to 'const Messages::PluginProcessProxy::DidCreateWebProcessConnection&' This appears to be a compatibility problem with the compiler GTK is using. (In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 156485 [details] [details]) > > Attachment 156485 [details] [details] did not pass gtk-ews (gtk): > > Output: http://queues.webkit.org/results/13424873 > > Last 500 characters of output: > xy::DidCreateWebProcessConnection&) > ./DerivedSources/WebKit2/PluginProcessProxyMessages.h:55:8: note: no known conversion for argument 1 from 'CoreIPC::Attachment' to 'const Messages::PluginProcessProxy::DidCreateWebProcessConnection&' > > This appears to be a compatibility problem with the compiler GTK is using. This is a generated file. And looking at the PluginProcessProxyMessages.h generated locally I'm not sure exactly why it's broken on GTK - Maybe their generated PluginProcessProxyMessages.h is different and broken? Comment on attachment 156485 [details] Part 3 - Add the feature (with some tests) Attachment 156485 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13421996 Comment on attachment 156485 [details] Part 3 - Add the feature (with some tests) Attachment 156485 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13425852 (In reply to comment #17) > (From update of attachment 156485 [details]) > Attachment 156485 [details] did not pass qt-wk2-ews (qt): > Output: http://queues.webkit.org/results/13421996 (In reply to comment #18) > (From update of attachment 156485 [details]) > Attachment 156485 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/13425852 Okay, clearly something quite real is happening. I did build successfully on Mac before uploading. Maybe I'll need to try a clean build to sort it out... Reviewing the patch is still an admirable task in the meantime. :) (In reply to comment #19) > (In reply to comment #17) > > (From update of attachment 156485 [details] [details]) > > Attachment 156485 [details] [details] did not pass qt-wk2-ews (qt): > > Output: http://queues.webkit.org/results/13421996 > > (In reply to comment #18) > > (From update of attachment 156485 [details] [details]) > > Attachment 156485 [details] [details] did not pass mac-ews (mac): > > Output: http://queues.webkit.org/results/13425852 > > Okay, clearly something quite real is happening. > > I did build successfully on Mac before uploading. Maybe I'll need to try a clean build to sort it out... > > Reviewing the patch is still an admirable task in the meantime. :) I did a 100% clean build on Mac with the patch as is... *sigh* Digging in to the specific failures. I see, probably compiler differences with OwnPtr = 0. Something my compiler supports but older ones don't, I guess. Will fix. Created attachment 156721 [details]
Part 3 - v2 - Add the feature (with some tests)
Comment on attachment 156721 [details] Part 3 - v2 - Add the feature (with some tests) Attachment 156721 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13445358 (In reply to comment #23) > (From update of attachment 156721 [details]) > Attachment 156721 [details] did not pass qt-wk2-ews (qt): > Output: http://queues.webkit.org/results/13445358 AHA, realized what the deal is. Didn't update the non-Mac block here. New patch forthcoming. Created attachment 156734 [details]
Part 3 - v3 - Add the feature (with some tests) (and non-Mac build fixes)
Created attachment 156781 [details]
Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes)
GTK doesn't have sleep() declared... trying a direct include of <unistd.h> as a fix Attachment 156781 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebKit2/PluginProcess/WebProcessConnection.cpp:38: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 38 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 156781 [details] Part 3 - v4 - Add the feature (with some tests) (and Qt build fixes) (and possibly GTK build fixes) > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:93 > +static HashSet<uint64_t>& asynchronousInstanceIDsToIgnore() > +{ > + DEFINE_STATIC_LOCAL(HashSet<uint64_t>, instances, ()); > + return instances; > +} This should be a member of WebProcessConnection. pluginInstanceIDs are not unique across multiple WebProcesses. > Source/WebKit2/PluginProcess/WebProcessConnection.cpp:311 > + if (creationParameters.artificialPluginInitializationDelayEnabled) { > + static unsigned int artificialPluginInitializationDelay = 5; > + sleep(artificialPluginInitializationDelay); > + } I don't like that this test code is here, but I guess it's OK. The constant should not be static, and you can just use unsigned instead of unsigned int. > Source/WebKit2/WebProcess/Plugins/Plugin.h:88 > + virtual bool isInitializingAsynchronously() const = 0; How about isBeingAsynchronouslyInitialized() instead? Also, add a comment indicating what this function does. > Source/WebKit2/WebProcess/Plugins/PluginController.h:164 > + virtual void didInitializePlugin() = 0; > + virtual void didFailToInitializePlugin() = 0; Please add comments indicating what these functions are supposed to do. Created attachment 156785 [details]
Part 3 - v5 - Add the feature, address review comments, and maybe have 100% working builds
|