[webkit-reviews] review granted: [Bug 135307] Web Inspector: Should be a way to go directly from an event in the overview view to the specialized timeline for that event : [Attachment 352122] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 135307: Web Inspector: Should be a way to go directly from an event in the
overview view to the specialized timeline for that event
https://bugs.webkit.org/show_bug.cgi?id=135307

Attachment 352122: Patch

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




--- Comment #5 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 352122
  --> https://bugs.webkit.org/attachment.cgi?id=352122
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:734
> +	       if (firstRecord.startTime < this.selectionStartTime ||
lastRecord.endTime > this.selectionStartTime + this.selectionDuration) {
> +		   let selectionPadding = this.secondsPerPixel * 10;

Neat!

> Source/WebInspectorUI/UserInterface/Views/TimelineOverviewGraph.js:40
>	   this._selectedRecord = null;
> -	   this._selectedRecordChanged = false;
> +	   this._selectedRecordBar = null;

It seems weird but possible that the selectedRecord and selectedRecordBar might
get out of sync. Can we add an assertion somewhere (such as inside of `set
selectedRecord` that the record is inside of the selectedRecordBar if that is
available)?

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.css:98
> +.timeline-record-bar:not(.selected).timeline-record-type-network > .segment
{

Can the selection colors above just be "!important" and avoid the
:not(.selected) classes? I think that is normally how we handle selection
styles.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:37
> +	   this._element.addEventListener("click",
this._handleClick.bind(this));

I wonder about the performance (CPU and Memory) different between doing a bind
like this versus doing an arrow function. I mused about this once over here:
https://bugs.webkit.org/show_bug.cgi?id=158315

Compare:

    this._element.addEventListener("click", this._handleClick.bind(this));
    this._element.addEventListener("click", (e) => { this._handleClick(e); });

This is in my head because:

    • Function.prototype.bind is a function call (maybe it gets optimized away
though?)
    • JSBoundFunction has extra memory (maybe this has been optimized away for
no-args as well?)

This might be a fun thing to measure some time in the future. It just always
jumps out at me in Timeline code for things we may be creating many many object
instances (like this), how much time we are likely spending inside of
Function.prototype.bind.

> Source/WebInspectorUI/UserInterface/Views/TimelineRecordBar.js:188
> +    set selected(selected)
> +    {
> +	   this._element.classList.toggle("selected", !!selected);
> +    }

We should add a getter just for consistency.