[webkit-reviews] review granted: [Bug 59134] WebKit2: Support docked mode for Web Inspector : [Attachment 90754] [PATCH] Win Fix v4

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted 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 90754: [PATCH] Win Fix v4
https://bugs.webkit.org/attachment.cgi?id=90754&action=review

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

> Source/WebKit2/ChangeLog:44
> +	   (WebKit::WebInspectorFrontendClient::closeWindow): Add a call to the
InspectorController to disconnect the inspector frontend
> +	       from the inspector backend. Without this line, when we close the
attached inspector, it won't re-open, because the
> +	       inspector controller still thinks there is a frontend.

Might be worth mentioning that this matches WebKit1. Maybe someday WebCore
should do this automatically.

> Source/WebKit2/ChangeLog:48
> +	   (WebKit::DrawingAreaImpl::sendUpdateBackingStoreState): Add an early
return if the WebPageProxy's viewSize is empty.

It would be good to expand on this comment a little bit to explain why this is
a good and OK thing to do.

> Source/WebKit2/UIProcess/WebPageProxy.h:496
> +    PageClient* pageClient() const { return m_pageClient; }
> +

It would be better to add a nativeWindow function that just calls through to
the PageClient rather than exposing the client itself. (That would match
WebPage::viewSize, for example.)