Bug 109298 - Web Inspector: Show Elements sidebar panes in two tabbed panes side by side
Summary: Web Inspector: Show Elements sidebar panes in two tabbed panes side by side
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: Vladislav Kaznacheev
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-08 06:44 PST by Vladislav Kaznacheev
Modified: 2013-02-15 09:06 PST (History)
12 users (show)

See Also:


Attachments
Current state (334.80 KB, image/png)
2013-02-08 06:45 PST, Vladislav Kaznacheev
no flags Details
Suggested UI (135.09 KB, image/png)
2013-02-08 06:45 PST, Vladislav Kaznacheev
no flags Details
Patch (23.20 KB, patch)
2013-02-08 06:53 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (21.71 KB, patch)
2013-02-11 06:05 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (36.47 KB, patch)
2013-02-14 06:24 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (32.41 KB, patch)
2013-02-15 01:04 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (31.41 KB, patch)
2013-02-15 04:43 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (31.35 KB, patch)
2013-02-15 08:43 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff
Patch (31.35 KB, patch)
2013-02-15 08:47 PST, Vladislav Kaznacheev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vladislav Kaznacheev 2013-02-08 06:44:26 PST
With the new experiment ("Allow horizontal split in Elements and Sources panels") the sidebar panes could be shown below the content in a tabbed pane. This often leaves quite a lot of unused. It would be better to have an option to arrange Elements panes into two separate tabbed panes.
Comment 1 Vladislav Kaznacheev 2013-02-08 06:45:24 PST
Created attachment 187309 [details]
Current state
Comment 2 Vladislav Kaznacheev 2013-02-08 06:45:56 PST
Created attachment 187310 [details]
Suggested UI
Comment 3 Vladislav Kaznacheev 2013-02-08 06:53:25 PST
Created attachment 187312 [details]
Patch
Comment 4 WebKit Review Bot 2013-02-08 07:27:06 PST
Comment on attachment 187312 [details]
Patch

Attachment 187312 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16443612

New failing tests:
http/tests/inspector/appcache/appcache-iframe-manifests.html
http/tests/inspector/injected-script-for-origin.html
http/tests/inspector/extensions-network-redirect.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/modify-cross-domain-rule.html
http/tests/inspector/appcache/appcache-manifest-with-non-existing-file.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/filesystem/delete-entry.html
http/tests/inspector/extensions-headers.html
http/tests/inspector-enabled/injected-script-discard.html
http/tests/inspector/console-cross-origin-iframe-logging.html
http/tests/inspector/appcache/appcache-swap.html
http/tests/inspector/extensions-useragent.html
http/tests/inspector-enabled/dynamic-scripts.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector/compiler-source-mapping-debug.html
http/tests/inspector/web-socket-frame-error.html
http/tests/inspector/resource-har-conversion.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
http/tests/inspector/console-cd.html
http/tests/inspector/audits/set-cookie-header-audit-no-false-positive.html
http/tests/inspector/resource-har-headers.html
http/tests/inspector/network-preflight-options.html
http/tests/inspector/inspect-iframe-from-different-domain.html
http/tests/inspector-enabled/console-log-before-frame-navigation.html
http/tests/inspector-enabled/dom-storage-open.html
http/tests/inspector/resource-parameters.html
http/tests/inspector-enabled/database-open.html
Comment 5 Build Bot 2013-02-08 09:31:54 PST
Comment on attachment 187312 [details]
Patch

Attachment 187312 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16432717

New failing tests:
inspector/debugger/breakpoint-manager.html
inspector/elements/delete-from-document.html
http/tests/inspector/extensions-network-redirect.html
inspector/editor/highlighter-basics.html
http/tests/inspector/change-iframe-src.html
inspector/console/command-line-api-getEventListeners.html
http/tests/inspector/console-resource-errors.html
http/tests/inspector/extensions-headers.html
inspector/cookie-resource-match.html
inspector/console/clients-ignored-in-privatebrowsing.html
http/tests/inspector/extensions-useragent.html
inspector/audits/audits-panel-functional.html
inspector/extensions/extensions-api.html
inspector/debugger/debugger-activation-crash2.html
inspector/cookie-parser.html
http/tests/inspector/compiler-script-mapping.html
inspector/editor/brace-matcher.html
inspector/console/alert-toString-exception.html
inspector/curl-command.html
inspector/elements/breadcrumb-updates.html
http/tests/inspector/extensions-ignore-cache.html
http/tests/inspector/console-xhr-logging.html
http/tests/inspector/console-xhr-logging-async.html
inspector/profiler/cpu-profiler-agent-crash-on-start.html
inspector/debugger/content-providers.html
fast/loader/javascript-url-in-object.html
inspector/editor/highlighter-chunk-limit.html
inspector/debugger/debugger-activation-crash.html
inspector/audits/audits-panel-noimages-functional.html
Comment 6 Vsevolod Vlasov 2013-02-11 01:21:02 PST
Comment on attachment 187312 [details]
Patch

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

> Source/WebCore/inspector/front-end/SidebarPane.js:29
> +importScript("SplitView.js");

Redundant, SplitView.js is now loaded with a script tag in inspector.html.

> Source/WebCore/inspector/front-end/SidebarPane.js:313
> +    this._tabbedPaneLeft.show(this.firstElement());

I would keep naming them First and Second.

> Source/WebCore/inspector/front-end/SidebarPane.js:341
> +        if (pane.secondary && !this._hasSecondaryPanes) {

I would think of "secondary" as a pane adding mode rather than a pane property, I think this should be an addPane parameter.

> Source/WebCore/inspector/front-end/SidebarPane.js:457
> +            this._currentView.addPane(this._panes[id], id);

I think pane order is lost here, or at least becomes javascript engine dependent.
We can keep an array of panes and add a paneHistory array like it is done in TabbedPane (please note that it has a reverse ordering there.).

> Source/WebCore/inspector/front-end/SplitView.js:86
> +     * @param {boolean} on

SplitView change looks like a completely separate thing, I would extract it.

> Source/WebCore/inspector/front-end/SplitView.js:88
> +    usePercentage: function(on)

A setter would be more readable here.
I would actually prefer an approach of Swing's SplitPane where a width between 0 and 1 is considered a percentage.
This way you just need to set width to 0.5 by default and you are done.

> Source/WebCore/inspector/front-end/SplitView.js:296
> +        this._totalSizePercent = this._totalSize / 100;

Looks like a private method would fit better than a field.

> Source/WebCore/inspector/front-end/SplitView.js:298
> +            this._sidebarSize *= this._totalSize / oldTotalSize;

this._sidebarSize is used externally to position custom resizers. I don't think it is acceptable to use complex calculations with potential accumulated error for it.
offsetWidth/Height would fit better especially since we already do that for the whole sidebar.

> Source/WebCore/inspector/front-end/SplitView.js:456
> +        if (this._usePercentage)

We should save size of sidebar in percents in settings if we use percentage.
Comment 7 Vladislav Kaznacheev 2013-02-11 03:30:44 PST
Comment on attachment 187312 [details]
Patch

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

>> Source/WebCore/inspector/front-end/SidebarPane.js:313
>> +    this._tabbedPaneLeft.show(this.firstElement());
> 
> I would keep naming them First and Second.

Done

>> Source/WebCore/inspector/front-end/SplitView.js:86
>> +     * @param {boolean} on
> 
> SplitView change looks like a completely separate thing, I would extract it.

Done (https://bugs.webkit.org/show_bug.cgi?id=109414)

>> Source/WebCore/inspector/front-end/SplitView.js:88
>> +    usePercentage: function(on)
> 
> A setter would be more readable here.
> I would actually prefer an approach of Swing's SplitPane where a width between 0 and 1 is considered a percentage.
> This way you just need to set width to 0.5 by default and you are done.

Done (https://bugs.webkit.org/show_bug.cgi?id=109414)

>> Source/WebCore/inspector/front-end/SplitView.js:296
>> +        this._totalSizePercent = this._totalSize / 100;
> 
> Looks like a private method would fit better than a field.

Got rid of the variable (https://bugs.webkit.org/show_bug.cgi?id=109414)

>> Source/WebCore/inspector/front-end/SplitView.js:298
>> +            this._sidebarSize *= this._totalSize / oldTotalSize;
> 
> this._sidebarSize is used externally to position custom resizers. I don't think it is acceptable to use complex calculations with potential accumulated error for it.
> offsetWidth/Height would fit better especially since we already do that for the whole sidebar.

Done (https://bugs.webkit.org/show_bug.cgi?id=109414)

>> Source/WebCore/inspector/front-end/SplitView.js:456
>> +        if (this._usePercentage)
> 
> We should save size of sidebar in percents in settings if we use percentage.

But we already do!
Comment 8 Vladislav Kaznacheev 2013-02-11 06:05:51 PST
Created attachment 187555 [details]
Patch
Comment 9 Vladislav Kaznacheev 2013-02-14 06:24:46 PST
Created attachment 188334 [details]
Patch
Comment 10 Vladislav Kaznacheev 2013-02-15 01:04:12 PST
Created attachment 188505 [details]
Patch
Comment 11 Vsevolod Vlasov 2013-02-15 03:37:48 PST
Comment on attachment 188505 [details]
Patch

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

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1189
> +        if (!this._splitDirectionSetting) {

This could be moved to constructor simply as
this._splitDirectionSetting = WebInspector.settings.createSetting(this.name + "PanelSplitHorizontally, false);
this._splitDirectionSetting.addChangeListener(this._arrangeSidebarPanes.bind(this));

> Source/WebCore/inspector/front-end/ScriptsPanel.js:1197
> +        if (!this._splitSidebarSetting) {

Ditto.

> Source/WebCore/inspector/front-end/SidebarPane.js:80
>      _setExpandCallback: function(callback)

I don't like that this setter returns a boolean value. Why not call expandCallback explicitly here?
Comment 12 Vladislav Kaznacheev 2013-02-15 04:43:32 PST
Created attachment 188536 [details]
Patch
Comment 13 WebKit Review Bot 2013-02-15 06:45:26 PST
Comment on attachment 188536 [details]
Patch

Rejecting attachment 188536 [details] from commit-queue.

New failing tests:
inspector/console/console-api-on-call-frame.html
inspector/debugger/scripts-panel.html
inspector/styles/css-live-edit.html
inspector/extensions/extensions-resources.html
inspector/debugger/open-close-open.html
inspector/debugger/script-formatter-breakpoints.html
http/tests/inspector/compiler-source-mapping-debug.html
Full output: http://queues.webkit.org/results/16581854
Comment 14 Vladislav Kaznacheev 2013-02-15 08:43:02 PST
Created attachment 188581 [details]
Patch
Comment 15 Vladislav Kaznacheev 2013-02-15 08:47:02 PST
Created attachment 188582 [details]
Patch
Comment 16 WebKit Review Bot 2013-02-15 09:06:50 PST
Comment on attachment 188582 [details]
Patch

Clearing flags on attachment: 188582

Committed r143006: <http://trac.webkit.org/changeset/143006>
Comment 17 WebKit Review Bot 2013-02-15 09:06:57 PST
All reviewed patches have been landed.  Closing bug.