[webkit-reviews] review denied: [Bug 59134] WebKit2: Support docked mode for Web Inspector : [Attachment 90728] [PATCH] Win fix v3

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has denied Brian Weinstein
<bweinstein at apple.com>'s request for review:
Bug 59134: WebKit2: Support docked mode for Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=59134

Attachment 90728: [PATCH] Win fix v3
https://bugs.webkit.org/attachment.cgi?id=90728&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=90728&action=review

> Source/WebKit2/ChangeLog:43
> +	   * WebProcess/WebCoreSupport/WebInspectorFrontendClient.cpp:
> +	   (WebKit::WebInspectorFrontendClient::attachWindow): Call
WebInspector::attach.
> +	   (WebKit::WebInspectorFrontendClient::detachWindow): Call
WebInspector::detach.
> +	   (WebKit::WebInspectorFrontendClient::setAttachedWindowHeight): Call
WebInspector::setAttachedWindowHeight.

No comment on closeWindow!?

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:216
>  void WebInspectorProxy::platformClose()
>  {
> +    if (m_isAttached) {
> +	   // Detach here so we only need to have one code path that is
responsible for cleaning up the inspector
> +	   // state.
> +	   detach();
> +    }
> +
> +    ASSERT(!m_isAttached);

Sure seems like this code should be in the cross-platform file.

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:269
> +    // We only want to show the inspector window if we're visible, if not,
we are in the process
> +    // of being destroyed, and we don't want this window to flash.
> +    if (m_isVisible)
> +	   ::ShowWindow(m_inspectorWindow, SW_SHOW);

I'm not sure the comment is needed here. Not showing the window when we're not
visible makes sense even if the "being destroyed" case isn't considered.

> Source/WebKit2/WebProcess/WebPage/DrawingAreaImpl.cpp:551
>  {
>      ASSERT(!m_isPaintingSuspended);
>      ASSERT(!m_layerTreeHost || m_layerTreeHost->participatesInDisplay());
> -    ASSERT(!m_webPage->size().isEmpty());
> +
> +    if (m_webPage->size().isEmpty())
> +	   return;

We should be bailing out at an earlier point than this.