[webkit-reviews] review denied: [Bug 14272] Cannot change the height of the Web Inspector when docked on Windows : [Attachment 32761] Take 3

[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 14272: Cannot change the height of the Web Inspector when docked on Windows
https://bugs.webkit.org/show_bug.cgi?id=14272

Attachment 32761: Take 3
https://bugs.webkit.org/attachment.cgi?id=32761&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
The improvements here look really good.

> +    virtual long inspectorPageHeight() = 0;

Do we really need the client to give us this information? I think
m_page->mainFrame()->view()->visibleHeight() would give us the same
information.

> +    virtual long inspectedPageHeight() = 0;

For the same reason, it seems like we don't need inspectedPageHeight(), either.
We can do the same thing as above, but use m_inspectedPage instead of m_page.

> +    static const float minimumAttachedHeight = 250.0;
> +    static const float maximumAttachedHeightRatio = 0.75;

I think you should move these up next to defaultAttachedHeight.

> +    long attachedHeight = round(max(minimumAttachedHeight, 
> +	   min(static_cast<float>(height), (m_client->inspectedPageHeight() +
m_client->inspectorPageHeight()) * maximumAttachedHeightRatio)));
> +
> +    // Save a preference for the user's preferred attached height and pass
it to the client
> +    setSetting(inspectorAttachedHeightName, Setting(attachedHeight));
> +    m_client->setInitialAttachedHeight(attachedHeight);
> +
> +    m_client->setAttachedWindowHeight(attachedHeight);

It seems bad that we have to call both of these functions one after the other.

I think that we can get rid of setInitialAttachedHeight completely. I think the
way this should work is:

1) Whenever the Inspector becomes attached (either because it is being shown
for the first time and is starting out attached, or because the user clicked
the button to attach it), the client should set the height of the Inspector to
the value stored in the settings (so the client would ask the
InspectorController for that height value)
2) Whenever the height of the attached Inspector changes, the
InspectorController saves the new height to settings and calls
InspectorClient::setAttachedWindowHeight, and the client obliges.

So there's no need for the whole "initial attached height" concept here. That
will let the clients get rid of their m_initialAttachedHeight members.

>  private:
>      void updateWindowTitle() const;
>  
> +    long m_initialAttachedHeight;

You still aren't initializing this in the constructor (in
WebInspectorClient.mm). But as I explained above, I think we can get rid of
this entirely.

>  void WebInspectorClient::setAttachedWindowHeight(unsigned height)
>  {
> -    // FIXME: implement this.
> +    if (!m_attached)
> +	   return;
> +
> +    RECT inspectedRect;
> +    GetClientRect(m_inspectedWebViewHwnd, &inspectedRect);
> +
> +    int webViewWidth = inspectedRect.right - inspectedRect.left;
> +    int totalHeight = inspectedPageHeight() + inspectorPageHeight();
> +
> +    SetWindowPos(m_webViewHwnd, 0, 0, totalHeight - height, webViewWidth,
height, SWP_NOZORDER);
> +    SetWindowPos(m_inspectedWebViewHwnd, 0, 0, 0, webViewWidth, totalHeight,
SWP_NOZORDER);
> +    
> +    RedrawWindow(m_webViewHwnd, 0, 0, RDW_INVALIDATE | RDW_ALLCHILDREN |
RDW_UPDATENOW); 
> +    RedrawWindow(m_inspectedWebViewHwnd, 0, 0, RDW_INVALIDATE |
RDW_ALLCHILDREN | RDW_UPDATENOW);
>  }

I guess you decided we don't need the WM_SETREDRAW stuff after all?

I think after making the changes I described above, we won't need any new
InspectorClient functions at all! That should make the patch a lot smaller and
simpler. Sorry my thinking on this wasn't clearer before.