Bug 147421 - Web Inspector: Filtered frames should be styled differently in the Rendering Frames overview graph
Summary: Web Inspector: Filtered frames should be styled differently in the Rendering ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-29 14:51 PDT by Matt Baker
Modified: 2015-08-20 15:08 PDT (History)
11 users (show)

See Also:


Attachments
[Patch] Proposed Fix (12.29 KB, patch)
2015-08-19 15:41 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Video] Duration filters applied (2.20 MB, video/quicktime)
2015-08-19 15:43 PDT, Matt Baker
no flags Details
[Patch] Proposed Fix (12.55 KB, patch)
2015-08-20 14:19 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Baker 2015-07-29 14:51:49 PDT
* 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.
Comment 1 Radar WebKit Bug Importer 2015-07-29 14:52:13 PDT
<rdar://problem/22059978>
Comment 2 Matt Baker 2015-08-19 15:41:38 PDT
Created attachment 259418 [details]
[Patch] Proposed Fix
Comment 3 Matt Baker 2015-08-19 15:43:02 PDT
Created attachment 259419 [details]
[Video] Duration filters applied
Comment 4 Devin Rousso 2015-08-19 18:39:12 PDT
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 5 Timothy Hatcher 2015-08-20 10:22:40 PDT
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!
Comment 6 Matt Baker 2015-08-20 11:56:03 PDT
(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.
Comment 7 Matt Baker 2015-08-20 14:19:27 PDT
Created attachment 259494 [details]
[Patch] Proposed Fix
Comment 8 WebKit Commit Bot 2015-08-20 15:08:09 PDT
Comment on attachment 259494 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 259494

Committed r188708: <http://trac.webkit.org/changeset/188708>
Comment 9 WebKit Commit Bot 2015-08-20 15:08:14 PDT
All reviewed patches have been landed.  Closing bug.