Bug 82664

Summary: Need DOMWindow mechanism to supplement UserScripts for page cache notifications
Product: WebKit Reporter: Brady Eidson <beidson>
Component: Page LoadingAssignee: 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 Flags
Patch v1 - Add the API and an API test
webkit-ews: commit-queue-
Patch v2 - Now with more build fixes!
buildbot: commit-queue-
Patch v3 - Now with even more project files!
none
Patch v4 - Explicit creation of DOMWindowExtensions
webkit-ews: commit-queue-
Patch v5 - More project file additions
sam: review-
Patch v6 - FrameLoaderClient method name changed, and WK2 client methods moved to BundlePageLoaderClient sam: review+

Description Brady Eidson 2012-03-29 14:57:06 PDT
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>
Comment 1 Brady Eidson 2012-03-29 16:25:34 PDT
Created attachment 134689 [details]
Patch v1 - Add the API and an API test
Comment 2 WebKit Review Bot 2012-03-29 16:28:28 PDT
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 3 Build Bot 2012-03-29 16:52:45 PDT
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
Comment 4 Brady Eidson 2012-03-29 16:56:56 PDT
(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 5 Early Warning System Bot 2012-03-29 17:09:24 PDT
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
Comment 6 Brady Eidson 2012-03-29 17:15:25 PDT
Created attachment 134699 [details]
Patch v2 - Now with more build fixes!
Comment 7 WebKit Review Bot 2012-03-29 17:19:40 PDT
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 8 Build Bot 2012-03-29 17:44:02 PDT
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 9 Sam Weinig 2012-03-29 18:09:09 PDT
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.
Comment 10 Brady Eidson 2012-03-29 18:11:39 PDT
(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.
Comment 11 Brady Eidson 2012-03-29 18:13:18 PDT
And - like a bonehead - forgot to add new files to the other WK2 builds.  Will do so in the morning.
Comment 12 Early Warning System Bot 2012-03-29 18:26:10 PDT
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 13 Collabora GTK+ EWS bot 2012-03-29 21:35:03 PDT
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
Comment 14 Brady Eidson 2012-03-30 09:27:49 PDT
Created attachment 134827 [details]
Patch v3 - Now with even more project files!
Comment 15 Jessie Berlin 2012-04-02 09:27:27 PDT
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?
Comment 16 Brady Eidson 2012-04-02 09:35:29 PDT
(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.
Comment 17 Dimitri Glazkov (Google) 2012-04-02 09:37:43 PDT
cc'ing Adam and Haraken, since they are frolicking in the same meadow (or nearby).
Comment 18 Brady Eidson 2012-04-02 10:30:25 PDT
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
Comment 19 Brady Eidson 2012-04-09 18:06:47 PDT
Created attachment 136358 [details]
Patch v4 - Explicit creation of DOMWindowExtensions
Comment 20 WebKit Review Bot 2012-04-09 18:12:05 PDT
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 21 Early Warning System Bot 2012-04-09 19:01:23 PDT
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 22 Build Bot 2012-04-09 20:54:13 PDT
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
Comment 23 Brady Eidson 2012-04-09 22:42:35 PDT
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...
Comment 24 Brady Eidson 2012-04-10 09:13:55 PDT
Created attachment 136466 [details]
Patch v5 - More project file additions
Comment 25 Sam Weinig 2012-04-15 16:16:42 PDT
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.
Comment 26 Brady Eidson 2012-04-15 17:06:12 PDT
(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.
Comment 27 Brady Eidson 2012-04-24 10:02:05 PDT
Created attachment 138587 [details]
Patch v6 - FrameLoaderClient method name changed, and WK2 client methods moved to BundlePageLoaderClient
Comment 28 Brady Eidson 2012-04-24 11:14:24 PDT
Landed in http://trac.webkit.org/changeset/115083
Comment 29 Csaba Osztrogonác 2012-04-24 12:56:54 PDT
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’
Comment 30 Jesus Sanchez-Palencia 2012-04-24 12:59:55 PDT
(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?
Comment 31 Csaba Osztrogonác 2012-04-24 13:04:13 PDT
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.
Comment 32 Brady Eidson 2012-04-24 13:05:44 PDT
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...
Comment 33 Brady Eidson 2012-04-24 13:06:54 PDT
(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.
Comment 34 Csaba Osztrogonác 2012-04-24 13:15:18 PDT
(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?