Bug 148010 – Web Inspector: NavigationBar.updateLayoutSoon should use requestAnimationFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
NavigationBar.updateLayoutSoon should use requestAnimationFrame. Currently it just uses a simple setTimeout(..., 0).

[Radar WebKit Bug Importer]

<rdar://problem/22281006>

[Matt Baker]

Created attachment 259062 [details]
[Patch] Proposed Fix

[Devin Rousso]

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?

[Matt Baker]

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

[Blaze Burg]

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.

[Matt Baker]

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

[Matt Baker]

Created attachment 259075 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

Comment on attachment 259075 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 259075

Committed r188506: <http://trac.webkit.org/changeset/188506>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Blaze Burg]

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