Bug 184591

Created attachment 337903 [details] Patch

Created attachment 337905 [details] Patch

Created attachment 337909 [details] Patch

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?

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.

Created attachment 337915 [details] Patch

Created attachment 337916 [details] Patch

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?

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.

Comment on attachment 337916 [details] Patch Clearing flags on attachment: 337916 Committed r230643: <https://trac.webkit.org/changeset/230643>

All reviewed patches have been landed. Closing bug.

<rdar://problem/39444728>