Bug 65763 - Web Inspector: implement dock/undock in WebKit2 without getting into WebCore.
Summary: Web Inspector: implement dock/undock in WebKit2 without getting into WebCore.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Pavel Feldman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-05 04:51 PDT by Pavel Feldman
Modified: 2011-08-08 01:12 PDT (History)
11 users (show)

See Also:


Attachments
Patch (22.17 KB, patch)
2011-08-05 05:14 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] With typo fixed (22.16 KB, patch)
2011-08-05 05:37 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Moved minimum size check & constants to WebInspectorProxy (21.89 KB, patch)
2011-08-05 07:24 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
[PATCH] Patch with better changelog, for landing (22.59 KB, patch)
2011-08-05 08:57 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pavel Feldman 2011-08-05 04:51:27 PDT
Patch to follow.
Comment 1 Pavel Feldman 2011-08-05 05:14:52 PDT
Created attachment 103061 [details]
Patch
Comment 2 Yury Semikhatsky 2011-08-05 05:34:03 PDT
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
Comment 3 Pavel Feldman 2011-08-05 05:37:38 PDT
Created attachment 103064 [details]
[PATCH] With typo fixed
Comment 4 Yury Semikhatsky 2011-08-05 05:39:32 PDT
Inspector stuff looks good, let's wait for WK2 experts to confirm that everything is OK.
Comment 5 Adam Roben (:aroben) 2011-08-05 06:32:45 PDT
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.
Comment 6 Pavel Feldman 2011-08-05 06:37:15 PDT
(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.
Comment 7 Adam Roben (:aroben) 2011-08-05 06:39:29 PDT
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.
Comment 8 Pavel Feldman 2011-08-05 07:24:38 PDT
Created attachment 103071 [details]
[PATCH] Moved minimum size check & constants to WebInspectorProxy
Comment 9 Adam Roben (:aroben) 2011-08-05 07:47:35 PDT
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.)
Comment 10 Pavel Feldman 2011-08-05 08:57:53 PDT
Created attachment 103076 [details]
[PATCH] Patch with better changelog, for landing
Comment 11 Brian Weinstein 2011-08-05 09:56:30 PDT
(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+.
Comment 12 Pavel Feldman 2011-08-08 01:12:58 PDT
Committed r92587: <http://trac.webkit.org/changeset/92587>