Bug 70215 - Generate WebKitCSSMatrix constructor for JSC by [Constructor] IDL
Summary: Generate WebKitCSSMatrix constructor for JSC by [Constructor] IDL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 65839
  Show dependency treegraph
 
Reported: 2011-10-16 21:51 PDT by Kentaro Hara
Modified: 2011-10-28 10:38 PDT (History)
12 users (show)

See Also:


Attachments
Patch (18.94 KB, patch)
2011-10-17 01:17 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Rebased patch for review (18.84 KB, patch)
2011-10-18 18:24 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (18.60 KB, patch)
2011-10-27 18:37 PDT, 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 2011-10-16 21:51:26 PDT
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.
Comment 1 Kentaro Hara 2011-10-17 01:17:06 PDT
Created attachment 111222 [details]
Patch
Comment 2 Adam Barth 2011-10-18 09:41:25 PDT
Ideally Sam or ggaren would comment on the JavaScriptCore aspect of this change.
Comment 3 Kentaro Hara 2011-10-18 18:24:02 PDT
Created attachment 111545 [details]
Rebased patch for review
Comment 4 Sam Weinig 2011-10-26 18:29:55 PDT
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.
Comment 5 Kentaro Hara 2011-10-26 19:56:57 PDT
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?
Comment 6 Gavin Barraclough 2011-10-27 15:13:39 PDT
(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.
Comment 7 Kentaro Hara 2011-10-27 18:37:49 PDT
Created attachment 112804 [details]
Patch
Comment 8 Kentaro Hara 2011-10-27 18:42:19 PDT
(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
Comment 9 Adam Barth 2011-10-27 18:42:44 PDT
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.
Comment 10 WebKit Review Bot 2011-10-27 19:43:31 PDT
Comment on attachment 112804 [details]
Patch

Clearing flags on attachment: 112804

Committed r98679: <http://trac.webkit.org/changeset/98679>
Comment 11 WebKit Review Bot 2011-10-27 19:43:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2011-10-27 19:49:17 PDT
How is Optional=CallWithNullValue different from Optional,ConvertNullToNullString?
Comment 13 Adam Barth 2011-10-27 19:55:22 PDT
> 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.
Comment 14 Kentaro Hara 2011-10-27 19:59:18 PDT
(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.
Comment 15 Ryosuke Niwa 2011-10-27 21:40:25 PDT
binding tests are failing :(

http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/25283
Comment 16 Ryosuke Niwa 2011-10-27 21:59:34 PDT
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 :(
Comment 17 Darin Adler 2011-10-28 10:38:41 PDT
(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.