[webkit-reviews] review denied: [Bug 185022] Use WindowProxy instead of DOMWindow in our IDL : [Attachment 338902] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Chris Dumez <cdumez at apple.com>'s request
for review:
Bug 185022: Use WindowProxy instead of DOMWindow in our IDL
https://bugs.webkit.org/show_bug.cgi?id=185022

Attachment 338902: Patch

https://bugs.webkit.org/attachment.cgi?id=338902&action=review




--- Comment #25 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 338902
  --> https://bugs.webkit.org/attachment.cgi?id=338902
Patch

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

> Source/WebCore/bindings/js/WindowProxy.cpp:-61
> -	       it->value->window()->setConsoleClient(nullptr);

What prompted this change?

> Source/WebCore/dom/Document.h:802
>      DOMWindow* domWindow() const { return m_domWindow.get(); }
>      // In DOM Level 2, the Document's DOMWindow is called the defaultView.
> -    DOMWindow* defaultView() const { return domWindow(); } 
> +    WEBCORE_EXPORT WindowProxy* defaultView() const;

In addition to defaultView() returning a WindowProxy*, I think there should be
a windowProxy() member function which does the same thing. defaultView() is a
terribly uninformatively named function, and only exists to appease the DOM.
Internally in WebCore, I think we should use this new windowProxy() member
function instead (much like we used to call the domWindow() instead).

> Source/WebCore/editing/AlternativeTextController.cpp:635
> -    Ref<TextEvent> event =
TextEvent::createForDictation(m_frame.document()->domWindow(), text,
dictationAlternatives);
> +    Ref<TextEvent> event =
TextEvent::createForDictation(m_frame.document()->defaultView(), text,
dictationAlternatives);

Seems like this could just call m_frame->windowProxy().  Also could probably
use auto.

> Source/WebCore/page/DragController.cpp:511
> +    auto event =
TextEvent::createForDrop(innerFrame->document()->defaultView(), text);

Could just call innerFrame->windowProxy().

> Source/WebCore/page/EventHandler.cpp:3805
> +    Ref<TextEvent> event =
TextEvent::create(m_frame.document()->defaultView(), text, inputType);

Could just call m_frame.windowProxy().	Could also use auto.

> Source/WebCore/page/Frame.cpp:215
> +    windowProxy().destroyAllJSWindowProxies();

This feels icky. It seems weird that it is called here, and then again in the
WindowProxy destructor. If we really can't figure out another way, please at
least add a comment explaining it.