Bug 109298 – Web Inspector: Show Elements sidebar panes in two tabbed panes side by side

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

[Vladislav Kaznacheev]

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.

[Vladislav Kaznacheev]

Created attachment 187309 [details]
Current state

[Vladislav Kaznacheev]

Created attachment 187310 [details]
Suggested UI

[Vladislav Kaznacheev]

Created attachment 187312 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

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

[Vsevolod Vlasov]

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.

[Vladislav Kaznacheev]

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!

[Vladislav Kaznacheev]

Created attachment 187555 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 188334 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 188505 [details]
Patch

[Vsevolod Vlasov]

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?

[Vladislav Kaznacheev]

Created attachment 188536 [details]
Patch

[WebKit Review Bot]

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

[Vladislav Kaznacheev]

Created attachment 188581 [details]
Patch

[Vladislav Kaznacheev]

Created attachment 188582 [details]
Patch

[WebKit Review Bot]

Comment on attachment 188582 [details]
Patch

Clearing flags on attachment: 188582

Committed r143006: <http://trac.webkit.org/changeset/143006>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.