Summary: | Web Inspector: Timelines UI redesign: relocate recording controls to TimelineTabContentView's content browser | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Baker <mattbaker> | ||||||||
Component: | Web Inspector | Assignee: | 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: | |||||||||||
Bug Blocks: | 153036 | ||||||||||
Attachments: |
|
Description
Matt Baker
2016-01-26 19:41:05 PST
Created attachment 272667 [details]
[Patch] Proposed Fix
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.
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. 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. 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. 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". (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. > Lets go with "Events" and "Frames".
Maybe "Time" and "Frames"?
(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(); Created attachment 272696 [details]
[Patch] Proposed Fix
Comment on attachment 272696 [details] [Patch] Proposed Fix Clearing flags on attachment: 272696 Committed r197479: <http://trac.webkit.org/changeset/197479> All reviewed patches have been landed. Closing bug. |