Bug 153532 – Web Inspector: Timelines UI redesign: relocate recording controls to TimelineTabContentView's content browser

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
Relocate recording controls to TimelineTabContentView's content browser. We might want to disable these controls when the content browser is showing a content view for a represented object other than a TimelineRecording/Timeline.

[Radar WebKit Bug Importer]

<rdar://problem/24361611>

[Matt Baker]

Created attachment 272667 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 272668 [details]
[Image] New navigation bar controls

Placement of the view mode controls (Timelines/Rendering Frames) is awkward without the sidebar. I tried a few styles and locations, without arriving at an ideal solution.

To expedite this patch the buttons were left much as they were when they lived in the sidebar. We can revisit this as the new Timelines UI matures.

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/UserInterface/Views/TabContentView.js:152
> +        this.restoreFromCookie(cookie);

Should this get the relaxMatchDelay passed in too?

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.css:29
> +/* remove me! */
> +.timeline.tab.content-view .navigation-bar > .item.toggle-navigation-sidebar.activate.button {
> +    display: none;
> +}

This should go away when WebInspector.ContentBrowserTabContentView no longer has a navigationSidebar.

The comment should be a FIXME.

[Timothy Hatcher]

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

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

> Source/WebInspectorUI/ChangeLog:15
> +        "Instruments" and "Frames". UI for switching modes isn't yet final, and the

Lets stick with "Timelines" and "Frames". Will talk offline.

[Joseph Pecoraro]

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

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

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:75
> +        this._viewMode = null;

Should we initialize the view mode based off of a setting? Or at least some default value?

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:373
> +        if (isCapturing) {
> +            WebInspector.timelineManager.stopCapturing();
> +        } else {

Nit: style, no braces

> Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:382
> +        let isCapturing = !WebInspector.timelineManager.isCapturing();

This statement reads weird since "isCapturing" is the opposite of isCapturing()". Perhaps this variable name should be "newCapturing" or something.

[Timothy Hatcher]

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

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

>> Source/WebInspectorUI/ChangeLog:15
>> +        "Instruments" and "Frames". UI for switching modes isn't yet final, and the
> 
> Lets stick with "Timelines" and "Frames". Will talk offline.

Lets go with "Events" and "Frames".

[Matt Baker]

(In reply to comment #4)
> Comment on attachment 272667 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272667&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TabContentView.js:152
> > +        this.restoreFromCookie(cookie);
> 
> Should this get the relaxMatchDelay passed in too?

The state saved by TimelineSidebarPanel (and now TimelineTabContentView) doesn't require it. This is only used by the NavigationSidebarPanel base class.

> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.css:29
> > +/* remove me! */
> > +.timeline.tab.content-view .navigation-bar > .item.toggle-navigation-sidebar.activate.button {
> > +    display: none;
> > +}
> 
> This should go away when WebInspector.ContentBrowserTabContentView no longer
> has a navigationSidebar.
> 
> The comment should be a FIXME.

This shouldn't have been included at all.

[Joseph Pecoraro]

> Lets go with "Events" and "Frames".

Maybe "Time" and "Frames"?

[Matt Baker]

(In reply to comment #6)
> Comment on attachment 272667 [details]
> [Patch] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272667&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:75
> > +        this._viewMode = null;
> 
> Should we initialize the view mode based off of a setting? Or at least some
> default value?

This is set by _displayedContentViewNavigationItemsDidChange in response to the first WebInspector.ContentView.Event.NavigationItemsDidChange event. If the value is non-null, the first call to _changeViewMode could return without setting up the view state, since this._viewMode === newViewMode could be true.

We could do this in the constructor before recordings are added:

const selectedByUser = false;
this._changeViewMode(WebInspector.TimelineOverview.ViewMode.Timelines, selectedByUser);

> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:373
> > +        if (isCapturing) {
> > +            WebInspector.timelineManager.stopCapturing();
> > +        } else {
> 
> Nit: style, no braces
> 
> > Source/WebInspectorUI/UserInterface/Views/TimelineTabContentView.js:382
> > +        let isCapturing = !WebInspector.timelineManager.isCapturing();
> 
> This statement reads weird since "isCapturing" is the opposite of
> isCapturing()". Perhaps this variable name should be "newCapturing" or
> something.

Let's rewrite as:
this._recordButton.toggled = !WebInspector.timelineManager.isCapturing();

[Matt Baker]

Created attachment 272696 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 272696

Committed r197479: <http://trac.webkit.org/changeset/197479>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.