Bug 65763 – Web Inspector: implement dock/undock in WebKit2 without getting into WebCore.

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

[Pavel Feldman]

Patch to follow.

[Pavel Feldman]

Created attachment 103061 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 103061 [details]
Patch

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

> Source/WebKit2/UIProcess/WebInspectorProxy.cpp:221
> +    my_isAttached = shouldOpenAttached();

my_isAttached -> m_isAttached

[Pavel Feldman]

Created attachment 103064 [details]
[PATCH] With typo fixed

[Yury Semikhatsky]

Inspector stuff looks good, let's wait for WK2 experts to confirm that everything is OK.

[Adam Roben (:aroben)]

Comment on attachment 103064 [details]
[PATCH] With typo fixed

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

> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:259
> +    return minimumAttachedHeight <= (inspectedWindowHeight * 3 / 4);

The 3/4 here should be a constant like in the Mac code. But ideally we'd share this code between all platforms. We'd just need a cross-platform abstraction for getting the inspected window's height.

[Pavel Feldman]

(In reply to comment #5)
> (From update of attachment 103064 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103064&action=review
> 
> > Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:259
> > +    return minimumAttachedHeight <= (inspectedWindowHeight * 3 / 4);
> 
> The 3/4 here should be a constant like in the Mac code. But ideally we'd share this code between all platforms. We'd just need a cross-platform abstraction for getting the inspected window's height.

I was thinking of doing the platformInspectedWindowHeight instead of the platformCanAttach, but then I thought that different platforms can have different considerations wrt preferred / minimum inspector size. Now sure which is better - flexibility or consistency in this case. One of the most popular requests on the inspector for chrome is do dock to side.

[Adam Roben (:aroben)]

Comment on attachment 103064 [details]
[PATCH] With typo fixed

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

>>> Source/WebKit2/UIProcess/win/WebInspectorProxyWin.cpp:259
>>> +    return minimumAttachedHeight <= (inspectedWindowHeight * 3 / 4);
>> 
>> The 3/4 here should be a constant like in the Mac code. But ideally we'd share this code between all platforms. We'd just need a cross-platform abstraction for getting the inspected window's height.
> 
> I was thinking of doing the platformInspectedWindowHeight instead of the platformCanAttach, but then I thought that different platforms can have different considerations wrt preferred / minimum inspector size. Now sure which is better - flexibility or consistency in this case. One of the most popular requests on the inspector for chrome is do dock to side.

Since the Web Inspector is a feature of WebKit, not a feature of any particular browsers, I think consistency should be preferred.

[Pavel Feldman]

Created attachment 103071 [details]
[PATCH] Moved minimum size check & constants to WebInspectorProxy

[Adam Roben (:aroben)]

Comment on attachment 103071 [details]
[PATCH] Moved minimum size check & constants to WebInspectorProxy

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

Should we change WebKit1 to use ?docked=true, too?

The WebKit2 code looks fine to me. I'll let someone more familiar with the Inspector give the final r+.

> Source/WebCore/ChangeLog:14
> +        Web Inspector: implement dock/undock in WebKit2 without getting into WebCore.
> +        https://bugs.webkit.org/show_bug.cgi?id=65763
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        * WebCore.exp.in:
> +        * inspector/InspectorController.cpp:
> +        * inspector/InspectorController.h:
> +        * inspector/InspectorFrontendClient.h:
> +        * inspector/InspectorFrontendClientLocal.h:
> +        * inspector/front-end/inspector.js:
> +        (windowLoaded):

You should explain what your patch is actually doing, and why. This makes it easier for reviewers and others to understand your changes. (Ditto for your other ChangeLogs.)

[Pavel Feldman]

Created attachment 103076 [details]
[PATCH] Patch with better changelog, for landing

[Brian Weinstein]

(In reply to comment #10)
> Created an attachment (id=103076) [details]
> [PATCH] Patch with better changelog, for landing

docked=true is a very nice way to handle this. Thanks for looking into it! I was going to ask if another solution would be to just port the "canAttach" to WebKit2, but you beat me to the punch. This looks good to me. If this lands, I can file a follow up bug to have the other WebKit ports switch over to docked=true.

This looks good to me, but would be nice if an inspector person could r+.

[Pavel Feldman]

Committed r92587: <http://trac.webkit.org/changeset/92587>