Bug 153033

Summary: Web Inspector: Timelines UI redesign: replace content view container with a content browser
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 154561, 154644    
Bug Blocks: 153036    
Attachments:
Description Flags
[Video] selection path components UI
none
[Patch] Proposed Fix
none
[Patch] Proposed Fix
none
[Patch] Proposed Fix
none
[Patch] Proposed Fix
none
[Patch] Proposed Fix commit-queue: commit-queue-

Description Matt Baker 2016-01-12 12:50:18 PST
* 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.
Comment 1 Radar WebKit Bug Importer 2016-01-14 14:32:54 PST
<rdar://problem/24195565>
Comment 2 Matt Baker 2016-02-24 14:50:34 PST
Created attachment 272150 [details]
[Video] selection path components UI
Comment 3 Matt Baker 2016-02-24 17:54:14 PST
Created attachment 272166 [details]
[Patch] Proposed Fix
Comment 4 Timothy Hatcher 2016-02-24 20:00:03 PST
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.
Comment 5 Joseph Pecoraro 2016-02-24 20:01:29 PST
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; }
Comment 6 Timothy Hatcher 2016-02-24 20:05:14 PST
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.
Comment 7 Matt Baker 2016-02-25 11:37:16 PST
(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.
Comment 8 Matt Baker 2016-02-25 11:47:53 PST
(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; }
Comment 9 Matt Baker 2016-02-25 11:51:42 PST
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.
Comment 10 Timothy Hatcher 2016-02-25 12:00:09 PST
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.
Comment 11 Matt Baker 2016-02-25 12:21:08 PST
Created attachment 272224 [details]
[Patch] Proposed Fix
Comment 12 Timothy Hatcher 2016-02-25 12:24:56 PST
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.
Comment 13 Matt Baker 2016-02-25 12:28:53 PST
Created attachment 272225 [details]
[Patch] Proposed Fix
Comment 14 Matt Baker 2016-03-02 15:30:18 PST
Created attachment 272695 [details]
[Patch] Proposed Fix
Comment 15 WebKit Commit Bot 2016-03-02 17:00:18 PST
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
Comment 16 Matt Baker 2016-03-02 17:18:21 PST
Created attachment 272709 [details]
[Patch] Proposed Fix
Comment 17 Matt Baker 2016-03-02 19:52:55 PST
Committed r197488: <https://trac.webkit.org/r197488>
Comment 18 WebKit Commit Bot 2016-03-02 20:30:09 PST
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