[webkit-reviews] review denied: [Bug 14272] Cannot change the height of the Web Inspector when docked on Windows : [Attachment 32903] Resize Attached Inspector

[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 32903: Resize Attached Inspector
https://bugs.webkit.org/attachment.cgi?id=32903&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +    int inspectedHeight =
m_inspectedPage->mainFrame()->view()->visibleHeight();

There seems to be a mix of "long", "int", and "unsigned" when talking about
attached window heights. It would be good to use a single type as much as
possible.

"inspectedHeight" is a confusing name. We aren't inspecting the height, we're
inspecting the page. "inspectedPageHeight" would be better.

>      m_client->attachWindow();
> +    
> +    Setting attachedHeight = setting(inspectorAttachedHeightName);
> +    int preferredHeight = attachedHeight.type() == Setting::IntegerType ?
attachedHeight.integerValue() : defaultAttachedHeight;
> +   
m_client->setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeig
ht, inspectedHeight));

It would be good to add a comment here about how we have to reconstrain the
height based on the current height of the inspected page.

> +long InspectorController::constrainedAttachedWindowHeight(long
preferredHeight, long totalWindowHeight)

This can just be a static helper function. It doesn't need to be a member of
the InspectorController class.

> +    // Save a preference for the user's preferred attached height and pass
it to the client
> +    setSetting(inspectorAttachedHeightName, Setting(attachedHeight));

We aren't saving the preferred height, we're saving the constrained height. I
don't think this comment is needed at all, though.

>  void InspectorController::showWindow()
>  {
>      ASSERT(enabled());
> +    
> +    int inspectedHeight =
m_inspectedPage->mainFrame()->view()->visibleHeight();
> +    
>      m_client->showWindow();
> +    
> +    Setting attachedHeight = setting(inspectorAttachedHeightName);
> +    int preferredHeight = attachedHeight.type() == Setting::IntegerType ?
attachedHeight.integerValue() : defaultAttachedHeight;
> +   
m_client->setAttachedWindowHeight(constrainedAttachedWindowHeight(preferredHeig
ht, inspectedHeight));
>  }

We only need to do this if we're going to start out attached, right? Again, I
think a comment would be useful. It might also be worth adding a FIXME about
how it's strange that we have to do this both here and in attachWindow; we
should only have to do it once. It might be that we can fix this eventually by
moving more code down into WebCore, but we don't have to do that now.

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

> -
> +	   

Please undo this whitespace change.

>  - (void)setAttachedWindowHeight:(unsigned)height
>  {
> -    [[NSUserDefaults standardUserDefaults] setInteger:height
forKey:WebKitInspectorAttachedViewHeightKey];
> -
>      if (!_attachedToInspectedWebView)
>	   return;
> -
> -    WebFrameView *frameView = [[_inspectedWebView mainFrame] frameView];
> -    NSRect frameViewRect = [frameView frame];
> -
> -    CGFloat attachedHeight = round(MAX(250.0, MIN(height,
(NSHeight([_inspectedWebView frame]) * 0.75))));
> -
> +    

Please remove the whitespace on this line.

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

Please add a comment here explaining why totalHeight is the right height to set
m_inspectedWebViewHwnd to.

So close! I'm really happy with how this is shaping up.