Summary: | Need DOMWindow mechanism to supplement UserScripts for page cache notifications | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brady Eidson <beidson> | ||||||||||||||
Component: | Page Loading | Assignee: | Brady Eidson <beidson> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | abarth, darin, dglazkov, gustavo.noronha, gustavo, haraken, japhet, jesus, menard, ossy, rakuco, webkit.review.bot, xan.lopez, zoltan | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Brady Eidson
2012-03-29 14:57:06 PDT
Created attachment 134689 [details]
Patch v1 - Add the API and an API test
Attachment 134689 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleClient.h:49: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/page/Frame.cpp:42: Alphabetical sorting problem. [build/include_order] [4]
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4]
Tools/TestWebKitAPI/InjectedBundleController.h:61: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:95: One space before end of line comments [whitespace/comments] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:56: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 15 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 134689 [details] Patch v1 - Add the API and an API test Attachment 134689 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12243005 (In reply to comment #3) > (From update of attachment 134689 [details]) > Attachment 134689 [details] did not pass win-ews (win): > Output: http://queues.webkit.org/results/12243005 I completely overlooked other WebCore builds since this is really to support a Mac WK2 feature. My bad! New patch is on its way. Comment on attachment 134689 [details] Patch v1 - Add the API and an API test Attachment 134689 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12207029 Created attachment 134699 [details]
Patch v2 - Now with more build fixes!
Attachment 134699 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleClient.h:49: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleDOMWindowExtension.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4]
Tools/WebKitTestRunner/InjectedBundle/InjectedBundle.cpp:95: One space before end of line comments [whitespace/comments] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:61: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:62: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:63: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/InjectedBundleController.h:64: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/ChangeLog:50: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:51: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:52: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:53: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:54: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:56: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59: The parameter name "bundle" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:59: The parameter name "extension" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 19 in 41 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 134699 [details] Patch v2 - Now with more build fixes! Attachment 134699 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12239015 Comment on attachment 134699 [details]
Patch v2 - Now with more build fixes!
I am not sold on this API, specially the one extension per world. I am going to take some time to look it over.
(In reply to comment #9) > (From update of attachment 134699 [details]) > I am not sold on this API, specially the one extension per world. I am going to take some time to look it over. Would love to talk with you about your concerns in person. It went through a few iterations here before we settled on something that fulfilled the use cases, followed the user script model, and was efficient. And - like a bonehead - forgot to add new files to the other WK2 builds. Will do so in the morning. Comment on attachment 134699 [details] Patch v2 - Now with more build fixes! Attachment 134699 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12262011 Comment on attachment 134699 [details] Patch v2 - Now with more build fixes! Attachment 134699 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12253039 Created attachment 134827 [details]
Patch v3 - Now with even more project files!
Comment on attachment 134827 [details] Patch v3 - Now with even more project files! View in context: https://bugs.webkit.org/attachment.cgi?id=134827&action=review Holding off on reviewing it because Sam Weinig wants to talk further about the design. > Tools/MiniBrowser/mac/WebBundle/WebBundleMain.m:105 > + 0 // didDestroyDOMWindowExtension Shouldn't you be bumping the version of the client here as well? > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic.cpp:2 > + * Copyright (C) 2010 Apple Inc. All rights reserved. I think you meant 2012 here. > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:2 > + * Copyright (C) 2010 Apple Inc. All rights reserved. I think you meant 2012. > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:78 > + ((DOMWindowExtensionBasic*)clientInfo)->frameLoadFinished(frame); Can this be a reinterpret cast instead? (In reply to comment #15) > (From update of attachment 134827 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134827&action=review > > Holding off on reviewing it because Sam Weinig wants to talk further about the design. > > > Tools/MiniBrowser/mac/WebBundle/WebBundleMain.m:105 > > + 0 // didDestroyDOMWindowExtension > > Shouldn't you be bumping the version of the client here as well? It's already using kWKBundleClientCurrentVersion. That's why it had to be updated to keep its build working. > > > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic.cpp:2 > > + * Copyright (C) 2010 Apple Inc. All rights reserved. > > I think you meant 2012 here. > > > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:2 > > + * Copyright (C) 2010 Apple Inc. All rights reserved. > > I think you meant 2012. Yuppers. > > > Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:78 > > + ((DOMWindowExtensionBasic*)clientInfo)->frameLoadFinished(frame); > > Can this be a reinterpret cast instead? There certainly doesn't seem to be an established style throughout TestWebKitAPI. It sure can be. cc'ing Adam and Haraken, since they are frolicking in the same meadow (or nearby). Comment on attachment 134827 [details]
Patch v3 - Now with even more project files!
Chatted with Sam and Jessie, going a direction slightly tweaked from this API. New patch in the works
Created attachment 136358 [details]
Patch v4 - Explicit creation of DOMWindowExtensions
Attachment 136358 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageGroupClient.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:41: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:76: The parameter name "frame" adds no information, so it should be removed. [readability/parameter_name] [5]
Tools/TestWebKitAPI/Tests/WebKit2/DOMWindowExtensionBasic_Bundle.cpp:146: Should have a space between // and comment [whitespace/comments] [4]
Total errors found: 4 in 42 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 136358 [details] Patch v4 - Explicit creation of DOMWindowExtensions Attachment 136358 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12371552 Comment on attachment 136358 [details] Patch v4 - Explicit creation of DOMWindowExtensions Attachment 136358 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12373508 Yep definitely need to add one more new file to to the other ports' WK2. Will do so in the A.M. Reviewing the rest of the patch in the meantime is *not* wasted effort... Created attachment 136466 [details]
Patch v5 - More project file additions
Comment on attachment 136466 [details] Patch v5 - More project file additions View in context: https://bugs.webkit.org/attachment.cgi?id=136466&action=review Looking better, still needs some work. > Source/WebCore/loader/FrameLoaderClient.h:336 > + virtual void dispatchCanCreateGlobalObject(DOMWrapperWorld*) { } I don't really understand the naming here. The client can't really create a global object can it? > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageGroup.h:50 > + WKBundlePageGroupDidCreateGlobalObjectForFrameCallback didCreateGlobalObjectForFrame; > + WKBundlePageGroupWillDisconnectDOMWindowExtensionFromGlobalObjectCallback willDisconnectDOMWindowExtensionFromGlobalObject; > + WKBundlePageGroupDidReconnectDOMWindowExtensionToGlobalObjectCallback didReconnectDOMWindowExtensionToGlobalObject; > + WKBundlePageGroupWillDestroyGlobalObjectForDOMWindowExtensionCallback willDestroyGlobalObjectForDOMWindowExtension; > +}; I think these should go on the BundlePageLoadClient as discussed. (In reply to comment #25) > (From update of attachment 136466 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=136466&action=review > > Looking better, still needs some work. > > > Source/WebCore/loader/FrameLoaderClient.h:336 > > + virtual void dispatchCanCreateGlobalObject(DOMWrapperWorld*) { } > > I don't really understand the naming here. The client can't really create a global object can it? Actually it can. The global object is created on demand, and the client can create it by demanding it. In this case the client is WebKit2 which translates it to "didCreateGlobalObject" after having created it. Perhaps "isReadyToAccessGlobalObject"? Some other variant? > > > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePageGroup.h:50 > > + WKBundlePageGroupDidCreateGlobalObjectForFrameCallback didCreateGlobalObjectForFrame; > > + WKBundlePageGroupWillDisconnectDOMWindowExtensionFromGlobalObjectCallback willDisconnectDOMWindowExtensionFromGlobalObject; > > + WKBundlePageGroupDidReconnectDOMWindowExtensionToGlobalObjectCallback didReconnectDOMWindowExtensionToGlobalObject; > > + WKBundlePageGroupWillDestroyGlobalObjectForDOMWindowExtensionCallback willDestroyGlobalObjectForDOMWindowExtension; > > +}; > > I think these should go on the BundlePageLoadClient as discussed. Indeed. Created attachment 138587 [details]
Patch v6 - FrameLoaderClient method name changed, and WK2 client methods moved to BundlePageLoaderClient
Landed in http://trac.webkit.org/changeset/115083 Reopen, because it broke the Qt-WK2 build: cc1plus: warnings being treated as errors /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp: In constructor ‘WebKit::QtBuiltinBundlePage::QtBuiltinBundlePage(WebKit::QtBuiltinBundle*, const OpaqueWKBundlePage*)’: /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::didCreateGlobalObjectForFrame’ /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::willDisconnectDOMWindowExtensionFromGlobalObject’ /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::didReconnectDOMWindowExtensionToGlobalObject’ /home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/qt/QtBuiltinBundlePage.cpp:73: error: missing initializer for member ‘WKBundlePageLoaderClient::willDestroyGlobalObjectForDOMWindowExtension’ (In reply to comment #28) > Landed in http://trac.webkit.org/changeset/115083 As warned by qt-wk2 ews, this broke the Qt bot and it was kept like that for a few rounds of patches, not even mentioning the style issues also warned by the style ews... I have landed a fix for the Qt build issue in http://trac.webkit.org/changeset/115095 but it seems there is still some layout failures for us after this. Can you please have a look at it, Brady? Thanks for the buildfix Jeez. I think the svg fails are unrelated to this bug, http://trac.webkit.org/changeset/115085 is the culprit. Maybe we only need to update them. Apologies for the breakage. I - admittedly - didn't wait for ews to finish as the last version passed ews, yet I didn't take in to considersation that the one substantive change might break another build. The style errors are bogus as they don't account for well established style within TestWebKitAPI. I'll see what I can find about the qt layouttest failures... they're almost certainly inconsequential based on the change... (In reply to comment #31) > Thanks for the buildfix Jeez. > > I think the svg fails are unrelated to this bug, http://trac.webkit.org/changeset/115085 is the culprit. Maybe we only need to update them. Yup, sure seems like it. I had a fix ready to check in but Jeez beat me to it. Sorry for the noise, guys. (In reply to comment #32) > Apologies for the breakage. I - admittedly - didn't wait for ews to finish as the last version passed ews, yet I didn't take in to considersation that the one substantive change might break another build. Not a big problem, the build was fixed quickly. And it show us that Qt EWS bots are too slow in rush hours, so we should setup more bots. > The style errors are bogus as they don't account for well established style within TestWebKitAPI. Is there a bug report to fix the style checker? |