148010 2015-08-13 18:36:07 -0700 Web Inspector: NavigationBar.updateLayoutSoon should use requestAnimationFrame 2015-08-14 21:35:04 -0700 1 1 1 Unclassified WebKit Web Inspector 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 1 mattbaker mattbaker bburg commit-queue graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1117510 0 mattbaker 2015-08-13 18:36:07 -0700 * SUMMARY NavigationBar.updateLayoutSoon should use requestAnimationFrame. Currently it just uses a simple setTimeout(..., 0). 1117511 1 webkit-bug-importer 2015-08-13 18:36:19 -0700 <rdar://problem/22281006> 1117851 2 259062 mattbaker 2015-08-14 18:16:36 -0700 Created attachment 259062 [Patch] Proposed Fix 1117859 3 259062 hi 2015-08-14 18:54:20 -0700 Comment on attachment 259062 [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259062&action=review > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:59 > + this._updateLayoutIdentifier = 0; To me, _updateLayoutIdentifier is a bit ambiguous as to what it represents. _updateLayoutTimeout was clear that it represented a timeout, so maybe use something like _updateLayoutAnimationFrameIdentifier? 1117874 4 mattbaker 2015-08-14 19:35:46 -0700 (In reply to comment #3) > Comment on attachment 259062 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259062&action=review > > > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:59 > > + this._updateLayoutIdentifier = 0; > > To me, _updateLayoutIdentifier is a bit ambiguous as to what it represents. > _updateLayoutTimeout was clear that it represented a timeout, so maybe use > something like _updateLayoutAnimationFrameIdentifier? It's consistent with our other request id variable names. However, we should create an abstraction to encapsulate the boilerplate layout code in Inspector view classes. This would would also address the ambiguity issue. 1117875 5 259062 bburg 2015-08-14 19:36:05 -0700 Comment on attachment 259062 [Patch] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=259062&action=review r=me >> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:59 >> + this._updateLayoutIdentifier = 0; > > To me, _updateLayoutIdentifier is a bit ambiguous as to what it represents. _updateLayoutTimeout was clear that it represented a timeout, so maybe use something like _updateLayoutAnimationFrameIdentifier? The existing name, or _scheduledUpdateIdentifier, would be consistent with existing code. > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:60 > + this._needsLayout = false; Not necessary. > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:148 > + this._updateLayoutIdentifier = 0; I would assign back to undefined, since the intention is less ambiguous. 1117876 6 mattbaker 2015-08-14 19:44:40 -0700 (In reply to comment #5) > Comment on attachment 259062 [details] > [Patch] Proposed Fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=259062&action=review > > r=me > > >> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:59 > >> + this._updateLayoutIdentifier = 0; > > > > To me, _updateLayoutIdentifier is a bit ambiguous as to what it represents. _updateLayoutTimeout was clear that it represented a timeout, so maybe use something like _updateLayoutAnimationFrameIdentifier? > > The existing name, or _scheduledUpdateIdentifier, would be consistent with > existing code. > > > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:60 > > + this._needsLayout = false; > > Not necessary. > > > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:148 > > + this._updateLayoutIdentifier = 0; > > I would assign back to undefined, since the intention is less ambiguous. Zero seems more appropriate, since requestAnimationFrame returns a non-zero value. It looks like we use undefined more than zero currently, and delete this._*Identifier the majority of the time. I'm fine with either, although I prefer zero. 1117877 7 259075 mattbaker 2015-08-14 19:54:44 -0700 Created attachment 259075 [Patch] Proposed Fix 1117884 8 259075 commit-queue 2015-08-14 20:47:29 -0700 Comment on attachment 259075 [Patch] Proposed Fix Clearing flags on attachment: 259075 Committed r188506: <http://trac.webkit.org/changeset/188506> 1117885 9 commit-queue 2015-08-14 20:47:34 -0700 All reviewed patches have been landed. Closing bug. 1117893 10 bburg 2015-08-14 21:35:04 -0700 (In reply to comment #6) > > > > Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:148 > > > + this._updateLayoutIdentifier = 0; > > > > I would assign back to undefined, since the intention is less ambiguous. > > Zero seems more appropriate, since requestAnimationFrame returns a non-zero > value. It looks like we use undefined more than zero currently, and delete > this._*Identifier the majority of the time. I'm fine with either, although I > prefer zero. What I meant was that `= undefined` is a generic replacement for delete, whereas `= 0` only makes sense in some context. So if I read the former, I need to think less about what is being done. 259062 2015-08-14 18:16:36 -0700 2015-08-14 19:54:41 -0700 [Patch] Proposed Fix bug-148010-20150814181616.patch text/plain 2709 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMTg4NTAzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCBj OWU1MzhiZmI1MjVmODc3Yjk2YzE3NzQ4YjliODk5NzY2NDcxMTRkLi5hNzhkNWZmYjI3NDZlNGM3 YzM3MTI5OWQyMmYwYmFmNTZiNmQyOWVlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxNS0wOC0xNCAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogTmF2aWdhdGlvbkJhci51cGRhdGVMYXlvdXRTb29u IHNob3VsZCB1c2UgcmVxdWVzdEFuaW1hdGlvbkZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDgwMTAKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBOYXZpZ2F0aW9uQmFyLnVwZGF0ZUxheW91dFNvb24g bm93IHVzZXMgcmVxdWVzdEFuaW1hdGlvbkZyYW1lIGluc3RlYWQgb2Ygc2V0VGltZW91dC4KKwor ICAgICAgICAqIFVzZXJJbnRlcmZhY2UvVmlld3MvTmF2aWdhdGlvbkJhci5qczoKKyAgICAgICAg KFdlYkluc3BlY3Rvci5OYXZpZ2F0aW9uQmFyKToKKyAgICAgICAgKFdlYkluc3BlY3Rvci5OYXZp Z2F0aW9uQmFyLnByb3RvdHlwZS51cGRhdGVMYXlvdXRTb29uLnVwZGF0ZSk6CisgICAgICAgIChX ZWJJbnNwZWN0b3IuTmF2aWdhdGlvbkJhci5wcm90b3R5cGUudXBkYXRlTGF5b3V0U29vbik6Cisg ICAgICAgIChXZWJJbnNwZWN0b3IuTmF2aWdhdGlvbkJhci5wcm90b3R5cGUudXBkYXRlTGF5b3V0 KToKKwogMjAxNS0wOC0xNCAgRGV2aW4gUm91c3NvICA8ZHJvdXNzb0BhcHBsZS5jb20+CiAKICAg ICAgICAgV2ViIEluc3BlY3RvcjogQWRkIFZpc3VhbFN0eWxlRGV0YWlsc1BhbmVsCmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9OYXZpZ2F0aW9u QmFyLmpzIGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvTmF2aWdh dGlvbkJhci5qcwppbmRleCA1YjM1ZjIzY2U0MDEwYjViNDhhNzgzMTU5Yjc2ZjNmODc5MmQ2YTQw Li5kZTQzZTk1M2NlMmUyZjQ0MTM5ODMzYTVkZTdmYjE1ZWRhYWE2NDczIDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9OYXZpZ2F0aW9uQmFyLmpz CisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL05hdmlnYXRp b25CYXIuanMKQEAgLTU1LDYgKzU1LDkgQEAgV2ViSW5zcGVjdG9yLk5hdmlnYXRpb25CYXIgPSBj bGFzcyBOYXZpZ2F0aW9uQmFyIGV4dGVuZHMgV2ViSW5zcGVjdG9yLk9iamVjdAogICAgICAgICB9 CiAKICAgICAgICAgZG9jdW1lbnQuaGVhZC5hcHBlbmRDaGlsZCh0aGlzLl9zdHlsZUVsZW1lbnQp OworCisgICAgICAgIHRoaXMuX3VwZGF0ZUxheW91dElkZW50aWZpZXIgPSAwOworICAgICAgICB0 aGlzLl9uZWVkc0xheW91dCA9IGZhbHNlOwogICAgIH0KIAogICAgIC8vIFB1YmxpYwpAQCAtMTIy LDI3ICsxMjUsMjcgQEAgV2ViSW5zcGVjdG9yLk5hdmlnYXRpb25CYXIgPSBjbGFzcyBOYXZpZ2F0 aW9uQmFyIGV4dGVuZHMgV2ViSW5zcGVjdG9yLk9iamVjdAogCiAgICAgdXBkYXRlTGF5b3V0U29v bigpCiAgICAgewotICAgICAgICBpZiAodGhpcy5fdXBkYXRlTGF5b3V0VGltZW91dCkKKyAgICAg ICAgaWYgKHRoaXMuX3VwZGF0ZUxheW91dElkZW50aWZpZXIpCiAgICAgICAgICAgICByZXR1cm47 CiAKICAgICAgICAgdGhpcy5fbmVlZHNMYXlvdXQgPSB0cnVlOwogCiAgICAgICAgIGZ1bmN0aW9u IHVwZGF0ZSgpCiAgICAgICAgIHsKLSAgICAgICAgICAgIGRlbGV0ZSB0aGlzLl91cGRhdGVMYXlv dXRUaW1lb3V0OworICAgICAgICAgICAgdGhpcy5fdXBkYXRlTGF5b3V0SWRlbnRpZmllciA9IDA7 CiAKICAgICAgICAgICAgIGlmICh0aGlzLl9uZWVkc0xheW91dCB8fCB0aGlzLl9uZWVkc1N0eWxl VXBkYXRlZCkKICAgICAgICAgICAgICAgICB0aGlzLnVwZGF0ZUxheW91dCgpOwogICAgICAgICB9 CiAKLSAgICAgICAgdGhpcy5fdXBkYXRlTGF5b3V0VGltZW91dCA9IHNldFRpbWVvdXQodXBkYXRl LmJpbmQodGhpcyksIDApOworICAgICAgICB0aGlzLl91cGRhdGVMYXlvdXRJZGVudGlmaWVyID0g cmVxdWVzdEFuaW1hdGlvbkZyYW1lKHVwZGF0ZS5iaW5kKHRoaXMpKTsKICAgICB9CiAKICAgICB1 cGRhdGVMYXlvdXQoKQogICAgIHsKLSAgICAgICAgaWYgKHRoaXMuX3VwZGF0ZUxheW91dFRpbWVv dXQpIHsKLSAgICAgICAgICAgIGNsZWFyVGltZW91dCh0aGlzLl91cGRhdGVMYXlvdXRUaW1lb3V0 KTsKLSAgICAgICAgICAgIGRlbGV0ZSB0aGlzLl91cGRhdGVMYXlvdXRUaW1lb3V0OworICAgICAg ICBpZiAodGhpcy5fdXBkYXRlTGF5b3V0SWRlbnRpZmllcikgeworICAgICAgICAgICAgY2FuY2Vs QW5pbWF0aW9uRnJhbWUodGhpcy5fdXBkYXRlTGF5b3V0SWRlbnRpZmllcik7CisgICAgICAgICAg ICB0aGlzLl91cGRhdGVMYXlvdXRJZGVudGlmaWVyID0gMDsKICAgICAgICAgfQogCiAgICAgICAg IGlmICh0aGlzLl9uZWVkc1N0eWxlVXBkYXRlZCkK 259075 2015-08-14 19:54:44 -0700 2015-08-14 20:47:29 -0700 [Patch] Proposed Fix bug-148010-20150814195425.patch text/plain 2452 mattbaker U3VidmVyc2lvbiBSZXZpc2lvbjogMTg4NTA0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVj dG9yVUkvQ2hhbmdlTG9nIGIvU291cmNlL1dlYkluc3BlY3RvclVJL0NoYW5nZUxvZwppbmRleCBj OWU1MzhiZmI1MjVmODc3Yjk2YzE3NzQ4YjliODk5NzY2NDcxMTRkLi4wZjgxMDlhYjBhODU0MDJj MDliNDQyOGUwMjQ3Y2RhMzgxZmU4NWJhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9y VUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOCBAQAorMjAxNS0wOC0xNCAgTWF0dCBCYWtlciAgPG1hdHRiYWtlckBhcHBsZS5jb20+ CisKKyAgICAgICAgV2ViIEluc3BlY3RvcjogTmF2aWdhdGlvbkJhci51cGRhdGVMYXlvdXRTb29u IHNob3VsZCB1c2UgcmVxdWVzdEFuaW1hdGlvbkZyYW1lCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDgwMTAKKworICAgICAgICBSZXZpZXdlZCBieSBC cmlhbiBCdXJnLgorCisgICAgICAgIE5hdmlnYXRpb25CYXIudXBkYXRlTGF5b3V0U29vbiBub3cg dXNlcyByZXF1ZXN0QW5pbWF0aW9uRnJhbWUgaW5zdGVhZCBvZiBzZXRUaW1lb3V0LgorCisgICAg ICAgICogVXNlckludGVyZmFjZS9WaWV3cy9OYXZpZ2F0aW9uQmFyLmpzOgorICAgICAgICAoV2Vi SW5zcGVjdG9yLk5hdmlnYXRpb25CYXIpOgorICAgICAgICAoV2ViSW5zcGVjdG9yLk5hdmlnYXRp b25CYXIucHJvdG90eXBlLnVwZGF0ZUxheW91dFNvb24udXBkYXRlKToKKyAgICAgICAgKFdlYklu c3BlY3Rvci5OYXZpZ2F0aW9uQmFyLnByb3RvdHlwZS51cGRhdGVMYXlvdXRTb29uKToKKyAgICAg ICAgKFdlYkluc3BlY3Rvci5OYXZpZ2F0aW9uQmFyLnByb3RvdHlwZS51cGRhdGVMYXlvdXQpOgor CiAyMDE1LTA4LTE0ICBEZXZpbiBSb3Vzc28gIDxkcm91c3NvQGFwcGxlLmNvbT4KIAogICAgICAg ICBXZWIgSW5zcGVjdG9yOiBBZGQgVmlzdWFsU3R5bGVEZXRhaWxzUGFuZWwKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL05hdmlnYXRpb25CYXIu anMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9OYXZpZ2F0aW9u QmFyLmpzCmluZGV4IDViMzVmMjNjZTQwMTBiNWI0OGE3ODMxNTliNzZmM2Y4NzkyZDZhNDAuLjZi YTZkMDFlY2M3NzI1NzdkZTc4MWNhYWIyYzdhMWQxNTdiOGNlYzMgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL05hdmlnYXRpb25CYXIuanMKKysr IGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvTmF2aWdhdGlvbkJh ci5qcwpAQCAtMTIyLDI3ICsxMjIsMjcgQEAgV2ViSW5zcGVjdG9yLk5hdmlnYXRpb25CYXIgPSBj bGFzcyBOYXZpZ2F0aW9uQmFyIGV4dGVuZHMgV2ViSW5zcGVjdG9yLk9iamVjdAogCiAgICAgdXBk YXRlTGF5b3V0U29vbigpCiAgICAgewotICAgICAgICBpZiAodGhpcy5fdXBkYXRlTGF5b3V0VGlt ZW91dCkKKyAgICAgICAgaWYgKHRoaXMuX3VwZGF0ZUxheW91dElkZW50aWZpZXIpCiAgICAgICAg ICAgICByZXR1cm47CiAKICAgICAgICAgdGhpcy5fbmVlZHNMYXlvdXQgPSB0cnVlOwogCiAgICAg ICAgIGZ1bmN0aW9uIHVwZGF0ZSgpCiAgICAgICAgIHsKLSAgICAgICAgICAgIGRlbGV0ZSB0aGlz Ll91cGRhdGVMYXlvdXRUaW1lb3V0OworICAgICAgICAgICAgdGhpcy5fdXBkYXRlTGF5b3V0SWRl bnRpZmllciA9IHVuZGVmaW5lZDsKIAogICAgICAgICAgICAgaWYgKHRoaXMuX25lZWRzTGF5b3V0 IHx8IHRoaXMuX25lZWRzU3R5bGVVcGRhdGVkKQogICAgICAgICAgICAgICAgIHRoaXMudXBkYXRl TGF5b3V0KCk7CiAgICAgICAgIH0KIAotICAgICAgICB0aGlzLl91cGRhdGVMYXlvdXRUaW1lb3V0 ID0gc2V0VGltZW91dCh1cGRhdGUuYmluZCh0aGlzKSwgMCk7CisgICAgICAgIHRoaXMuX3VwZGF0 ZUxheW91dElkZW50aWZpZXIgPSByZXF1ZXN0QW5pbWF0aW9uRnJhbWUodXBkYXRlLmJpbmQodGhp cykpOwogICAgIH0KIAogICAgIHVwZGF0ZUxheW91dCgpCiAgICAgewotICAgICAgICBpZiAodGhp cy5fdXBkYXRlTGF5b3V0VGltZW91dCkgewotICAgICAgICAgICAgY2xlYXJUaW1lb3V0KHRoaXMu X3VwZGF0ZUxheW91dFRpbWVvdXQpOwotICAgICAgICAgICAgZGVsZXRlIHRoaXMuX3VwZGF0ZUxh eW91dFRpbWVvdXQ7CisgICAgICAgIGlmICh0aGlzLl91cGRhdGVMYXlvdXRJZGVudGlmaWVyKSB7 CisgICAgICAgICAgICBjYW5jZWxBbmltYXRpb25GcmFtZSh0aGlzLl91cGRhdGVMYXlvdXRJZGVu dGlmaWVyKTsKKyAgICAgICAgICAgIHRoaXMuX3VwZGF0ZUxheW91dElkZW50aWZpZXIgPSB1bmRl ZmluZWQ7CiAgICAgICAgIH0KIAogICAgICAgICBpZiAodGhpcy5fbmVlZHNTdHlsZVVwZGF0ZWQp Cg==