Need DOMWindow mechanism to supplement UserScripts for page cache notifications. --- Clients of the user script mechanism often also inject native hooks into the javascript context for those user scripts to interact with. When a page is suspended for the page cache and then restored from the page cache, those injected hooks are cleared, leading to many types of bugs with the user scripts. Such clients need to have notifications for when the global object (the DOMWindow) is suspended for the page cache, resumed from the page cache, and destroyed - whether or not in the page cache. DOMWindowProperty gives us a fantastic hook for this, and we create a "DOMWindowExtension" for each user script we inject into a world to uniquely track the lifetime of each world in each DOMWindow. The mechanism has room to be extended in the future to provide more functionality than the "unique-identifier plus notifications" included in this first pass. WebCore implementation, WebKit2 API, and WebKit2 API Test forthcoming. In radar as <rdar://problem/10120155>
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?