Bug 101612 - Web Inspector: split SplitView into SplitView and SidebarView
Summary: Web Inspector: split SplitView into SplitView and SidebarView
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: 2012-11-08 08:49 PST by Pavel Feldman
Modified: 2012-11-09 01:23 PST (History)
11 users (show)

See Also:


Attachments
Patch (54.63 KB, patch)
2012-11-08 08:58 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
[Patch] with canvas profiler migrated to the split (57.82 KB, patch)
2012-11-08 11:01 PST, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (58.02 KB, patch)
2012-11-09 01:08 PST, Pavel Feldman
vsevik: 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 2012-11-08 08:49:13 PST
Currently, SplitView is rather a sidebar view. I'm extracting the reusable split component from it.
Comment 1 Pavel Feldman 2012-11-08 08:58:32 PST
Created attachment 173055 [details]
Patch
Comment 2 Pavel Feldman 2012-11-08 11:01:19 PST
Created attachment 173074 [details]
[Patch] with canvas profiler migrated to the split
Comment 3 kov's GTK+ EWS bot 2012-11-08 13:24:32 PST
Comment on attachment 173074 [details]
[Patch] with canvas profiler migrated to the split

Attachment 173074 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/14777132
Comment 4 Vsevolod Vlasov 2012-11-09 00:07:47 PST
Comment on attachment 173074 [details]
[Patch] with canvas profiler migrated to the split

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

> Source/WebCore/inspector/front-end/ScriptsPanel.js:75
> +    this.splitView.setMinimalMainWidthPercent(maximalDebugSidebarWidthPercent);

"100 - " is missing

> Source/WebCore/inspector/front-end/SidebarView.js:38
> +    WebInspector.SplitView.call(this, true, sidebarWidthSettingName, defaultSidebarWidth || 200);

I don't think this is correct way to set defaultSidebarWidth in case of right sidebar.
Also your code will save splitOffset in setting which is not correct, we should save "constant width" element width.

> Source/WebCore/inspector/front-end/SidebarView.js:50
> +    this.setChangeFirstOnResize(sidebarPosition !== WebInspector.SidebarView.SidebarPosition.Left);

I would move setChangeFirstOnResize call to _innerSetSidebarPosition

> Source/WebCore/inspector/front-end/SidebarView.js:127
> +        this.setSplitOffset(width);

This is not correct for right sidebar as far as I can see.

> Source/WebCore/inspector/front-end/SidebarView.js:135
> +        return this.splitOffset();

Ditto

> Source/WebCore/inspector/front-end/SplitView.js:46
> +    this._firstElement.className = "split-view-contents split-view-contents-" + (isVertical ? "vertical" : "horizontal");

var orientationString = isVertical ? "vertical" : "horizontal"?
Comment 5 Pavel Feldman 2012-11-09 01:08:16 PST
Created attachment 173232 [details]
Patch
Comment 6 Pavel Feldman 2012-11-09 01:23:06 PST
Committed r134031: <http://trac.webkit.org/changeset/134031>