Bug 194110 – Web Inspector: Timeline graphs have drawing issues with multiple discontinuities

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

[Joseph Pecoraro]

Created attachment 360767 [details]
[IMAGE] Issue

Timeline graphs have drawing issues with multiple discontinuities

Steps to Reproduce:
1. Inspect this page
2. Show Memory timeline
3. Start and stop recording rapidly
  => Drawing issues

Notes:
• Happens when there are multiple discontinuities between valid records, since only 1 is taken into account at a time

[Radar WebKit Bug Importer]

<rdar://problem/47714356>

[Joseph Pecoraro]

Created attachment 360782 [details]
[PATCH] Proposed Fix

[Devin Rousso]

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

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

rs=me, with some issues/questions

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:145
>              dataPoints.push({time, size: usage});

Unrelated: should we be using `size` here?  What about `mainThreadUsage` and `nonMainThreadUsage`?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:203
> +            insertDiscontinuity.call(this, previousRecord, discontinuities[0], discontinuities[0], null);

Is it necessary to also update the equivalent of this in >CPUTimelineView.js:151?

> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:-252
> -            let size = new WI.Size(xScale(graphEndTime), memoryCategoryViewHeight);

Why did this move?  We need `size` to be defined in `yScale`.

[Devin Rousso]

Does this resolve <https://webkit.org/b/158195>?

[Joseph Pecoraro]

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

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

>> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:145
>>              dataPoints.push({time, size: usage});
> 
> Unrelated: should we be using `size` here?  What about `mainThreadUsage` and `nonMainThreadUsage`?

Oops, y es mainThread/nonMainThread come in a future patch, so I shouldn't have those here!

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineOverviewGraph.js:203
>> +            insertDiscontinuity.call(this, previousRecord, discontinuities[0], discontinuities[0], null);
> 
> Is it necessary to also update the equivalent of this in >CPUTimelineView.js:151?

Technically it doesn't matter at this point. At this point in both files we are past the last visible record. So 1 or more discontinuities do not matter, we just draw down to zero for the next discontinuity and stop.

>> Source/WebInspectorUI/UserInterface/Views/MemoryTimelineView.js:-252
>> -            let size = new WI.Size(xScale(graphEndTime), memoryCategoryViewHeight);
> 
> Why did this move?  We need `size` to be defined in `yScale`.

The position of the function doesn't matter, but I can move this back.

[Joseph Pecoraro]

https://trac.webkit.org/changeset/240867/webkit