Bug 195079 - Web Inspector: Add a new Scanner TimelineMarker to show up when mousing over TimelineView graphs
Summary: Web Inspector: Add a new Scanner TimelineMarker to show up when mousing over ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks: 194455
  Show dependency treegraph
 
Reported: 2019-02-26 16:48 PST by Joseph Pecoraro
Modified: 2019-03-04 12:08 PST (History)
5 users (show)

See Also:


Attachments
[IMAGE] Light Mode (816.01 KB, image/png)
2019-02-26 16:49 PST, Joseph Pecoraro
no flags Details
[IMAGE] Dark Mode (832.59 KB, image/png)
2019-02-26 16:49 PST, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (15.19 KB, patch)
2019-02-26 16:53 PST, Joseph Pecoraro
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
[Image] Marker over/under (21.89 KB, image/png)
2019-02-26 17:39 PST, Nikita Vasilyev
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-02-26 16:48:40 PST
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.
Comment 1 Joseph Pecoraro 2019-02-26 16:49:04 PST
Created attachment 363037 [details]
[IMAGE] Light Mode
Comment 2 Joseph Pecoraro 2019-02-26 16:49:15 PST
Created attachment 363038 [details]
[IMAGE] Dark Mode
Comment 3 Joseph Pecoraro 2019-02-26 16:49:27 PST
Images are when I'm hovering a 3s point in one of the sub-graphs.
Comment 4 Joseph Pecoraro 2019-02-26 16:53:51 PST
Created attachment 363040 [details]
[PATCH] Proposed Fix
Comment 5 Devin Rousso 2019-02-26 17:16:20 PST
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 :(
Comment 6 Nikita Vasilyev 2019-02-26 17:39:46 PST
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.
Comment 7 Nikita Vasilyev 2019-02-26 17:59:59 PST
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");
Comment 8 Devin Rousso 2019-02-26 18:58:23 PST
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.
Comment 9 Nikita Vasilyev 2019-02-27 11:54:51 PST
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.
Comment 10 Nikita Vasilyev 2019-02-27 12:15:03 PST
(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
Comment 11 Joseph Pecoraro 2019-02-27 22:55:17 PST
(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?
Comment 12 Joseph Pecoraro 2019-02-27 22:55:28 PST
<https://trac.webkit.org/r242194>
Comment 13 Radar WebKit Bug Importer 2019-02-28 00:15:28 PST
<rdar://problem/48466055>
Comment 14 Devin Rousso 2019-02-28 09:48:27 PST
(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).
Comment 15 Joseph Pecoraro 2019-03-04 11:55:46 PST
> > > 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!
Comment 16 Joseph Pecoraro 2019-03-04 11:56:22 PST
(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"
Comment 17 Devin Rousso 2019-03-04 12:08:27 PST
(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.