Bug 157627 – Drop toJS() overload taking a PassRefPtr<> parameter

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Drop toJS() overload taking a PassRefPtr<> parameter.

[Chris Dumez]

Created attachment 278746 [details]
Patch

[Chris Dumez]

Created attachment 278748 [details]
Patch

[Alex Christensen]

Comment on attachment 278748 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValue.cpp:-945
> -    ec = 0;

Why are you deleting this here and not elsewhere?

[Chris Dumez]

Comment on attachment 278748 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValue.cpp:-945
>> -    ec = 0;
> 
> Why are you deleting this here and not elsewhere?

Where else do you want me to remove it?

We don't check ec in this function so there is no point in setting it to 0.

[Alex Christensen]

Comment on attachment 278748 [details]
Patch

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

>>> Source/WebCore/css/CSSPrimitiveValue.cpp:-945
>>> -    ec = 0;
>> 
>> Why are you deleting this here and not elsewhere?
> 
> Where else do you want me to remove it?
> 
> We don't check ec in this function so there is no point in setting it to 0.

ec is a reference, so changing this changes the behavior of the caller.  Many other functions in this file do the same thing.  I think this is a bad change.

[Chris Dumez]

Comment on attachment 278748 [details]
Patch

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

>>>> Source/WebCore/css/CSSPrimitiveValue.cpp:-945
>>>> -    ec = 0;
>>> 
>>> Why are you deleting this here and not elsewhere?
>> 
>> Where else do you want me to remove it?
>> 
>> We don't check ec in this function so there is no point in setting it to 0.
> 
> ec is a reference, so changing this changes the behavior of the caller.  Many other functions in this file do the same thing.  I think this is a bad change.

and yet. The assignment to 0 here is unnecessary. The rule is that you need to set ec to 0 if you are going to check its value after calling a function. It is therefore the caller of getRGBColorValue()'s responsibility to set ec to 0 if they check ec after calling getRGBColorValue(). Note that getRGBColorValue() is only called from the bindings and they do indeed set ec to 0 before calling getRGBColorValue() and then check the value of ec after calling the function.

[Chris Dumez]

Comment on attachment 278748 [details]
Patch

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

>>>>> Source/WebCore/css/CSSPrimitiveValue.cpp:-945
>>>>> -    ec = 0;
>>>> 
>>>> Why are you deleting this here and not elsewhere?
>>> 
>>> Where else do you want me to remove it?
>>> 
>>> We don't check ec in this function so there is no point in setting it to 0.
>> 
>> ec is a reference, so changing this changes the behavior of the caller.  Many other functions in this file do the same thing.  I think this is a bad change.
> 
> and yet. The assignment to 0 here is unnecessary. The rule is that you need to set ec to 0 if you are going to check its value after calling a function. It is therefore the caller of getRGBColorValue()'s responsibility to set ec to 0 if they check ec after calling getRGBColorValue(). Note that getRGBColorValue() is only called from the bindings and they do indeed set ec to 0 before calling getRGBColorValue() and then check the value of ec after calling the function.

And to answer your other question, I did not update the other methods because I did not touch them in this patch and dropping ec like this requires some work to make sure the call sites are correct.

[Alex Christensen]

Comment on attachment 278748 [details]
Patch

ok

[Chris Dumez]

Comment on attachment 278748 [details]
Patch

Clearing flags on attachment: 278748

Committed r200789: <http://trac.webkit.org/changeset/200789>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.