Bug 153033 – Web Inspector: Timelines UI redesign: replace content view container with a content browser

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* SUMMARY
Replace content view container with a content browser. The content browser's navigation bar has advantages for the redesigned UI:

- Creates a visual separation between the overview graphs and timeline view.
- Provides a place to relocate the filter and scope bar elements that exist in the sidebar.
- Show the path components specific to the selection in the timeline view. We currently show these in the main navigation bar above the overview graph. It makes more sense to show it directly above view to which it applies.

[Radar WebKit Bug Importer]

<rdar://problem/24195565>

[Matt Baker]

Created attachment 272150 [details]
[Video] selection path components UI

[Matt Baker]

Created attachment 272166 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 272166 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87
> +    filter: opacity(0.6);

opacity: 0.6;

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:52
> +        this._timelineSelectionPathComponent.previousSibling = this._entireRecordingPathComponent;

Should also do:
this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent;

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
> +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);

Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:685
> +            let zeroTime = this._timelineOverview.viewMode === WebInspector.TimelineOverview.ViewMode.Timelines ? this.currentTimelineView.zeroTime : 0;

Why won't this.currentTimelineView.zeroTime work for either mode?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:746
> +        this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent;

Maybe this is why you don't set nextSibling at the beginning. If so that might be worth a comment above.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754
> +            startValue += 1;    // Convert index to frame number.

One space.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797
> +WebInspector.TimelineRange = class TimelineRange

Should be in a new file. One class per file.

[Joseph Pecoraro]

Comment on attachment 272166 [details]
[Patch] Proposed Fix

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

Looking good! I had mostly style nits.

> Source/WebInspectorUI/ChangeLog:107
> +        (WebInspector.TimelineRange):
> +        (WebInspector.TimelineRange.prototype.get startValue):
> +        (WebInspector.TimelineRange.prototype.set startValue):
> +        (WebInspector.TimelineRange.prototype.get endValue):
> +        (WebInspector.TimelineRange.prototype.set endValue):
> +        New represented object for ruler selection path components.

We should pull this out into its own Model file then, and it should extend WebInspector.Object. Just for consistency:

    UserInterface/Models/TimelineRange.js

Which should probably also be included by Test.html.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:46
> +        let disableBackForward = true;

Style: For temps like this that name a parameter I've been using `const`.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50
> +        this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording"));

Nit: I think xenon suggested lowercasing the second word here: "Entire recording".

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:349
> +        return new WebInspector.GeneralTreeElement(iconClassName, title, representedObject, false);

Nit: You could have a `const hasChildren = false` here for clarity.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
>> +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);
> 
> Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444.

We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?).

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:396
> +            timelineRuler.selectionStartTime = timelineRuler.zeroTime + timelineRange.startValue
> +            timelineRuler.selectionEndTime = timelineRuler.zeroTime + timelineRange.endValue

Style: Semicolon.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754
>> +            startValue += 1;    // Convert index to frame number.
> 
> One space.

Style: For a trailing line comment, one space after the semicolon is the norm. // Like so.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:762
> +        this._timelineSelectionPathComponent.title = displayName;

I think this line is stale. There is setter for `title` in HierarchicalPathComponent, and nothing seems to use it. Also, HierarchicalPathComponent seem to use the displayName as the tooltip title on the element already.

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797
>> +WebInspector.TimelineRange = class TimelineRange
> 
> Should be in a new file. One class per file.

Deserves its own file.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:808
> +    get startValue()
> +    {
> +        return this._startValue;
> +    }

Style: Lately for trivial getters/setters we've been single lining them:

    // Public

    get startValue() { return this._startValue; }
    set startValue(x) { this._startValue = x; }

[Timothy Hatcher]

Comment on attachment 272166 [details]
[Patch] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50
>> +        this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording"));
> 
> Nit: I think xenon suggested lowercasing the second word here: "Entire recording".

I sugested Entire Recording, which is what he did. The video he posted had recording lowercase. He corrected it for the patch.

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 272166 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272166&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87
> > +    filter: opacity(0.6);
> 
> opacity: 0.6;
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:52
> > +        this._timelineSelectionPathComponent.previousSibling = this._entireRecordingPathComponent;
> 
> Should also do:
> this._entireRecordingPathComponent.nextSibling =
> this._timelineSelectionPathComponent;
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
> > +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);
> 
> Might add a FIXME here to localize whole strings sainted of using %s.
> Localizers like whole strings in cases line this so they can control it
> better. See rdar://problem/24206444.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:685
> > +            let zeroTime = this._timelineOverview.viewMode === WebInspector.TimelineOverview.ViewMode.Timelines ? this.currentTimelineView.zeroTime : 0;
> 
> Why won't this.currentTimelineView.zeroTime work for either mode?

Like all timeline views, the rendering frames view has a zero time equal to that of the recording. Since it isn't used it would probably be cleaner to override it in the view and remove this special case.

> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:746
> > +        this._entireRecordingPathComponent.nextSibling = this._timelineSelectionPathComponent;
> 
> Maybe this is why you don't set nextSibling at the beginning. If so that
> might be worth a comment above.

Correct. "Entire Recording" has no siblings until a selection is made. This state is fleeting since a default selection is created immediately, but that's an implementation detail of TimelineOverview.
 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754
> > +            startValue += 1;    // Convert index to frame number.
> 
> One space.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797
> > +WebInspector.TimelineRange = class TimelineRange
> 
> Should be in a new file. One class per file.

[Matt Baker]

(In reply to comment #5)
> Comment on attachment 272166 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272166&action=review
> 
> Looking good! I had mostly style nits.
> 
> > Source/WebInspectorUI/ChangeLog:107
> > +        (WebInspector.TimelineRange):
> > +        (WebInspector.TimelineRange.prototype.get startValue):
> > +        (WebInspector.TimelineRange.prototype.set startValue):
> > +        (WebInspector.TimelineRange.prototype.get endValue):
> > +        (WebInspector.TimelineRange.prototype.set endValue):
> > +        New represented object for ruler selection path components.
> 
> We should pull this out into its own Model file then, and it should extend
> WebInspector.Object. Just for consistency:
> 
>     UserInterface/Models/TimelineRange.js
> 
> Which should probably also be included by Test.html.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:46
> > +        let disableBackForward = true;
> 
> Style: For temps like this that name a parameter I've been using `const`.

Nice.

> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50
> > +        this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording"));
> 
> Nit: I think xenon suggested lowercasing the second word here: "Entire
> recording".
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:349
> > +        return new WebInspector.GeneralTreeElement(iconClassName, title, representedObject, false);
> 
> Nit: You could have a `const hasChildren = false` here for clarity.
> 
> >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
> >> +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);
> > 
> > Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444.
> 
> We may want to rename "navigationSidebarTreeOutlineLabel" to something more
> appropriate now that there is no sidebar (right?).

Maybe we should remove this altogether. It's only used in the filter bar placeholder text now, and most (4 of 6) timeline views just return "Records".

> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:396
> > +            timelineRuler.selectionStartTime = timelineRuler.zeroTime + timelineRange.startValue
> > +            timelineRuler.selectionEndTime = timelineRuler.zeroTime + timelineRange.endValue
> 
> Style: Semicolon.
> 
> >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:754
> >> +            startValue += 1;    // Convert index to frame number.
> > 
> > One space.
> 
> Style: For a trailing line comment, one space after the semicolon is the
> norm. // Like so.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:762
> > +        this._timelineSelectionPathComponent.title = displayName;
> 
> I think this line is stale. There is setter for `title` in
> HierarchicalPathComponent, and nothing seems to use it. Also,
> HierarchicalPathComponent seem to use the displayName as the tooltip title
> on the element already.

Probably a copy-paste error. HierarchicalPathComponent has no `title` property.

> >> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:797
> >> +WebInspector.TimelineRange = class TimelineRange
> > 
> > Should be in a new file. One class per file.
> 
> Deserves its own file.
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:808
> > +    get startValue()
> > +    {
> > +        return this._startValue;
> > +    }
> 
> Style: Lately for trivial getters/setters we've been single lining them:
> 
>     // Public
> 
>     get startValue() { return this._startValue; }
>     set startValue(x) { this._startValue = x; }

[Matt Baker]

Comment on attachment 272166 [details]
[Patch] Proposed Fix

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

>>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:50
>>>> +        this._entireRecordingPathComponent = this._createTimelineRangePathComponent(WebInspector.UIString("Entire Recording"));
>>> 
>>> Nit: I think xenon suggested lowercasing the second word here: "Entire recording".
>> 
>> I sugested Entire Recording, which is what he did. The video he posted had recording lowercase. He corrected it for the patch.
> 
> Maybe we should remove this altogether. It's only used in the filter bar placeholder text now, and most (4 of 6) timeline views just return "Records".

Commented on the wrong line.

>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
>>> +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);
>> 
>> Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444.
> 
> We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?).

Let's remove this and just use "Filter Records". Most timeline views return "Records" anyway, and it's only used in the placeholder text now.

[Timothy Hatcher]

Comment on attachment 272166 [details]
[Patch] Proposed Fix

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

>>>> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:366
>>>> +            this._filterBarNavigationItem.filterBar.placeholder = WebInspector.UIString("Filter %s").format(timelineView.navigationSidebarTreeOutlineLabel);
>>> 
>>> Might add a FIXME here to localize whole strings sainted of using %s. Localizers like whole strings in cases line this so they can control it better. See rdar://problem/24206444.
>> 
>> We may want to rename "navigationSidebarTreeOutlineLabel" to something more appropriate now that there is no sidebar (right?).
> 
> Let's remove this and just use "Filter Records". Most timeline views return "Records" anyway, and it's only used in the placeholder text now.

Works for me.

[Matt Baker]

Created attachment 272224 [details]
[Patch] Proposed Fix

[Timothy Hatcher]

Comment on attachment 272224 [details]
[Patch] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TimelineIcons.css:87
> +    opacity: 0.6);

Stray ). Needs removed.

[Matt Baker]

Created attachment 272225 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 272695 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 272695 [details]
[Patch] Proposed Fix

Rejecting attachment 272695 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 272695, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
01f82785f6ce0adbc491c9dce28
r197476 = 5405850fc2be6a01894c329602aee0b564fa399b
r197478 = 0fff5f339ba9d4e782f7ae563b3f6c1f3457461f
r197479 = 176cf13651db31cb6f644f9960e78d2f305e8bf6
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.

Full output: http://webkit-queues.webkit.org/results/913120

[Matt Baker]

Created attachment 272709 [details]
[Patch] Proposed Fix

[Matt Baker]

Committed r197488: <https://trac.webkit.org/r197488>

[WebKit Commit Bot]

Comment on attachment 272709 [details]
[Patch] Proposed Fix

Rejecting attachment 272709 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 272709, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
a/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Parsed 11 diffs from patch file(s).
patching file Source/WebInspectorUI/ChangeLog
Hunk #1 succeeded at 1 with fuzz 1.
Original content of Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 283.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/913837