Bug 151910 – Web Inspector: Comparisons in setters should use the massaged value (" = x || 0/false/null/etc")

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

[Matt Baker]

* SUMMARY
Comparisons in setters should use the massaged value (" = x || 0/false/null/etc"). The following idiom is being used in a number of places:

set foo(x)
{
    if (this._x === x)
        return;

    this._x = x || <default>; // Typical default values: 0|false|null|{}|[]
    this.needsLayout();       // Or another method with side effects
}

At the very least this will result in unnecessary work being done when the default value is passed in. At worst, this can cause an infinite recursion when a view with such a property attempts to set it to the default value during layout. The pattern should be changed to:

set foo(x)
{
    x = x || <default>;
    if (this._x === x)
        return;

    this._x = x;
    this.needsLayout();
}

[Radar WebKit Bug Importer]

<rdar://problem/23772319>

[Matt Baker]

(In reply to comment #0)
> * SUMMARY
> Comparisons in setters should use the massaged value (" = x ||
> 0/false/null/etc"). The following idiom is being used in a number of places:
> 
> set foo(x)
> {
>     if (this._x === x)
>         return;
> 
>     this._x = x || <default>; // Typical default values: 0|false|null|{}|[]
>     this.needsLayout();       // Or another method with side effects
> }
> 
> At the very least this will result in unnecessary work being done when the
> default value is passed in.

This should read:

...this will result in unnecessary work when a falsy value not equal to the default is passed in, and the current value is equal to the default.

[Matt Baker]

It isn't necessary to make the change when the default is NaN, since NaN !== NaN.

[Matt Baker]

Created attachment 266708 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 266710 [details]
[Patch] Proposed Fix

[Devin Rousso]

So quick question on this: if we used a default argument instead (e.g. function(x = "") ) would the default value ("") get set whenever the function is called with a falsy value, or only if its called with NO values?

function()      -  x = ""
function(null)  -  is x = "" or x = null?

Just double checking that I understand how the spec works :)

[Timothy Hatcher]

(In reply to comment #6)
> So quick question on this: if we used a default argument instead (e.g.
> function(x = "") ) would the default value ("") get set whenever the
> function is called with a falsy value, or only if its called with NO values?
> 
> function()      -  x = ""
> function(null)  -  is x = "" or x = null?
> 
> Just double checking that I understand how the spec works :)

Sadly default values would not help here. The default is only used if the argument is excluded and maybe undefined is passed, not sure on that part though.

[Timothy Hatcher]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=266710&action=review

> Source/WebInspectorUI/UserInterface/Models/TimelineMarker.js:48
> +        x = x || 0;

While we are at it, it might be best to assert x is a number.

> Source/WebInspectorUI/UserInterface/Models/TimelineMarker.js:49
>          if (this._time === x)

If put a new line between.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1328
> +        x = x || false;

Similarly, we might want to just do: x = !!x; for the booleans. That way we know the value is only true or false, not false and anything else.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1414
> +        x = x || {};

Could assert x is an object.

[Matt Baker]

(In reply to comment #7)
> (In reply to comment #6)
> > So quick question on this: if we used a default argument instead (e.g.
> > function(x = "") ) would the default value ("") get set whenever the
> > function is called with a falsy value, or only if its called with NO values?
> > 
> > function()      -  x = ""
> > function(null)  -  is x = "" or x = null?
> > 
> > Just double checking that I understand how the spec works :)
> 
> Sadly default values would not help here. The default is only used if the
> argument is excluded and maybe undefined is passed, not sure on that part
> though.

A parameter is initialized to the default value when no value or undefined is passed in. This isn't equivalent to the "x || 0" style being used now, but it may actually be preferable. For example, with the current method it's impossible to set TimerRuler's duration to zero, as 0 || NaN will evaluate to NaN. We should evaluate these cases and see if default parameters would be appropriate.

[Timothy Hatcher]

Do default parameters even work for setters? Only in x.foo = undefined? It's not like you can call a setter with no arguments.

[Matt Baker]

(In reply to comment #10)
> Do default parameters even work for setters? Only in x.foo = undefined? It's
> not like you can call a setter with no arguments.

Probably not! Instead of massaging the input maybe we should just assert that the value is of the expected type, so we don't miss any subtle bugs.

[Matt Baker]

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

View in context: https://bugs.webkit.org/attachment.cgi?id=266710&action=review

>> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1328
>> +        x = x || false;
> 
> Similarly, we might want to just do: x = !!x; for the booleans. That way we know the value is only true or false, not false and anything else.

I wonder why we're being so paranoid with these inputs. It makes sense to check inputs in certain cases, such as data coming across the protocol, but here it seems like overkill. Just a thought.

[Matt Baker]

Created attachment 266749 [details]
[Patch] Proposed Fix

[WebKit Commit Bot]

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

Clearing flags on attachment: 266749

Committed r193612: <http://trac.webkit.org/changeset/193612>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.