* SUMMARY NavigationBar.updateLayoutSoon should use requestAnimationFrame. Currently it just uses a simple setTimeout(..., 0).
<rdar://problem/22281006>
Created attachment 259062 [details] [Patch] Proposed Fix
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?
(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.
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.
(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.
Created attachment 259075 [details] [Patch] Proposed Fix
Comment on attachment 259075 [details] [Patch] Proposed Fix Clearing flags on attachment: 259075 Committed r188506: <http://trac.webkit.org/changeset/188506>
All reviewed patches have been landed. Closing bug.
(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.