* SUMMARY Filtered frames should be styled differently in the Rendering Frames overview graph. Frames are excluded from the tree/grid in four ways: 1) Ruler selection 2) Filter text 3) Filter by duration (https://bugs.webkit.org/show_bug.cgi?id=147419) 4) Filter by task type (https://bugs.webkit.org/show_bug.cgi?id=147389) In cases 2-4, we should change the style of the frames in the overview graph to reflect the filtered state. We could make them hollow, or increase the brightness or decrease opacity to make them less prominent.
<rdar://problem/22059978>
Created attachment 259418 [details] [Patch] Proposed Fix
Created attachment 259419 [details] [Video] Duration filters applied
Comment on attachment 259418 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259418&action=review > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:533 > + var wasHidden = currentTreeElement.hidden; (super NIT) I would rename this to something like "currentTreeElementWasHidden" to provide better clarity, because "wasHidden" could mean a bunch of different things. Also, let instead of var. > Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:537 > currentTreeElement = currentTreeElement.traverseNextTreeElement(false, null, dontPopulate); I would put an extra newline above this to be clear that this line isn't part of the if statement above. > Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.js:87 > + var startIndex = Math.floor(this.startTime); let instead of var (also line 88 and 92). > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:286 > + var overviewGraph = this._timelineOverviewGraphsMap.get(timeline); let instead of var. > Source/WebInspectorUI/UserInterface/Views/TimelineRecordFrame.js:71 > + return this._element.classList.contains("filtered"); We are trying to move away from this style of state tracking, so please move this into a member called something like "this._filtered". > Source/WebInspectorUI/UserInterface/Views/TimelineSidebarPanel.js:413 > + var records; let instead of var (line 410, 419, and 420).
Comment on attachment 259418 [details] [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259418&action=review >> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:533 >> + var wasHidden = currentTreeElement.hidden; > > (super NIT) I would rename this to something like "currentTreeElementWasHidden" to provide better clarity, because "wasHidden" could mean a bunch of different things. > Also, let instead of var. Should be const even. >> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.js:87 >> + var startIndex = Math.floor(this.startTime); > > let instead of var (also line 88 and 92). const!
(In reply to comment #5) > Comment on attachment 259418 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259418&action=review > > >> Source/WebInspectorUI/UserInterface/Views/NavigationSidebarPanel.js:533 > >> + var wasHidden = currentTreeElement.hidden; > > > > (super NIT) I would rename this to something like "currentTreeElementWasHidden" to provide better clarity, because "wasHidden" could mean a bunch of different things. > > Also, let instead of var. > > Should be const even. > > >> Source/WebInspectorUI/UserInterface/Views/RenderingFrameTimelineOverviewGraph.js:87 > >> + var startIndex = Math.floor(this.startTime); > > > > let instead of var (also line 88 and 92). > > const! To be clear, do we now prefer let when making any changes to a file, no matter how small? It seems odd to mix var/let within a file, and even more so within a single function.
Created attachment 259494 [details] [Patch] Proposed Fix
Comment on attachment 259494 [details] [Patch] Proposed Fix Clearing flags on attachment: 259494 Committed r188708: <http://trac.webkit.org/changeset/188708>
All reviewed patches have been landed. Closing bug.