[webkit-reviews] review granted: [Bug 195079] Web Inspector: Add a new Scanner TimelineMarker to show up when mousing over TimelineView graphs : [Attachment 363040] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195079: Web Inspector: Add a new Scanner TimelineMarker to show up when
mousing over TimelineView graphs
https://bugs.webkit.org/show_bug.cgi?id=195079

Attachment 363040: [PATCH] Proposed Fix

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363040
  --> https://bugs.webkit.org/attachment.cgi?id=363040
[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
:(