[webkit-reviews] review granted: [Bug 197440] Web Inspector: Timelines: CPU/memory timeline bars sometimes don't draw correctly and jump around on scrolling : [Attachment 369033] 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 197440: Web Inspector: Timelines: CPU/memory timeline bars sometimes don't
draw correctly and jump around on scrolling
https://bugs.webkit.org/show_bug.cgi?id=197440

Attachment 369033: Patch

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




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

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

r=me

> Source/WebInspectorUI/UserInterface/Models/CPUTimeline.js:37
> +	   super.addRecord(record);
> +
> +	   if (lastRecord)
> +	       record.adjustStartTimeToLastRecord(lastRecord);

I don't think I like this approach of modifying the CPUTimelineRecord like
this. That said, it seems like an elegant solution. This means we are treating
records not as a Sample time but as a Range from the last timestamp to the
sample time. This changes the meaning of the model class slightly from my
mental model. I suppose it is fine because CPUTimelineRecord has `timestamp`
for the sample time and `start/end` may be used for other purposes.

Either way, I would expect the modification of the record to happen before
`super.addRecord`, thus before any observers would have seen the pre-modified
`startTime`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:450
> -	   // Don't include the record before the graph start if the graph
start is within a gap.
> -	   let includeRecordBeforeStart = !discontinuities.length ||
discontinuities[0].startTime > graphStartTime;
> -	   let visibleRecords =
this.representedObject.recordsInTimeRange(graphStartTime, visibleEndTime,
includeRecordBeforeStart);
> +	   let visibleRecords =
this.representedObject.recordsInTimeRange(graphStartTime, visibleEndTime, {
> +	       includeRecordBeforeStart: !discontinuities.length ||
discontinuities[0].startTime > graphStartTime,
> +	   });

Maybe this could include AfterEnd, and we would see the graph slope up/down to
the next entry that is just beyond the selection range. I don't think we do
that properly right now.

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

Oops.

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:787
> +	       if (startTime < this.selectionStartTime || endTime >
this.selectionStartTime + this.selectionDuration || firstRecord instanceof
WI.CPUTimelineRecord) {

Wrap the `endTime > (...)` in parens to improve readability.