Bug 101978 – Web Inspector: Fix docking behaviour on Windows.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Roger Fong]

There are a number of problems for docking behaviour on Windows.
For starters it does not ever constrain the inspectors size properly while docked.
It also does not properly set the whether or not the inspector can be docked/undocked.

[Roger Fong]

Created attachment 173710 [details]
patch

[Tim Horton]

Comment on attachment 173710 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173710&action=review

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:143
> +    // Call setDockingUnavailable before bringToFront. If we display the inspector's HWND first it causes the call to canAttachWindow to return the wrong result.

I wonder if you can talk about this in a more generic way. Using the word HWND in cross-platform code is weird (at the very least, note that this is Windows only).
Also, what/why does it cause canAttachWindow to return the wrong result?

> Source/WebKit/win/ChangeLog:8
> +        There are a number of problems for docking behaviour on Windows.

s/for/with/

> Source/WebKit/win/ChangeLog:9
> +        For starters it does not ever constrain the inspectors size properly while docked.

s/For starters/For starters,/
s/inspectors/inspector's/

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:296
> +    // We need to set the attached windows height before we actually attach the window.

s/windows/window's/

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:297
> +    // Make sure that m_attached is true so that setAttachedWindowHeight succeeds.

s/succeeds/doesn't return early/?

[Brian Weinstein]

Comment on attachment 173710 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173710&action=review

I'd like to see one more version with Tim's comments and mine addressed. Patch looks good though.

> Source/WebKit/win/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=100000

100000!!!

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:258
> +        restoreAttachedWindowHeight();

A comment explaining why we are restoring the attached window height before attaching would be nice.

>> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:297
>> +    // Make sure that m_attached is true so that setAttachedWindowHeight succeeds.
> 
> s/succeeds/doesn't return early/?

Should this say restoreAttachedWindowHeight instead of setAttachedWindowHeight?

[Roger Fong]

> >> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:297
> >> +    // Make sure that m_attached is true so that setAttachedWindowHeight succeeds.
> > 
> > s/succeeds/doesn't return early/?
> 
> Should this say restoreAttachedWindowHeight instead of setAttachedWindowHeight?

I meant setAttachedWindowHeight because restore calls setAttachedWindowHeight which has a check:

if (!m_attached)
 return;

which prevents the final height from being et properly. I guess both are correct but I figured it'd be a little clearer using the method that actually fails. Do you think I should make it restoreAttachedWindowHeight tho?

[Roger Fong]

Created attachment 173746 [details]
revised patch

[Brian Weinstein]

Comment on attachment 173746 [details]
revised patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173746&action=review

> Source/WebCore/inspector/InspectorFrontendClientLocal.cpp:143
> +    // Call setDockingUnavailable before bringToFront. If we display the inspector window via bringToFront first it causes the call to canAttachWindow to return the wrong result.

Another sentence on why this is the case would be helpful.

> Source/WebKit/win/ChangeLog:15
> +        (WebInspectorFrontendClient::frontendLoaded):
> +        (WebInspectorFrontendClient::attachWindow):

A high level comment about the changes in these functions would nice for people reading the ChangeLog later.

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:299
> +    // Immediately after calling showWindowWithoutNotifications(), the parent frameview's visibleHeight incorrectly returns 0 always.

Is this a WebKit bug or a Windows bug?

[Roger Fong]

Committed: http://trac.webkit.org/changeset/134327

And made the last few remaining changes.

> Is this a WebKit bug or a Windows bug?

Yeup, windows bug. I added a comment or two to clarify

[Tim Horton]

Closing since it looks like this landed.