Bug 197440 – Web Inspector: Timelines: CPU/memory timeline bars sometimes don't draw correctly and jump around on scrolling

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

[Devin Rousso]

# STEPS TO REPRODUCE:
1. inspect any page with some activity
2. enable the CPU/memory timeline
3. start a timeline recording
4. scroll/interact with the page to capture activity
5. stop the timeline recording
6. try zooming in/our or scrolling left/right
 => CPU "columns" may appear/disappear if the record is mostly offscreen
 => the memory graph may suddenly update when the next record becomes visible (e.g. the slope of the graph changes)

[Devin Rousso]

<rdar://problem/46886315>

[Devin Rousso]

Created attachment 368617 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 368617 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/Timeline.js:122
> +        return this._records.slice(constrain(lowerIndex - 1), constrain(upperIndex + 1));

It sounds like records overlapping time range now include one record before and after both ends?

That is not what I'd expect in some cases. Sometimes we use this to get an exact count of the records in the range. If we are including the surrounding records that are not overlapping then that is bad.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1270
> -        let networkRecords = networkTimeline ? networkTimeline.recordsInTimeRange(startTime, endTime) : [];
> +        let networkRecords = networkTimeline ? networkTimeline.recordsOverlappingTimeRange(startTime, endTime) : [];

Does this now include records outside the range? We use this for a count.

[Devin Rousso]

Created attachment 369026 [details]
Patch

[Devin Rousso]

Created attachment 369033 [details]
Patch

[EWS Watchlist]

Comment on attachment 369033 [details]
Patch

Attachment 369033 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12096164

New failing tests:
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html
security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html

[EWS Watchlist]

Created attachment 369053 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Joseph Pecoraro]

Comment on attachment 369033 [details]
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.

[Devin Rousso]

Created attachment 370184 [details]
Patch

[Devin Rousso]

Created attachment 370186 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370186 [details]
Patch

Clearing flags on attachment: 370186

Committed r245498: <https://trac.webkit.org/changeset/245498>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.