Summary: | Web Inspector: Timelines UI redesign: add the timelines tree outline to the TimelineOverview | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
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: | 153146, 154000 | ||||||||||||||||||
Bug Blocks: | 153036, 153904 | ||||||||||||||||||
Attachments: |
|
Description
Matt Baker
2016-01-12 12:50:24 PST
Created attachment 270473 [details]
[Patch] Proposed Fix
Comment on attachment 270473 [details] [Patch] Proposed Fix Waiting on other UI patches. See blocking issues for https://bugs.webkit.org/show_bug.cgi?id=153036. Comment on attachment 270473 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=270473&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:52 > - height: 29px; > + height: 23px; Can you see what this does to the MemoryTimelineView? I think this will break positioning of things. Created attachment 270707 [details]
[Patch] Proposed Fix
Created attachment 270712 [details]
[Image] New UI
Shown with navigation sidebar collapsed.
Comment on attachment 270707 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=270707&action=review r- for the questions raised below. > Source/WebInspectorUI/ChangeLog:14 > + Web Inspector: Timelines UI redesign: add the timelines tree outline to the TimelineOverview > + https://bugs.webkit.org/show_bug.cgi?id=153034 > + <rdar://problem/24195628> > + > + Reviewed by NOBODY (OOPS!). > + > + Move the Timelines tree outline to the TimelineRecordingContentView, in preparation > + for removing the navigation sidebar in <https://webkit.org/b/153036>. > + > + TimelineOverview dispatches two new events, GraphAdded and GraphRemoved. This was > + added so the TimelineRecordingContent can set the height of timeline tree elements > + to match the height of the corresponding TimelineOverGraph. The title says "add timelines tree outline to TimelineOverview" but it is instead adding it to TimelineRecordingContentView. TimelineOverview, already knows about the graphs. And TimelineOverviewGraph already knows its height. So putting it in TimelineOverview sounds like it would have avoided these GraphAdded / GraphRemoved events. Why did you end up putting the TreeOutline in the RecordingContentView instead of the Overview? > Source/WebInspectorUI/ChangeLog:24 > + * UserInterface/Views/LayoutTimelineOverviewGraph.js: > + (WebInspector.LayoutTimelineOverviewGraph): > + * UserInterface/Views/MemoryTimelineOverviewGraph.js: > + (WebInspector.MemoryTimelineOverviewGraph): > + (WebInspector.MemoryTimelineOverviewGraph.prototype.layout): > + (WebInspector.MemoryTimelineOverviewGraph.prototype._visibleRecords): > + * UserInterface/Views/NetworkTimelineOverviewGraph.js: > + (WebInspector.NetworkTimelineOverviewGraph): > + Refactoring for new TimelineOverviewGraph timeline property. All of this could be in a standalone patch separate from this one. It would have made reviewing this one simpler. > Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:28 > - constructor(timelineOverview) > + constructor(timeline, timelineOverview) Hmm, this implies that a TimelineOverviewGraph will only have a single timeline. In practice, that is currently the case. Maybe we should strive to keep it that way. There is certainly a lot of code right now assuming it, so this seems fine. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:54 > + WebInspector.TimelineOverview.addEventListener(WebInspector.TimelineOverview.Event.GraphAdded, this._timelineOverviewGraphAdded, this); This is listening for ANY timeline overview adding a graph. This includes a different RecordingContentView's TimelineOverview getting a graph. Likewise, it doesn't listen for graphs getting removed. It seems like it should. Ideally this listener would be added to the TimelineOverview instances. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:626 > + let height = this._timelineOverviewGraphHeightMap.get(timeline); > + if (height) { > + timelineTreeElement.listItemElement.style.height = height + "px"; > + this._timelineOverviewGraphHeightMap.delete(timeline); > + } This seems gross. It looks like the RecordingContentView is juggling the height value between GraphAdded and InstrumentAdded because it doesn't know both at the same time. Is that right? > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:647 > + this._timelineOverviewGrapHeightMap.delete(timeline); Typo in this variable name would throw exceptions. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:677 > + console.assert(timelineOverviewGraph.timelineOverview === this._currentTimelineOverview); Seems this assert would fire frequently when this TimelineRecordingContentView is hidden and a different TimelineRecordingContentView is active. Created attachment 271447 [details]
[Patch] Proposed Fix
Created attachment 271450 [details]
[Image] New overview UI (sidebar collapsed)
Comment on attachment 271447 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=271447&action=review > Source/WebInspectorUI/UserInterface/Views/Variables.css:37 > + --border-color: hsl(0, 0%, 70%); > + --border-inactive-color: hsl(0, 0%, 85%); > + --panel-background-color: hsl(0, 0%, 94%); Yes! Created attachment 271580 [details] Updated for Variables.css changes in https://webkit.org/b/154302 Created attachment 272694 [details]
[Patch] Proposed Fix
Comment on attachment 272694 [details] [Patch] Proposed Fix Clearing flags on attachment: 272694 Committed r197473: <http://trac.webkit.org/changeset/197473> All reviewed patches have been landed. Closing bug. |