Bug 200637 - Web Inspector: REGRESSION: Inspector Timeline always starts at about 500ms mark on a reload
Summary: Web Inspector: REGRESSION: Inspector Timeline always starts at about 500ms ma...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-12 11:48 PDT by Joseph Pecoraro
Modified: 2019-09-11 23:55 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.41 KB, patch)
2019-09-11 20:53 PDT, Joseph Pecoraro
drousso: review-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (5.31 KB, patch)
2019-09-11 22:19 PDT, Joseph Pecoraro
drousso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-08-12 11:48:17 PDT
REGRESSION: Inspector Timeline always starts at about 500ms mark on a reload
Comment 1 Radar WebKit Bug Importer 2019-08-12 11:48:30 PDT
<rdar://problem/54218967>
Comment 2 Joseph Pecoraro 2019-09-11 20:53:03 PDT
I believe this was a regression from r245498, when the CPUTimelineRecord's startTime was moved back by the sampling rate (500ms).
Comment 3 Joseph Pecoraro 2019-09-11 20:53:18 PDT
Created attachment 378616 [details]
[PATCH] Proposed Fix
Comment 4 Devin Rousso 2019-09-11 21:32:59 PDT
Comment on attachment 378616 [details]
[PATCH] Proposed Fix

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

r-, explained below

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:108
> +    get unadjustedStartTime()

The name of this function doesn't seem accurate, or at the very least seems like a roundabout/circumstantial way of getting to the desired result.  The `timestamp` itself isn't guaranteed to be unadjusted either (I know it is right now, but that may not be the case for other future timeline record subclasses).

I think a better approach would be to check `if (record instanceof WI.CPUTimelineRecord || record instanceof WI.MemoryTimelineRecord)` in `_updateTimesIfNeeded` and use the `record.timestamp` in that case instead.
Comment 5 Joseph Pecoraro 2019-09-11 21:50:18 PDT
Comment on attachment 378616 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:108
>> +    get unadjustedStartTime()
> 
> The name of this function doesn't seem accurate, or at the very least seems like a roundabout/circumstantial way of getting to the desired result.  The `timestamp` itself isn't guaranteed to be unadjusted either (I know it is right now, but that may not be the case for other future timeline record subclasses).
> 
> I think a better approach would be to check `if (record instanceof WI.CPUTimelineRecord || record instanceof WI.MemoryTimelineRecord)` in `_updateTimesIfNeeded` and use the `record.timestamp` in that case instead.

I'm not sure I buy the argument.

That would mean:
  • The next time we add a timestamp based record we'd have to update that location as well
  • Each update call now has to do a chain of instanceof checks
  • Each TimelineRecord subclass can override unadjusted*Time as needed. So if "timestamp" was adjusted in some future class they would be able to override it to return sane values.

Would you rather I make CPUTimelineRecord / MemoryTimelineRecord provide their own unadjusted*Time (or a common TimestampTimelineRecord superclass) and just return timestamp?
Comment 6 Devin Rousso 2019-09-11 21:55:21 PDT
Comment on attachment 378616 [details]
[PATCH] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:108
>>> +    get unadjustedStartTime()
>> 
>> The name of this function doesn't seem accurate, or at the very least seems like a roundabout/circumstantial way of getting to the desired result.  The `timestamp` itself isn't guaranteed to be unadjusted either (I know it is right now, but that may not be the case for other future timeline record subclasses).
>> 
>> I think a better approach would be to check `if (record instanceof WI.CPUTimelineRecord || record instanceof WI.MemoryTimelineRecord)` in `_updateTimesIfNeeded` and use the `record.timestamp` in that case instead.
> 
> I'm not sure I buy the argument.
> 
> That would mean:
>   • The next time we add a timestamp based record we'd have to update that location as well
>   • Each update call now has to do a chain of instanceof checks
>   • Each TimelineRecord subclass can override unadjusted*Time as needed. So if "timestamp" was adjusted in some future class they would be able to override it to return sane values.
> 
> Would you rather I make CPUTimelineRecord / MemoryTimelineRecord provide their own unadjusted*Time (or a common TimestampTimelineRecord superclass) and just return timestamp?

Good point.  I like your suggestion.

How about something like:

TimelineRecord.js
```
    get unadjustedStartTime()
    {
        // Overridden by subclasses if needed.
        return this.startTime;
    }
```

CPUTimelineRecord.js
```
    get unadjustedStartTime()
    {
        return this.timestamp;
    }
```
Comment 7 Joseph Pecoraro 2019-09-11 22:19:20 PDT
Created attachment 378620 [details]
[PATCH] Proposed Fix
Comment 8 Devin Rousso 2019-09-11 23:18:17 PDT
Comment on attachment 378620 [details]
[PATCH] Proposed Fix

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

r=me, thanks for iterating :)

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:108
> +    get unadjustedStartTime()

We should move this to be next to `get activeStartTime`.

> Source/WebInspectorUI/UserInterface/Models/TimelineRecord.js:114
> +    get unadjustedEndTime()

We should move this to be right after `get endTime`.
Comment 9 Joseph Pecoraro 2019-09-11 23:55:52 PDT
https://trac.webkit.org/changeset/249799/webkit