Bug 222930 - Web Inspector: Regression (r270134) Timeline recordings 2 and beyond do not show a timescale.
Summary: Web Inspector: Regression (r270134) Timeline recordings 2 and beyond do not s...
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: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-08 12:09 PST by Patrick Angle
Modified: 2021-03-31 19:02 PDT (History)
4 users (show)

See Also:


Attachments
Patch v1.0 (1.91 KB, patch)
2021-03-29 14:52 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.1 - Revised approach (2.44 KB, patch)
2021-03-29 18:56 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Patch v1.2 - Use didInitialLayout instead of isAttached (2.49 KB, patch)
2021-03-31 11:07 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-03-08 12:09:25 PST
Steps to reproduce:
1. Inspect `webkit.org`
2. Go to the timelines tab and have Automatically stop... enabled.
3. Refresh the page to take a brief recording.
4. Refresh the page again to take a second recording.

Results:
The second timeline recording has an empty timescale bar (the area with time/tic marks above timeline graphs for each recording type).
Comment 1 Radar WebKit Bug Importer 2021-03-15 13:10:15 PDT
<rdar://problem/75443284>
Comment 2 Patrick Angle 2021-03-29 14:52:42 PDT
Created attachment 424585 [details]
Patch v1.0
Comment 3 Devin Rousso 2021-03-29 15:15:41 PDT
Comment on attachment 424585 [details]
Patch v1.0

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

> Source/WebInspectorUI/ChangeLog:9
> +        Starting with r270134, when a new `TimelineOverview` with a `TimelineRuler` is created, the order of operations
> +        no longer causes `WI.TimelineRuler.__prototype__.sizeDidChange` to be invoked in the initial layout of the view,

Really??!?  This seems like a deeper bug than just `WI.TimelineOverview` then.  `sizeDidChange` should always be called for the initial layout (see bug 196901).  Maybe you can explain a bit more of the "order of operations"? 

NIT: this should just be `.prototype.`
Comment 4 Patrick Angle 2021-03-29 17:07:52 PDT
Comment on attachment 424585 [details]
Patch v1.0

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

>> Source/WebInspectorUI/ChangeLog:9
>> +        no longer causes `WI.TimelineRuler.__prototype__.sizeDidChange` to be invoked in the initial layout of the view,
> 
> Really??!?  This seems like a deeper bug than just `WI.TimelineOverview` then.  `sizeDidChange` should always be called for the initial layout (see bug 196901).  Maybe you can explain a bit more of the "order of operations"? 
> 
> NIT: this should just be `.prototype.`

Looked further into this: sizeDidChange **is** called, but is called at a point where the underlying DOM element is not attached anywhere, so the clientWidth reads as zero. After further digging, this issue is caused by `TimelineOverview.prototype._viewModeDidChange`  which updates the layout of it's children and itself before being attached, which consumes the opportunity for sizeDidChange to be called on the actual initial layout where the TimelineRuler's element is attached to the DOM, since sizeDidChange is only invoked on the initial layout, or as the result of a resize layout (attaching a subview counts instead as a `LayoutReason.Dirty` layout).
Comment 5 Patrick Angle 2021-03-29 18:56:57 PDT
Created attachment 424610 [details]
Patch v1.1 - Revised approach
Comment 6 Patrick Angle 2021-03-30 08:51:23 PDT
mac-AS-debug-wk2 test failure is historical flakey crash being addressed in bug 222277.
Comment 7 Devin Rousso 2021-03-30 14:56:44 PDT
Comment on attachment 424610 [details]
Patch v1.1 - Revised approach

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

r=me

> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:856
> +        if (this.isAttached)

I wonder if maybe we should also/instead check `this.didInitialLayout` 🤔

Just a thought.  No need to change it if you'd rather not introduce more variables :)
Comment 8 Patrick Angle 2021-03-30 15:33:55 PDT
Comment on attachment 424610 [details]
Patch v1.1 - Revised approach

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

>> Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:856
>> +        if (this.isAttached)
> 
> I wonder if maybe we should also/instead check `this.didInitialLayout` 🤔
> 
> Just a thought.  No need to change it if you'd rather not introduce more variables :)

It's not a bad idea, since otherwise it is possible to attach the TimelineOverview somewhere and then change the view mode, which would force layout immediately, instead of when it is already scheduled.
Comment 9 Patrick Angle 2021-03-31 11:07:50 PDT
Created attachment 424788 [details]
Patch v1.2 - Use didInitialLayout instead of isAttached
Comment 10 EWS 2021-03-31 19:02:52 PDT
Committed r275337: <https://commits.webkit.org/r275337>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424788 [details].