Bug 45217 – Indicate which one of the ScriptWorlds for a Frame the Window Object has been cleared for.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Jessie Berlin]

WKBundlePageLoaderClient's WKBundlePageDidClearWindowObjectForFrameCallback should be a WKBundlePageDidClearWindowObjectForFrameInScriptWorldCallback, since there may be multiple ScriptWorlds for one Frame.

[Jessie Berlin]

Created attachment 66723 [details]
didClearWindowObjectForFrame -> didClearWindowObjectForFrameInScriptWorld

[WebKit Review Bot]

Attachment 66723 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h:81:  Extra space between WKBundlePageDidClearWindowObjectForFrameInScriptWorldCallback and didClearWindowObjectForFrameInScriptWorld  [whitespace/declaration] [3]
WebKit2/WebProcess/InjectedBundle/InjectedBundlePageLoaderClient.h:34:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jessie Berlin]

Created attachment 66726 [details]
didClearWindowObjectForFrame -> didClearWindowObjectForFrameInScriptWorld (Take 2 - Style Errors Fixed)

Fixed style errors.

[Darin Adler]

Comment on attachment 66726 [details]
didClearWindowObjectForFrame -> didClearWindowObjectForFrameInScriptWorld (Take 2 - Style Errors Fixed)

I don’t think you need to add “InScriptWord” to the name of the function. That’s the normal naming for Objective-C, but here in C++ I think it is not much clearer and makes the function name longer than necessary. I suppose it’s nice to clarify that this has to be called separately for each world, but I also think the arguments makes that clear.

It does seem unfortunate that we will have to create a script world just to pass to the caller.

> -    WKBundlePageDidClearWindowObjectForFrameCallback                    didClearWindowObjectForFrame;
> +    WKBundlePageDidClearWindowObjectForFrameInScriptWorldCallback didClearWindowObjectForFrameInScriptWorld;

It looks like this is no longer aligned. This is why I normally request that we not line things up like this. Worth talking to Sam about eliminating the vertical alignment, but for now maybe you should add spaces to keep things lined up.

r=me

[Jessie Berlin]

(In reply to comment #4)
> (From update of attachment 66726 [details])
> I don’t think you need to add “InScriptWord” to the name of the function. That’s the normal naming for Objective-C, but here in C++ I think it is not much clearer and makes the function name longer than necessary. I suppose it’s nice to clarify that this has to be called separately for each world, but I also think the arguments makes that clear.
> 

I originally added "InScriptWorld" to match IWebFrameLoadDelegatePrivate2's didClearWindowObjectForFrameInScriptWorld. However, your argument makes more sense, so I just removed the extra "InScriptWorld"s.

> It does seem unfortunate that we will have to create a script world just to pass to the caller.
> 

I am not sure how else we would go about getting the script world.

> > -    WKBundlePageDidClearWindowObjectForFrameCallback                    didClearWindowObjectForFrame;
> > +    WKBundlePageDidClearWindowObjectForFrameInScriptWorldCallback didClearWindowObjectForFrameInScriptWorld;
> 
> It looks like this is no longer aligned. This is why I normally request that we not line things up like this. Worth talking to Sam about eliminating the vertical alignment, but for now maybe you should add spaces to keep things lined up.
> 

In my original patch it was aligned, but then the check-webkit-style script issued a warning about too many spaces. I will align it again.

> r=me

Thanks!

[Jessie Berlin]

Comment on attachment 66726 [details]
didClearWindowObjectForFrame -> didClearWindowObjectForFrameInScriptWorld (Take 2 - Style Errors Fixed)

Committed in r66894:
http://trac.webkit.org/changeset/66894