Bug 147421 – Web Inspector: Filtered frames should be styled differently in the Rendering Frames overview graph

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/22059978>

[Matt Baker]

Created attachment 259418 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 259419 [details]
[Video] Duration filters applied

[Devin Rousso]

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).

[Timothy Hatcher]

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!

[Matt Baker]

(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.

[Matt Baker]

Created attachment 259494 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 259494

Committed r188708: <http://trac.webkit.org/changeset/188708>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.