Bug 77611 – Rename [ConvertUndefinedOrNullToNullString] to [TreatNullAs=NullString, TreatUndefinedAs=NullString]

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

We should rename [ConvertUndefinedOrNullToNullString] to [TreatNullAs=EmptyString, TreatUndefinedAs=EmptyString], according to the spec (http://dev.w3.org/2006/webapi/WebIDL/#TreatNullAs, http://dev.w3.org/2006/webapi/WebIDL/#TreatUndefinedAs).

[Kentaro Hara]

Created attachment 125084 [details]
Patch

[Adam Barth]

Comment on attachment 125084 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2875
> +        }
> +        if (($signature->extendedAttributes->{"TreatNullAs"} and $signature->extendedAttributes->{"TreatNullAs"} eq "EmptyString") or $signature->extendedAttributes->{"Reflect"}) {

Do we need an "else" here?

> Source/WebCore/bindings/scripts/test/TestObj.idl:195
> -        void convert5(in [ConvertUndefinedOrNullToNullString] e);
> +        void convert5(in [TreatNullAs=EmptyString, TreatUndefinedAs=EmptyString] e);

In WebKit, the null string is different from the empty string.  Are you sure this is right?

[Kentaro Hara]

Comment on attachment 125084 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2875
>> +        if (($signature->extendedAttributes->{"TreatNullAs"} and $signature->extendedAttributes->{"TreatNullAs"} eq "EmptyString") or $signature->extendedAttributes->{"Reflect"}) {
> 
> Do we need an "else" here?

I think no, according to the coding style of WebKit: "An else if statement should be written as an if statement when the prior if concludes with a return statement."
http://www.webkit.org/coding/coding-style.html#indentation-namespace

>> Source/WebCore/bindings/scripts/test/TestObj.idl:195
>> +        void convert5(in [TreatNullAs=EmptyString, TreatUndefinedAs=EmptyString] e);
> 
> In WebKit, the null string is different from the empty string.  Are you sure this is right?

I think that this is not correct, but this is the way WebKit has been working; i.e. this patch does not at all change the WebKit behavior.

When we specify [TreatUndefinedAs=EmptyString] on an attribute, V8Parameter<WithUndefinedOrNullCheck>::prepare() is called:
http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8Binding.h&exact_package=chromium&q=v8binding.h&type=cs&l=504

Then, V8Parameter<WithUndefinedOrNullCheck>::prepare() creates String() (i.e. the null string in WebKit) if the attribute is JS's null or JS's undefined. This is wrong, since the spec requires the empty string in WebKit. That being said, I guess that this does not cause serious issues in WebKit because the null string is "included" in the empty string. (i.e. String::IsEmpty() returns true for the null string.)

Maybe we should land the patch as-is, and then fix V8Parameter<WithUndefinedOrNullCheck>::prepare() so that it creates the empty string?

[Adam Barth]

Comment on attachment 125084 [details]
Patch

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

Makes sense.  Thanks.

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2875
>>> +        if (($signature->extendedAttributes->{"TreatNullAs"} and $signature->extendedAttributes->{"TreatNullAs"} eq "EmptyString") or $signature->extendedAttributes->{"Reflect"}) {
>> 
>> Do we need an "else" here?
> 
> I think no, according to the coding style of WebKit: "An else if statement should be written as an if statement when the prior if concludes with a return statement."
> http://www.webkit.org/coding/coding-style.html#indentation-namespace

Oh, you're right.  I misread this and thought it was just generating code with a return in it.

[WebKit Review Bot]

Comment on attachment 125084 [details]
Patch

Clearing flags on attachment: 125084

Committed r106575: <http://trac.webkit.org/changeset/106575>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 125084 [details])
> > In WebKit, the null string is different from the empty string.  Are you sure this is right?
> 
> I think that this is not correct, but this is the way WebKit has been working; i.e. this patch does not at all change the WebKit behavior.
> 
> When we specify [TreatUndefinedAs=EmptyString] on an attribute, V8Parameter<WithUndefinedOrNullCheck>::prepare() is called:
> http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8Binding.h&exact_package=chromium&q=v8binding.h&type=cs&l=504
> 
> Then, V8Parameter<WithUndefinedOrNullCheck>::prepare() creates String() (i.e. the null string in WebKit) if the attribute is JS's null or JS's undefined. This is wrong, since the spec requires the empty string in WebKit. That being said, I guess that this does not cause serious issues in WebKit because the null string is "included" in the empty string. (i.e. String::IsEmpty() returns true for the null string.)
> 
> Maybe we should land the patch as-is, and then fix V8Parameter<WithUndefinedOrNullCheck>::prepare() so that it creates the empty string?

It appears that the current WebKit implementation is wrong but it might be a good "optimization". As I mentioned above, using the null string instead of the empty string would cause no problem. Given that we change the null string to the empty string, we need to create |m_impl| for the string, which might hit performance. WDYT? If you think we should fix the current implementation, I'll file a bug.

[Adam Barth]

I might add a FIXME comment explaining the situation.  The proper time to fix this is probably when it causes a real bug.

[Kentaro Hara]

Let me reopen this bug to insert "FIXME".

[Kentaro Hara]

Created attachment 125292 [details]
Patch

[Darin Adler]

I think these FIXMEs are misleading and we are not heading in exactly the right direction.

The concept of a null string definitely won’t be mentioned in an IDL specification, because it’s not a web platform concept. The null string vs. empty string distinction is a WebKit implementation detail.

In cases where we want the same behavior for a JavaScript empty string and a JavaScript null argument, it’s possibly better to use an empty string in the bindings rather than null string. Because the null string is passed today, it’s likely that there is code, far from the bindings layer, that is carefully treating null strings and empty strings the same because it can get either from the bindings. If we instead guaranteed that the bindings would never use a null string, then worries about handling a null string the same way as an empty string could be confined to calls from inside WebKit. It’s possible that the null string is more efficient to construct and pass around than the empty string. We may be getting some performance benefits from the current way of doing things.

However, switching to the empty string creates a problem. There are a number of DOM APIs where a JavaScript null does something *different* than a JavaScript empty string. For those APIs, we are currently using the null vs. empty string distinction to differentiate the JavaScript null or JavaScript missing argument case from the JavaScript empty string case.

The classic example of this is the setAttribute function on Element. Another example I spotted is the getItems function on Document. It would be dangerous to change this behavior unless we have sufficient test coverage for calls to all functions that are doing this.

I also think it was a bit dangerous to change the names in IDL files. The new name obscures this because it states clearly, but inaccurately, that the empty string will be passed if the argument is null. That change makes it even harder for future programmers to figure this out.

The long term target should be to have something that is clear (possibly by being explicit when null handling is needed), efficient (possibly by using null string in cases where we could use either null or empty string), and also minimizes unnecessary differences from the standard. It’s not obvious to me exactly how to accomplish all three, but it shouldn’t be too difficult.

[Adam Barth]

Comment on attachment 125292 [details]
Patch

Maybe the incremental step forward here is to changes these to TreatNullAs=NullString so that the attribute match the implementation more closely?

[Kentaro Hara]

Thanks, darin and adam.

(In reply to comment #12)
> (From update of attachment 125292 [details])
> Maybe the incremental step forward here is to changes these to TreatNullAs=NullString so that the attribute match the implementation more closely?

This appears to be a good approach.

[Kentaro Hara]

Created attachment 125563 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125563 [details]
Patch

Clearing flags on attachment: 125563

Committed r106776: <http://trac.webkit.org/changeset/106776>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.