[webkit-reviews] review denied: [Bug 14272] Cannot change the height of the Web Inspector when docked on Windows : [Attachment 32889] Fixed Reattach Bug - No variable Caching

[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 32889: Fixed Reattach Bug - No variable Caching
https://bugs.webkit.org/attachment.cgi?id=32889&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +long InspectorController::constrainedAttachedWindowHeight(long
preferredHeight, long totalWindowHeight)
> +{
> +    if (preferredHeight == 0) {

We normally write this as if (!preferredHeight).

> @@ -323,7 +320,8 @@ void WebInspectorClient::updateWindowTit
>  
>      if (_shouldAttach) {
>	   WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView];

> -
> +	   NSRect frameViewRect = [frameView frame];

I don't think this local variable is really useful.

> @@ -331,8 +329,8 @@ void WebInspectorClient::updateWindowTit
>	   [frameView setAutoresizingMask:(NSViewWidthSizable |
NSViewHeightSizable | NSViewMinYMargin)];
>  
>	   _attachedToInspectedWebView = YES;
> -
> -	   [self setAttachedWindowHeight:[[NSUserDefaults standardUserDefaults]
integerForKey:WebKitInspectorAttachedViewHeightKey]];
> +	   
> +	   [self setAttachedWindowHeight:[_inspectedWebView
page]->inspectorController()->constrainedAttachedWindowHeight(0,
NSHeight(frameViewRect))];

Why do both InspectorClient implementations have to call
setAttachedWindowHeight themselves? Can't the InspectorController make this
call for us (passing in the height of the inspected page from before
InspectorClient::attachWindow was called)? If InspectorController did it for
us, we could get rid of the constrainedAttachedWindowHeight function.

> @@ -240,6 +238,16 @@ void WebInspectorClient::attachWindow()
>  
>      closeWindowWithoutNotifications();
>      showWindowWithoutNotifications();
> +
> +    HWND hostWindow;
> +    if (SUCCEEDED(m_inspectedWebView->hostWindow((OLE_HANDLE*)&hostWindow)))


Please use reinterpret_cast instead of a C-style cast.

> +	   SendMessage(hostWindow, WM_SIZE, 0, 0);     // Make sure we have the
correct size

What happens if you omit this SendMessage call?

> +    RECT hostWindowRect;
> +    GetClientRect(hostWindow, &hostWindowRect);
> +
> +    // This re-constrains our attached height, in case while the inspector
was detached, they resized the inspected window,
> +    // and drawing it at the size it used to be could lead to the inspector
taking up the whole window or more

Please put a period at the end of this comment.

>  void WebInspectorClient::setAttachedWindowHeight(unsigned height)
>  {
> -    // FIXME: implement this.
> +    if (!m_attached)
> +	   return;
> +
> +    HWND hostWindow;
> +    if
(!SUCCEEDED(m_inspectedWebView->hostWindow((OLE_HANDLE*)&hostWindow)))

You should use reinterpret_cast here, and FAILED() instead of !SUCCEEDED().

> +	   return;
> +
> +    RECT hostWindowRect;
> +    GetClientRect(hostWindow, &hostWindowRect);
> +
> +    RECT inspectedRect;
> +    GetClientRect(m_inspectedWebViewHwnd, &inspectedRect);
> +
> +    int totalHeight = hostWindowRect.bottom - hostWindowRect.top;
> +    int webViewWidth = inspectedRect.right - inspectedRect.left;
> +
> +    SetWindowPos(m_webViewHwnd, 0, 0, totalHeight - height, webViewWidth,
height, SWP_NOZORDER);
> +    SetWindowPos(m_inspectedWebViewHwnd, 0, 0, 0, webViewWidth, totalHeight,
SWP_NOZORDER);

This doesn't seem right. Shouldn't the height of m_inspectedWebViewHwnd be set
to totalHeight - height?