Bug 195079 – Web Inspector: Add a new Scanner TimelineMarker to show up when mousing over TimelineView graphs

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

Add a new Scanner TimelineMarker to show up when mousing over TimelineView graphs

The idea here is to correlate something in a timeline graph with the events in the overview.

[Joseph Pecoraro]

Created attachment 363037 [details]
[IMAGE] Light Mode

[Joseph Pecoraro]

Created attachment 363038 [details]
[IMAGE] Dark Mode

[Joseph Pecoraro]

Images are when I'm hovering a 3s point in one of the sub-graphs.

[Joseph Pecoraro]

Created attachment 363040 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 363040 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363040&action=review

rs=me

> Source/WebInspectorUI/ChangeLog:25
> +
> +

Style: extra newline.

> Source/WebInspectorUI/ChangeLog:46
> +        New events that a TimelineView can dispatch to update the overviews.

s/overviews/overview, as there should only be one overview.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:200
>  Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithClass",

This could be reimplemented in terms of `enclosingNodeOrSelfWithClassInArray` (just like how `enclosingNodeOrSelfWithNodeNameInArray` is used by `enclosingNodeOrSelfWithNodeName`).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:725
> +        let chartElement = event.target.enclosingNodeOrSelfWithClassInArray(["area-chart", "stacked-area-chart", "range-chart"]);

You should add something in the ChangeLog (or a comment) explaining why this was necessary (the whole 1px border thing you explained to me in person).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:840
> +        let secondsPerPixel = this._timelineRuler.secondsPerPixel;

NIT: you could inline this.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:842
> +

Style: extra newline.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:305
> +        let secondsPerPixel = this._timelineRuler.secondsPerPixel;

NIT: you could inline this.

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:307
> +

Style: extra newline.

> Source/WebInspectorUI/UserInterface/Views/ShaderProgramContentView.css:-31
> -    --border-start-style: 1px solid lightgrey;;

lol

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordingContentView.js:863
> +        if (!this.visible)
> +            return;

We should probably still clear the scanner if it's been requested, regardless of whether we're visible or not.  Otherwise, we may have a case where switching tabs, clearing, and going back to the first tab would still show a scanner.

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.css:200
> +.timeline-ruler > .markers > .marker.scanner {

Do we want this marker to appear above any timeline record "bubbles"?

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:379
> +        if (this._scannerMarker)

NIT: this `if` is unnecessary, as either outcome will guarantee a `null` value (might as well remove the `if` and simplify the code path/readability a bit).

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:400
> +        if (this._scannerMarker)

The other `clear*` functions in this class actually remove/"destruct" objects, but this one doesn't.  It seems like the naming isn't entirely consistent.  I'd suggest `resetScanner` or `hideScanner`, or actually make `clearScanner` remove the element and set the variable to `null`.

> Source/WebInspectorUI/UserInterface/Views/TimelineRuler.js:401
> +            this._scannerMarker.time = -1;

This is because `WI.TimelineMarker.prototype.set time` has a fallback of `0` if the value is falsy?

> Source/WebInspectorUI/UserInterface/Views/TimelineView.js:341
> +    ScannerTimeDidChange: "timeline-view-scanner-time-did-change",
> +    ScannerDidClear: "timeline-view-scanner-did-clear",

Please, I beg of you, don't do this 😭

Don't put "did" or "was" or any other unnecessary verb in front of a perfectly valid past tense verb (e.g. "Changed" or "Cleared").  It's really not necessary :(

[Nikita Vasilyev]

Created attachment 363046 [details]
[Image] Marker over/under

This marker (and other markers, such as DOMContentLoaded, loaded, and etc.) is above the CPU bars but below the bars of the other graphs.

The gray color of the marker has a very similar luminosity to the light green of the CPU graph.

I still think it would be better to have the marker be semi-transparent and always on top. This new marker could be a semi-transparent white on Dark Mode and semi-transparent black on Light Mode.

[Nikita Vasilyev]

Comment on attachment 363040 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363040&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:198
> +Object.defineProperty(Node.prototype, "enclosingNodeOrSelfWithClassInArray",
> +{
> +    value(classNames)
> +    {
> +        for (let node = this; node; node = node.parentElement) {
> +            if (node.nodeType === Node.ELEMENT_NODE) {
> +                for (let className of classNames) {
> +                    if (node.classList.contains(className))
> +                        return node;
> +                }                
> +            }
> +        }
> +
> +        return null;
> +    }
> +});

Please use `closest` instead.
https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:725
>> +        let chartElement = event.target.enclosingNodeOrSelfWithClassInArray(["area-chart", "stacked-area-chart", "range-chart"]);
> 
> You should add something in the ChangeLog (or a comment) explaining why this was necessary (the whole 1px border thing you explained to me in person).

let chartElement = event.target.closest(".area-chart, .stacked-area-chart, .range-chart");

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:285
> +        let chartElement = event.target.enclosingNodeOrSelfWithClassInArray(["area-chart", "stacked-area-chart"]);

let chartElement = event.target.closest(".area-chart, .stacked-area-chart, .range-chart");

[Devin Rousso]

Comment on attachment 363040 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363040&action=review

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:198
>> +});
> 
> Please use `closest` instead.
> https://developer.mozilla.org/en-US/docs/Web/API/Element/closest

I totally forgot about this!  We should change all of the functions like this (e.g. `enclosingNodeOrSelfWithNodeName, `enclosingNodeOrSelfWithNodeNameInArray`, and `enclosingNodeOrSelfWithClass`) to use this.

[Nikita Vasilyev]

Comment on attachment 363040 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=363040&action=review

> Source/WebInspectorUI/ChangeLog:18
> +        (WI.CPUTimelineView.prototype._handleGraphMouseMove):        

Trailing white-space at the end of the line

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:192
> +                }                

Trailing white-space.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 363040 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363040&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Base/Utilities.js:198
> >> +});
> > 
> > Please use `closest` instead.
> > https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
> 
> I totally forgot about this!  We should change all of the functions like
> this (e.g. `enclosingNodeOrSelfWithNodeName,
> `enclosingNodeOrSelfWithNodeNameInArray`, and
> `enclosingNodeOrSelfWithClass`) to use this.

Bug 173747 - Web Inspector: Use Element.closest for internal code

[Joseph Pecoraro]

(In reply to Nikita Vasilyev from comment #6)
> Created attachment 363046 [details]
> [Image] Marker over/under
> 
> This marker (and other markers, such as DOMContentLoaded, loaded, and etc.)
> is above the CPU bars but below the bars of the other graphs.
> 
> The gray color of the marker has a very similar luminosity to the light
> green of the CPU graph.
> 
> I still think it would be better to have the marker be semi-transparent and
> always on top. This new marker could be a semi-transparent white on Dark
> Mode and semi-transparent black on Light Mode.

I agree. This is something that I believe Devin changed. Devin still feel its good to hide behind?

[Joseph Pecoraro]

<https://trac.webkit.org/r242194>

[Radar WebKit Bug Importer]

<rdar://problem/48466055>

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #11)
> (In reply to Nikita Vasilyev from comment #6)
> > Created attachment 363046 [details]
> > [Image] Marker over/under
> > 
> > This marker (and other markers, such as DOMContentLoaded, loaded, and etc.) is above the CPU bars but below the bars of the other graphs.
> > 
> > The gray color of the marker has a very similar luminosity to the light green of the CPU graph.
> > 
> > I still think it would be better to have the marker be semi-transparent and always on top. This new marker could be a semi-transparent white on Dark Mode and semi-transparent black on Light Mode.
> 
> I agree. This is something that I believe Devin changed. Devin still feel its good to hide behind?
When zoomed out very far, such that the records are smaller, having a line (even a semi-transparent one) will possibly obstruct what's underneath, making it harder to see (and therefore select) a particular record in that area.  Given that there is a lot of empty/extra space above/below all records (assuming that there even is one), it's still easy to see where the line would go (not to mention the little V in the timing header area).

[Joseph Pecoraro]

> > > I still think it would be better to have the marker be semi-transparent and always on top. This new marker could be a semi-transparent white on Dark Mode and semi-transparent black on Light Mode.
> > 
> > I agree. This is something that I believe Devin changed. Devin still feel its good to hide behind?
> When zoomed out very far, such that the records are smaller, having a line
> (even a semi-transparent one) will possibly obstruct what's underneath,
> making it harder to see (and therefore select) a particular record in that
> area.

We should be able to make clicks go through toolbars.

Also if zoomed out very far the events are merged so much that bubble timings are enormous ranges. If 3 pixels spans even half a second then accuracy is just about gone!

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #15)
> > > > I still think it would be better to have the marker be semi-transparent and always on top. This new marker could be a semi-transparent white on Dark Mode and semi-transparent black on Light Mode.
> > > 
> > > I agree. This is something that I believe Devin changed. Devin still feel its good to hide behind?
> > When zoomed out very far, such that the records are smaller, having a line
> > (even a semi-transparent one) will possibly obstruct what's underneath,
> > making it harder to see (and therefore select) a particular record in that
> > area.
> 
> We should be able to make clicks go through toolbars.

Ugh, not "toolbars"... "to bars"

[Devin Rousso]

(In reply to Joseph Pecoraro from comment #15)
> > > > I still think it would be better to have the marker be semi-transparent and always on top. This new marker could be a semi-transparent white on Dark Mode and semi-transparent black on Light Mode.
> > > 
> > > I agree. This is something that I believe Devin changed. Devin still feel its good to hide behind?
> > When zoomed out very far, such that the records are smaller, having a line (even a semi-transparent one) will possibly obstruct what's underneath, making it harder to see (and therefore select) a particular record in that area.
> 
> We should be able to make clicks go through to bars.
I think this already should work.  If not, it should be as simple as setting `pointer-events: none;` on the marker element (or it's container, depending on the `z-index` awfulness).

> Also if zoomed out very far the events are merged so much that bubble timings are enormous ranges. If 3 pixels spans even half a second then accuracy is just about gone!
I would agree in that case, but I think this may be for that "just right" scenario where there is a short-lived record (e.g. a "quick" script evaluation) that is partially/mostly obstructed by the marker.  I generally believe that we should avoid putting things over other things unless absolutely necessary, as there's always going to be some case where we get it wrong.