Bug 70215 – Generate WebKitCSSMatrix constructor for JSC by [Constructor] IDL

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

Currently, the WebKitCSSMatrix constructor in JSC is written manually as a custom constructor. We should replace this with automatically generated code by the [Constructor] IDL.

[Kentaro Hara]

Created attachment 111222 [details]
Patch

[Adam Barth]

Ideally Sam or ggaren would comment on the JavaScriptCore aspect of this change.

[Kentaro Hara]

Created attachment 111545 [details]
Rebased patch for review

[Sam Weinig]

Comment on attachment 111545 [details]
Rebased patch for review

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

We should not be special casing an empty JSValue.  If you get one of these, something is probably wrong.

> Source/JavaScriptCore/runtime/JSString.h:712
> +        if (isEmpty())
> +            return UString();

This is wrong.  The empty value JSValue should be treated like a null pointer, and you should never call functions on it.

[Kentaro Hara]

Comment on attachment 111545 [details]
Rebased patch for review

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

>> Source/JavaScriptCore/runtime/JSString.h:712
>> +            return UString();
> 
> This is wrong.  The empty value JSValue should be treated like a null pointer, and you should never call functions on it.

Thanks, sam!

However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. 

Why do you think that the empty value should be treated like a null pointer?

[Gavin Barraclough]

(In reply to comment #5)
> (From update of attachment 111545 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review
> 
> >> Source/JavaScriptCore/runtime/JSString.h:712
> >> +            return UString();
> > 
> > This is wrong.  The empty value JSValue should be treated like a null pointer, and you should never call functions on it.
> 
> Thanks, sam!
> 
> However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. 
> 
> Why do you think that the empty value should be treated like a null pointer?

The empty value is not a valid value to be passed into JavaScript code (it does not have a a meaning within the language), and JS code will not check for this (e.g. passing the empty value to JIT code could lead to a null pointer exception).  The correct interpretation of the empty value is as an in-band signal of an error.  It is not a valid value to operate on or to pass to functions.

I think the numeric conversions should probably not be accepting the empty value, that this is an error.  They should likely be able to ASSERT against this usage.

[Kentaro Hara]

Created attachment 112804 [details]
Patch

[Kentaro Hara]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 111545 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=111545&action=review
> > 
> > >> Source/JavaScriptCore/runtime/JSString.h:712
> > >> +            return UString();
> > > 
> > > This is wrong.  The empty value JSValue should be treated like a null pointer, and you should never call functions on it.
> > 
> > Thanks, sam!
> > 
> > However, toNumber(), toFloat(), toInt32(), toUInt32() and toInteger() return 0 when an empty value is passed to them. Specifically, in case of the empty value, those methods fallback to JSValue::toNumberSlowCase(), which returns 0. toBoolean() returns false when the empty value is passed. Judging from these behaviors, I guess that it would make sense to return the "default value" of the given type when the empty value is passed. If so, returning UString() from toString() would also make sense. 
> > 
> > Why do you think that the empty value should be treated like a null pointer?
> 
> The empty value is not a valid value to be passed into JavaScript code (it does not have a a meaning within the language), and JS code will not check for this (e.g. passing the empty value to JIT code could lead to a null pointer exception).  The correct interpretation of the empty value is as an in-band signal of an error.  It is not a valid value to operate on or to pass to functions.
> 
> I think the numeric conversions should probably not be accepting the empty value, that this is an error.  They should likely be able to ASSERT against this usage.

Thank you, I got it. I fixed the patch. 

c.f. Why do we need to introduce [CallWithNullValue] and MAYBE_MISSING_PARAMETER() macro: https://bugs.webkit.org/show_bug.cgi?id=67458

[Adam Barth]

Comment on attachment 112804 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        This patch implements [Optional=CallWithNullValue] IDL for JSC.
> +        While a parameter specified as [Optional=CallWithDefaultValue] is
> +        handled as a string "undefined", a parameter specified as
> +        [Optional=CallWithNullValue] is handled as a null string.
> +        (Note: not a string "null", but a null string).

Interesting.

[WebKit Review Bot]

Comment on attachment 112804 [details]
Patch

Clearing flags on attachment: 112804

Committed r98679: <http://trac.webkit.org/changeset/98679>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString?

[Adam Barth]

> How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString?

I would expect the two to be different from when the argument null is supplied.  In the ConvertNullToNullString case, the null string will be supplied whereas in the Optional=CallWithNullValue, the string "null" will be supplied.

[Kentaro Hara]

(In reply to comment #12)
> How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString?

ConvertNullToNullString defines the behavior when null is passed to a parameter. On the other hand, Optional=CallWithNullValue defines the behavior when the parameter is missing.

Please assume func(in [XXXX] DOMString str).

If XXXX is "Optional, ConvertNullToNullString":
func(null) -> (not a string "null" but) a null string is passed to a WebCore method.
func() -> a string "undefined" is passed to a WebCore method.

If XXXX is "Optional=CallWithNullValue":
func(null) -> a string "null" is passed to a WebCore method.
func() -> a null string is passed to a WebCore method.

If XXXX is "Optional=CallWithDefaultValue":
func(null) -> a string "null" is passed to a WebCore method.
func() -> a string "undefined" is passed to a WebCore method.

[Ryosuke Niwa]

binding tests are failing :(

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/25283

[Ryosuke Niwa]

Rebaselined binding tests in http://trac.webkit.org/changeset/98688.

We should really do something about these binding tests. It's shadowing other more serious failures by turning bots red :(

[Darin Adler]

(In reply to comment #16)
> We should really do something about these binding tests.

Yes, I think we should make sure at least one EWS server runs them.