Summary: | Web Inspector: Regression (r270134) Timeline recordings 2 and beyond do not show a timescale. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Patrick Angle <pangle> | ||||||||
Component: | Web Inspector | Assignee: | Patrick Angle <pangle> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ews-watchlist, hi, inspector-bugzilla-changes, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Patrick Angle
2021-03-08 12:09:25 PST
Created attachment 424585 [details]
Patch v1.0
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 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). Created attachment 424610 [details]
Patch v1.1 - Revised approach
mac-AS-debug-wk2 test failure is historical flakey crash being addressed in bug 222277. 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 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. Created attachment 424788 [details]
Patch v1.2 - Use didInitialLayout instead of isAttached
Committed r275337: <https://commits.webkit.org/r275337> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424788 [details]. |