Bug 184591 – Split WindowProxy handling out of ScriptController and into a new class owned by AbstractFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Split WindowProxy handling out of ScriptController and into a new class owned by AbstractFrame. RemoteFrames do not need a ScriptController but do need to maintain WindowProxies.

[Chris Dumez]

Created attachment 337903 [details]
Patch

[Chris Dumez]

Created attachment 337905 [details]
Patch

[Chris Dumez]

Created attachment 337909 [details]
Patch

[Sam Weinig]

Comment on attachment 337909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337909&action=review

> Source/WebCore/bindings/js/DOMWrapperWorld.h:64
> +    HashSet<WindowProxyController*> m_controllersWithWindowProxies;

I think this can just be called m_windowProxyControllers.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:270
> -    return frame.script().windowProxy(world)->window();
> +    return frame.script().globalObject(world);

Why globalObject() and not window() here?

> Source/WebCore/bindings/js/JSDOMWindowProxy.cpp:127
>  JSDOMWindowProxy* toJSDOMWindowProxy(Frame& frame, DOMWrapperWorld& world)
>  {
> -    return frame.script().windowProxy(world);
> +    return &frame.windowProxyController().windowProxy(world);
>  }

Can we switch this function to return a reference since this will never return null?  Or,

> Source/WebCore/bindings/js/WindowProxyController.h:41
> +    Vector<JSC::Strong<JSDOMWindowProxy>> windowProxiesAsVector() const;

Do all the places that use windowProxiesAsVector() really need to copy to a Vector?

[Chris Dumez]

Comment on attachment 337909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337909&action=review

>> Source/WebCore/bindings/js/DOMWrapperWorld.h:64
>> +    HashSet<WindowProxyController*> m_controllersWithWindowProxies;
> 
> I think this can just be called m_windowProxyControllers.

Ok.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:270
>> +    return frame.script().globalObject(world);
> 
> Why globalObject() and not window() here?

This is identical but shorter since ScriptController::globalObject() is implemented as:
JSDOMWindow* globalObject(DOMWrapperWorld& world)
{
    return windowProxyController().windowProxy(world).window();
}

Once benefit also is that in the next patch, JSDOMWindowProxy::window() will start returning a more generic type, which ScriptController::globalObject() can keep returning a JSDOMWindow* since it is associated with a (local) Frame.

>> Source/WebCore/bindings/js/JSDOMWindowProxy.cpp:127
>>  }
> 
> Can we switch this function to return a reference since this will never return null?  Or,

I will look into it, I was trying to limit patch size.

>> Source/WebCore/bindings/js/WindowProxyController.h:41
>> +    Vector<JSC::Strong<JSDOMWindowProxy>> windowProxiesAsVector() const;
> 
> Do all the places that use windowProxiesAsVector() really need to copy to a Vector?

Some of them definitely do and this maintain pre-existing behavior. I am trying to reduce the risk of regression from my refactoring by maintaining pre-existing behavior as much as possible.

[Chris Dumez]

Created attachment 337915 [details]
Patch

[Chris Dumez]

Created attachment 337916 [details]
Patch

[Sam Weinig]

Comment on attachment 337916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337916&action=review

> Source/WebCore/bindings/js/WindowProxyController.h:41
> +    Vector<JSC::Strong<JSDOMWindowProxy>> windowProxiesAsVector() const;

I kind of feel this function should have the word "copy" somewhere in its name, but I can't think of one that isn't overly verbose.

> Source/WebCore/bindings/js/WindowProxyController.h:43
> +    ProxyMap releaseWindowProxies() { return std::exchange(m_windowProxies, ProxyMap()); }

Does anyone call this?

> Source/WebCore/bindings/js/WindowProxyController.h:44
> +    void setWindowProxies(ProxyMap&& windowProxies) { m_windowProxies = WTFMove(windowProxies); }

Does anyone call this?

[Chris Dumez]

Comment on attachment 337916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=337916&action=review

>> Source/WebCore/bindings/js/WindowProxyController.h:41
>> +    Vector<JSC::Strong<JSDOMWindowProxy>> windowProxiesAsVector() const;
> 
> I kind of feel this function should have the word "copy" somewhere in its name, but I can't think of one that isn't overly verbose.

copyWindowProxies() ?

>> Source/WebCore/bindings/js/WindowProxyController.h:43
>> +    ProxyMap releaseWindowProxies() { return std::exchange(m_windowProxies, ProxyMap()); }
> 
> Does anyone call this?

The next patch.

>> Source/WebCore/bindings/js/WindowProxyController.h:44
>> +    void setWindowProxies(ProxyMap&& windowProxies) { m_windowProxies = WTFMove(windowProxies); }
> 
> Does anyone call this?

The next patch.

[WebKit Commit Bot]

Comment on attachment 337916 [details]
Patch

Clearing flags on attachment: 337916

Committed r230643: <https://trac.webkit.org/changeset/230643>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39444728>