Bug 77611 - Rename [ConvertUndefinedOrNullToNullString] to [TreatNullAs=NullString, TreatUndefinedAs=NullString]
Summary: Rename [ConvertUndefinedOrNullToNullString] to [TreatNullAs=NullString, Treat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks: 77393
  Show dependency treegraph
 
Reported: 2012-02-01 23:36 PST by Kentaro Hara
Modified: 2012-02-05 23:06 PST (History)
8 users (show)

See Also:


Attachments
Patch (24.72 KB, patch)
2012-02-01 23:44 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (4.44 KB, patch)
2012-02-03 01:55 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (69.59 KB, patch)
2012-02-05 21:20 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-02-01 23:36:18 PST
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).
Comment 1 Kentaro Hara 2012-02-01 23:44:11 PST
Created attachment 125084 [details]
Patch
Comment 2 Adam Barth 2012-02-01 23:49:10 PST
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?
Comment 3 Kentaro Hara 2012-02-02 01:27:41 PST
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?
Comment 4 Adam Barth 2012-02-02 09:50:24 PST
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.
Comment 5 WebKit Review Bot 2012-02-02 11:24:41 PST
Comment on attachment 125084 [details]
Patch

Clearing flags on attachment: 125084

Committed r106575: <http://trac.webkit.org/changeset/106575>
Comment 6 WebKit Review Bot 2012-02-02 11:24:46 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Kentaro Hara 2012-02-02 17:41:44 PST
(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.
Comment 8 Adam Barth 2012-02-03 00:43:55 PST
I might add a FIXME comment explaining the situation.  The proper time to fix this is probably when it causes a real bug.
Comment 9 Kentaro Hara 2012-02-03 01:32:44 PST
Let me reopen this bug to insert "FIXME".
Comment 10 Kentaro Hara 2012-02-03 01:55:37 PST
Created attachment 125292 [details]
Patch
Comment 11 Darin Adler 2012-02-03 11:29:19 PST
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.
Comment 12 Adam Barth 2012-02-03 14:15:30 PST
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?
Comment 13 Kentaro Hara 2012-02-03 16:51:03 PST
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.
Comment 14 Kentaro Hara 2012-02-05 21:20:32 PST
Created attachment 125563 [details]
Patch
Comment 15 WebKit Review Bot 2012-02-05 23:06:19 PST
Comment on attachment 125563 [details]
Patch

Clearing flags on attachment: 125563

Committed r106776: <http://trac.webkit.org/changeset/106776>
Comment 16 WebKit Review Bot 2012-02-05 23:06:26 PST
All reviewed patches have been landed.  Closing bug.