Bug 222930 – Web Inspector: Regression (r270134) Timeline recordings 2 and beyond do not show a timescale.

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

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).

[Radar WebKit Bug Importer]

<rdar://problem/75443284>

[Patrick Angle]

Created attachment 424585 [details]
Patch v1.0

[Devin Rousso]

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.`

[Patrick Angle]

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).

[Patrick Angle]

Created attachment 424610 [details]
Patch v1.1 - Revised approach

[Patrick Angle]

mac-AS-debug-wk2 test failure is historical flakey crash being addressed in bug 222277.

[Devin Rousso]

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 :)

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 424788 [details]
Patch v1.2 - Use didInitialLayout instead of isAttached

[EWS]

Committed r275337: <https://commits.webkit.org/r275337>

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