[webkit-reviews] review denied: [Bug 14272] Cannot change the height of the Web Inspector when docked on Windows : [Attachment 32688] Cross Platform Fix + Refactoring

[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 32688: Cross Platform Fix + Refactoring
https://bugs.webkit.org/attachment.cgi?id=32688&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
> +	   * inspector/InspectorController.cpp: Moved resizing logic from
InspectorClient to here

That should be "WebInspectorClient.mm", not "InspectorClient".

> +    virtual long getInspectedWebViewHeight() = 0;

We normally only prefix function names with "get" if they return their result
through an out-parameter. Since that is not the case here, I'd just call this
"inspectedWebViewHeight()". Since WebCore doesn't really know what a "WebView"
is, I'd also rename it to "inspectedPageHeight()". But this function doesn't
return the height of the inspected page. It returns the height the page would
be if the Inspector weren't docked. We should either rename the function to
indicate that, or make it truly return the inspected page's height and make
InspectorController responsible for adding in the docked Inspector's height.
The latter option seems conceptually simpler to me.

> @@ -188,6 +191,12 @@ InspectorController::InspectorController
>      ASSERT_ARG(page, page);
>      ASSERT_ARG(client, client);
>      ++s_inspectorControllerCount;
> +
> +    // Set the initial attached height to the users preferred attached
height
> +    // It might not start out attached, but this variable needs to be
populated before
> +    // it could be attached

Typo: "users" should be "user's". You should also end your sentences with
periods.

> +    long attachedHeight = round(max(static_cast<float>(250.0), 
> +	   static_cast<float>(min(static_cast<float>(height), 
> +	   static_cast<float>(m_client->getInspectedWebViewHeight() * .75)))));


You should put the constants into named variables. I think that would reduce
the need for casting, too:

static const minimumAttachedHeight = 250;
static const maximumAttachedHeightRatio = 0.75;

setAttachedWindowHeight(height)
{
    long attachedHeight = round(max(minimumAttachedHeight,
min(static_cast<float>(height), maximumAttachedHeightRatio *
inspectedPageHeight())));
}

Why is defaultAttachedHeight unsigned when we're using long elsewhere?

> +    // Save a preference for the users preferred attached height
> +    Setting heightSetting;
> +    heightSetting.set(attachedHeight);
> +    setSetting(inspectorAttachedHeightName, heightSetting);

It would probably be better to add an explicit Setting constructor that takes a
long.

> +++ WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp	(working copy)
> @@ -112,6 +112,17 @@ String InspectorClient::hiddenPanels()
>      notImplemented();
>      return String();
>  }
> +    
> +long getInspectedWebViewHeight()
> +{
> +    notImplemented();
> +    return 0;
> +}
> +    
> +void setInitialAttachedHeight(long height)
> +{
> +    notImplemented();
> +}

These function names need to be prefixed with "InspectorClient::". (Ditto for
the other platforms, though the prefix will be different for each.)

> @@ -69,6 +72,7 @@ public:
>  private:
>      void updateWindowTitle() const;
>  
> +    long m_initialAttachedHeight;

It looks like this member doesn't get initialized by the constructor.

> +- (long)getInspectedWebViewHeight
> +{
> +    ASSERT(_attachedToInspectedWebView);
> +    
> +    // For calculations, we want the height of the frame view and the height
of the attached inspector
> +    // to calculate the correct bounds

This comment doesn't really make sense to me as written. But if we either
rename the function or change its behavior as described above, you probably
won't need a comment at all.

> @@ -332,7 +363,7 @@ void WebInspectorClient::updateWindowTit
>  
>	   _attachedToInspectedWebView = YES;
>  
> -	   [self setAttachedWindowHeight:[[NSUserDefaults standardUserDefaults]
integerForKey:WebKitInspectorAttachedViewHeightKey]];
> +	   [self setAttachedWindowHeight:_attachedHeight];

I think you can get rid of the definition of
WebKitInspectorAttachedViewHeightKey now.

Should we migrate the old NSUserDefaults-style preference to the new
InspectorController::Setting-style preference?

> @@ -381,23 +412,21 @@ void WebInspectorClient::updateWindowTit
>  
>  - (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))));
> -
> +    
> +    _attachedHeight = height;

Why do we need to store this value? Can't we just calculate it by querying the
bounds of the Inspector window?

> @@ -76,6 +74,7 @@ WebInspectorClient::WebInspectorClient(W
>      , m_webViewHwnd(0)
>      , m_shouldAttachWhenShown(false)
>      , m_attached(false)
> +    , m_attachedHeight(0)

You're leaving m_attachedParentHeight uninitialized here.

>  void WebInspectorClient::setAttachedWindowHeight(unsigned height)
>  {
> -    // FIXME: implement this.
> +    RECT inspectedRect;
> +    GetClientRect(m_inspectedWebViewHwnd, &inspectedRect);
> +
> +    int webViewX = inspectedRect.left;
> +    int webViewY = m_attachedParentHeight - height;
> +    int webViewWidth = inspectedRect.right - inspectedRect.left;
> +    int webViewHeight = height;
> +
> +    m_attachedHeight = height;

Why do we need to store this value? Can't we just calculate it by querying the
client rect of the Inspector window?

> +    SendMessage(m_inspectedWebViewHwnd, WM_SETREDRAW, FALSE, NULL);
> +    SendMessage(m_webViewHwnd, WM_SETREDRAW, FALSE, NULL);

We use 0 instead of NULL in C++ files. (This comment applies elsewhere in this
patch.)

Should we be doing this WM_SETREDRAW stuff in onWebViewWindowPosChanging, too?

> +    ::SetWindowPos(m_inspectedWebViewHwnd, 0, webViewX, inspectedRect.top,
webViewWidth, m_attachedParentHeight, SWP_NOZORDER);
> +    ::SetWindowPos(m_webViewHwnd, 0, webViewX, webViewY, webViewWidth,
webViewHeight, SWP_NOZORDER);

You should remove the "::" prefixes here, or add them to the other Win32 API
calls in this function (whatever is consistent with the rest of the file). In
general, in new code you should leave them off.

> @@ -410,12 +440,16 @@ void WebInspectorClient::onWebViewWindow
>      WINDOWPOS* windowPos = reinterpret_cast<WINDOWPOS*>(lParam);
>      ASSERT_ARG(lParam, windowPos);
>  
> +    // Cache this value because GetClientRect doesn't give the correct
parent height
> +    // And whenever the height updates and an inspector is attached, this
function will be called first.
> +    m_attachedParentHeight = windowPos->cy;

Why do we need to store this value? Can't we calculate it by getting the client
rects of the inspected WebView and the Inspector, and adding their heights?