153392 2016-01-22 21:42:32 -0800 Web Inspector: Reduce unnecessary forced layouts in TimelineOverview 2016-01-25 11:49:00 -0800 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 joepeck joepeck bburg commit-queue graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1157960 0 joepeck 2016-01-22 21:42:32 -0800 * SUMMARY Reduce unnecessary forced layouts in TimelineOverview. It looks like autoscroll when not autoscrolling would set the scrollContainerElement.scrollLeft to 0 (which it already was) and force a synchronous layout. Seems we can just skip this work if the calculated scrollLeft value would have been 0. This eliminated the forced layouts I saw triggered by TimelineOverview recording reloads of numerous pages. 1157961 1 webkit-bug-importer 2016-01-22 21:42:49 -0800 <rdar://problem/24312344> 1157964 2 269646 joepeck 2016-01-22 21:44:00 -0800 Created attachment 269646 [PATCH] Proposed Fix 1157976 3 269646 timothy 2016-01-22 22:00:41 -0800 Comment on attachment 269646 [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=269646&action=review > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:390 > + if (scrollLeft) Does this need to be: scrollLeft || this._scrollContainerElement.scrollLeft That way if it is no zero already it will set it still. Or scrollLeft !== this._scrollContainerElement.scrollLeft 1158385 4 joepeck 2016-01-25 11:01:01 -0800 (In reply to comment #3) > Comment on attachment 269646 [details] > [PATCH] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=269646&action=review > > > Source/WebInspectorUI/UserInterface/Views/TimelineOverview.js:390 > > + if (scrollLeft) > > Does this need to be: scrollLeft || this._scrollContainerElement.scrollLeft > > That way if it is no zero already it will set it still. Or scrollLeft !== > this._scrollContainerElement.scrollLeft This was intentional. I do not want to actually call `element.scrollLeft` since that could force a layout. I don't think autoscroll will every intentionally scroll you to zero. Starting a new recording automatically resets the scroll position anyways. So this was a cheap hack, if the scrollLeft we calculate is zero, do no work. I didn't find any cases where this was a problem, and I saw fewer (zero) forced layouts caused by TimelineOverview in reload recordings that didn't cause the overview to auto-scroll. 1158424 5 269646 commit-queue 2016-01-25 11:48:55 -0800 Comment on attachment 269646 [PATCH] Proposed Fix Clearing flags on attachment: 269646 Committed r195544: <http://trac.webkit.org/changeset/195544> 1158426 6 commit-queue 2016-01-25 11:49:00 -0800 All reviewed patches have been landed. Closing bug. 269646 2016-01-22 21:44:00 -0800 2016-01-25 11:48:55 -0800 [PATCH] Proposed Fix timeline-overview.patch text/plain 1859 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDczZmMxNzUuLjZmNDIxYTUgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDE2LTAxLTIyICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKKyAgICAgICAgV2ViIEluc3BlY3RvcjogUmVkdWNl IHVubmVjZXNzYXJ5IGZvcmNlZCBsYXlvdXRzIGluIFRpbWVsaW5lT3ZlcnZpZXcKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MzM5MgorICAgICAgICA8 cmRhcjovL3Byb2JsZW0vMjQzMTIzNDQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lT3ZlcnZpZXcu anM6CisgICAgICAgIChXZWJJbnNwZWN0b3IuVGltZWxpbmVPdmVydmlldy5wcm90b3R5cGUubGF5 b3V0KToKKyAgICAgICAgSWdub3JlIHNldHRpbmcgdGhlIHNjcm9sbExlZnQgaWYgd2Ugd291bGQg YmUgc2V0dGluZyBpdCB0byAwLgorICAgICAgICBUaGlzIGhlbHBzIGF2b2lkIGEgZm9yY2VkIGxh eW91dCBpbiBjb21tb24gY2FzZXMuCisKKzIwMTYtMDEtMjIgIEpvc2VwaCBQZWNvcmFybyAgPHBl Y29yYXJvQGFwcGxlLmNvbT4KKwogICAgICAgICBXZWIgSW5zcGVjdG9yOiBSZWR1Y2UgdW5uZWNl c3NhcnkgZm9yY2VkIGxheW91dHMgaW4gVGltZWxpbmVSdWxlcgogICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTUzMzkwCiAgICAgICAgIDxyZGFyOi8vcHJv YmxlbS8yNDMxMjI0MT4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50 ZXJmYWNlL1ZpZXdzL1RpbWVsaW5lT3ZlcnZpZXcuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkv VXNlckludGVyZmFjZS9WaWV3cy9UaW1lbGluZU92ZXJ2aWV3LmpzCmluZGV4IDg2YmNlMzIuLjFm NGFmNGQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1Zp ZXdzL1RpbWVsaW5lT3ZlcnZpZXcuanMKKysrIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJ bnRlcmZhY2UvVmlld3MvVGltZWxpbmVPdmVydmlldy5qcwpAQCAtMzg2LDcgKzM4Niw5IEBAIFdl Ykluc3BlY3Rvci5UaW1lbGluZU92ZXJ2aWV3ID0gY2xhc3MgVGltZWxpbmVPdmVydmlldyBleHRl bmRzIFdlYkluc3BlY3Rvci5WaWV3CiAKICAgICAgICAgaWYgKCF0aGlzLl9kb250VXBkYXRlU2Ny b2xsTGVmdCkgewogICAgICAgICAgICAgdGhpcy5faWdub3JlTmV4dFNjcm9sbEV2ZW50ID0gdHJ1 ZTsKLSAgICAgICAgICAgIHRoaXMuX3Njcm9sbENvbnRhaW5lckVsZW1lbnQuc2Nyb2xsTGVmdCA9 IE1hdGguY2VpbCgoc2Nyb2xsU3RhcnRUaW1lIC0gdGhpcy5fc3RhcnRUaW1lKSAvIHRoaXMuX2R1 cmF0aW9uUGVyUGl4ZWwpOworICAgICAgICAgICAgbGV0IHNjcm9sbExlZnQgPSBNYXRoLmNlaWwo KHNjcm9sbFN0YXJ0VGltZSAtIHRoaXMuX3N0YXJ0VGltZSkgLyB0aGlzLl9kdXJhdGlvblBlclBp eGVsKTsKKyAgICAgICAgICAgIGlmIChzY3JvbGxMZWZ0KQorICAgICAgICAgICAgICAgIHRoaXMu X3Njcm9sbENvbnRhaW5lckVsZW1lbnQuc2Nyb2xsTGVmdCA9IHNjcm9sbExlZnQ7CiAgICAgICAg IH0KIAogICAgICAgICBmb3IgKGxldCB0aW1lbGluZU92ZXJ2aWV3R3JhcGggb2YgdGhpcy5fdGlt ZWxpbmVPdmVydmlld0dyYXBoc01hcC52YWx1ZXMoKSkgewo=