Bug 67458 - Generate a WebKitCSSMatrix constructor of V8 using the IDL 'Constructor' extended attribute
Summary: Generate a WebKitCSSMatrix constructor of V8 using the IDL 'Constructor' exte...
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:
: 66276 (view as bug list)
Depends on:
Blocks: 65839
  Show dependency treegraph
 
Reported: 2011-09-01 17:35 PDT by Kentaro Hara
Modified: 2011-10-12 01:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (12.93 KB, patch)
2011-09-01 17:55 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Rebased Patch (13.02 KB, patch)
2011-09-01 18:12 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (16.21 KB, patch)
2011-09-02 11:05 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (16.72 KB, patch)
2011-09-07 16:26 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Updated run-binding-tests results (53.15 KB, patch)
2011-09-08 08:29 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Rebased patch (36.84 KB, patch)
2011-09-08 11:09 PDT, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (36.86 KB, patch)
2011-09-09 12:38 PDT, Kentaro Hara
abarth: review+
abarth: commit-queue-
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-09-01 17:35:13 PDT
Currently, the WebKitCSSMatrix constructor is written manually as a custom constructor. We should replace this with automatically generated code by the IDL 'Constructor' extended attribute.
Comment 1 Kentaro Hara 2011-09-01 17:55:44 PDT
Created attachment 106072 [details]
Patch
Comment 2 Kentaro Hara 2011-09-01 18:12:14 PDT
Created attachment 106074 [details]
Rebased Patch
Comment 3 Adam Barth 2011-09-01 23:37:06 PDT
Comment on attachment 106074 [details]
Rebased Patch

I'm sorry, I don't fully understand this patch.  We might need someone else to help review.
Comment 4 Adam Barth 2011-09-01 23:37:24 PDT
@antonm: Thoughts?
Comment 5 Kentaro Hara 2011-09-02 02:27:35 PDT
Adam and Antonm: The motivation for the change is as follows. 

As you can see in V8WebKitCSSMatrixConstructor.cpp, constructorCallback() expects that...

- If the first argument is a DOMString, the corresponding WTF::String is passed to the first argument of WebKitCSSMatrix::create().
- If the first argument is omitted, a null WTF::String (i.e. String()) is passed to the first argument of WebKitCSSMatrix::create().

I first thought that describing 'Constructor(in [Optional=CallWithDefaultValue] DOMString cssValue)' in IDL will generate the code that behaves as above, but it does not. In fact, the generated code behaves...

- If the first argument is a DOMString, the corresponding WTF::String is passed to the first argument of WebKitCSSMatrix::create().
- If the first argument is omitted, a string "undefined" is passed to the first argument of WebKitCSSMatrix::create(). This is because when we execute 'V8Parameter cssValue(args[0])' in the situation where args[0] is omitted, args[0] becomes v8::Undefined(), which will make cssValue.toString() "undefined".

Thus, we need a way to express a null string in V8Parameter (note: V8Parameter is something like an abstraction class for String and AtomicString). The change in V8Binding.h is just doing this. 

The same scenario happens in other constructors, such as V8SharedWorker::constructorCallback().
Comment 6 anton muhin 2011-09-02 04:05:54 PDT
Comment on attachment 106074 [details]
Rebased Patch

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

Overall, won't WithUndefinedOrNullCheck attribute do what you are looking for?

In general I am not sure (but I didn't check) that the current implementation correctly processes null, but if your policy regarding null is different, I would still suggest adding another policy to V8Parameter instead of brand-new attribute.

May you give it a try?

And probably add a test to see what happens with new WebKitCSSMatrix(null)

> Source/WebCore/bindings/v8/V8Binding.h:390
> +            if (m_v8Object.IsEmpty())

I would put it differently: if (m_v8Object.IsEmpty()) {  setString(String(); return true; } and remove all special cases below.
Comment 7 Kentaro Hara 2011-09-02 11:05:36 PDT
Created attachment 106159 [details]
Patch
Comment 8 Kentaro Hara 2011-09-02 11:05:46 PDT
(In reply to comment #6)
> (From update of attachment 106074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106074&action=review
> 
> Overall, won't WithUndefinedOrNullCheck attribute do what you are looking for?
> 
> In general I am not sure (but I didn't check) that the current implementation correctly processes null, but if your policy regarding null is different, 

No.

First of all, as far as I see the current implementation of V8WebKitCSSMatrixConstructor.cpp, the IDL of the WebKitCSSMatrix constructor should be Constructor(optional DOMString string), although there is no spec for the IDL since WebKitCSSMatrix is WebKit-specific. 

Given that we assume the IDL, the expected behavior is as follows (The spec is here: http://www.w3.org/TR/WebIDL/#es-DOMString):

- WebKitCSSMatrix() -> a null string is passed to WebKitCSSMatrix::create().
- WebKitCSSMatrix(null) -> a string "null" is passed to WebKitCSSMatrix::create().
- WebKitCSSMatrix(undefined) -> a string "undefined" is passed to WebKitCSSMatrix::create().

On the other hand, WithUndefinedOrNullCheck behaves as follows:

- WebKitCSSMatrix() -> a string "undefined" is passed to WebKitCSSMatrix::create().
- WebKitCSSMatrix(null) -> a null string is passed to WebKitCSSMatrix::create().
- WebKitCSSMatrix(undefined) -> a null string is passed to WebKitCSSMatrix::create().

> I would still suggest adding another policy to V8Parameter instead of brand-new attribute.

I guess that adding another policy would not solve this problem. The essence is that in the current implementation there is no way to create a V8Parameter object that expresses a null String (or AtomicString). This patch enables to create the V8Parameter object that expresses a null String.


> And probably add a test to see what happens with new WebKitCSSMatrix(null)

Added it to custom-constructors.html.
Comment 9 Kentaro Hara 2011-09-02 11:06:00 PDT
Comment on attachment 106074 [details]
Rebased Patch

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

>> Source/WebCore/bindings/v8/V8Binding.h:390
>> +            if (m_v8Object.IsEmpty())
> 
> I would put it differently: if (m_v8Object.IsEmpty()) {  setString(String(); return true; } and remove all special cases below.

Does "all special cases below" mean the object().IsEmpty() checks in the line 459 and 469?

I think that we cannot remove those object().IsEmpty() checks, anyway. Please note that it is not object()->IsEmpty() but object().IsEmpty(). If we omit the object().IsEmpty() checks, the following object()->IsNull() causes SEGV if object() does not exist.
Comment 10 Kentaro Hara 2011-09-02 11:12:33 PDT
FYI: The generated code for the WebKitCSSMatrix constructor is as follows.

v8::Handle<v8::Value> V8WebKitCSSMatrix::constructorCallback(const v8::Arguments& args)
{
    INC_STATS("DOM.WebKitCSSMatrix.Constructor");

    if (!args.IsConstructCall())
        return throwError("DOM object constructor cannot be called as a function.", V8Proxy::TypeError);


    ExceptionCode ec = 0;
    V8Parameter<> cssValue;
    if (args.Length() >= 1)
        cssValue = V8Parameter<>(args[0]);
    if (!cssValue.prepare())
        return v8::Undefined();

    RefPtr<WebKitCSSMatrix> obj = WebKitCSSMatrix::create(cssValue, ec);
    if (ec)
        goto fail;

    V8DOMWrapper::setDOMWrapper(args.Holder(), &info, obj.get());
    obj->ref();
    V8DOMWrapper::setJSWrapperForDOMObject(obj.get(), v8::Persistent<v8::Object>::New(args.Holder()));
    return args.Holder();
  fail:
    return throwError(ec);
}
Comment 11 anton muhin 2011-09-02 11:22:03 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > (From update of attachment 106074 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=106074&action=review
> > 
> > Overall, won't WithUndefinedOrNullCheck attribute do what you are looking for?
> > 
> > In general I am not sure (but I didn't check) that the current implementation correctly processes null, but if your policy regarding null is different, 
> 
> No.
> 
> First of all, as far as I see the current implementation of V8WebKitCSSMatrixConstructor.cpp, the IDL of the WebKitCSSMatrix constructor should be Constructor(optional DOMString string), although there is no spec for the IDL since WebKitCSSMatrix is WebKit-specific. 
> 
> Given that we assume the IDL, the expected behavior is as follows (The spec is here: http://www.w3.org/TR/WebIDL/#es-DOMString):
> 
> - WebKitCSSMatrix() -> a null string is passed to WebKitCSSMatrix::create().
> - WebKitCSSMatrix(null) -> a string "null" is passed to WebKitCSSMatrix::create().
> - WebKitCSSMatrix(undefined) -> a string "undefined" is passed to WebKitCSSMatrix::create().
> 
> On the other hand, WithUndefinedOrNullCheck behaves as follows:
> 
> - WebKitCSSMatrix() -> a string "undefined" is passed to WebKitCSSMatrix::create().
> - WebKitCSSMatrix(null) -> a null string is passed to WebKitCSSMatrix::create().
> - WebKitCSSMatrix(undefined) -> a null string is passed to WebKitCSSMatrix::create().

This strikes me as very un-JSy: in JS foo() and foo(undefined) is almost indistingishable and it appears sketchy to me distingish between those in DOM.

> 
> > I would still suggest adding another policy to V8Parameter instead of brand-new attribute.
> 
> I guess that adding another policy would not solve this problem. The essence is that in the current implementation there is no way to create a V8Parameter object that expresses a null String (or AtomicString). This patch enables to create the V8Parameter object that expresses a null String.

If by null string you mean result of String() (or isNull()), then apparently V8Parameter<WithNullCheck> and V8Parameter<WithUndefinedOrNullCheck> provide exactly null strings in the proper conditions.  If you really like (but I honestly find it pretty much unJSy), you can implement <WithEmptyCheck> which will give you exactly your semantics.

> 
> 
> > And probably add a test to see what happens with new WebKitCSSMatrix(null)
> 
> Added it to custom-constructors.html.
Comment 12 anton muhin 2011-09-02 11:27:18 PDT
(In reply to comment #9)
> (From update of attachment 106074 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106074&action=review
> 
> >> Source/WebCore/bindings/v8/V8Binding.h:390
> >> +            if (m_v8Object.IsEmpty())
> > 
> > I would put it differently: if (m_v8Object.IsEmpty()) {  setString(String(); return true; } and remove all special cases below.
> 
> Does "all special cases below" mean the object().IsEmpty() checks in the line 459 and 469?
> 
> I think that we cannot remove those object().IsEmpty() checks, anyway. Please note that it is not object()->IsEmpty() but object().IsEmpty(). If we omit the object().IsEmpty() checks, the following object()->IsNull() causes SEGV if object() does not exist.

Yes, I meant that.  object()->IsEmpty() just doesn't exist.

You should be able to remove the checks as at least as of now, empty handles should never reach <WithNullCheck> or <WithUndefinedOrNullCheck> and, even with your change, can only appear on <Default> path.

And that's exactly the reason why I suggest you to add another conversion policy into V8Parameter.
Comment 13 Kentaro Hara 2011-09-02 11:44:07 PDT
> > - WebKitCSSMatrix() -> a null string is passed to WebKitCSSMatrix::create().
> > - WebKitCSSMatrix(null) -> a string "null" is passed to WebKitCSSMatrix::create().
> > - WebKitCSSMatrix(undefined) -> a string "undefined" is passed to WebKitCSSMatrix::create().
> 
> This strikes me as very un-JSy: in JS foo() and foo(undefined) is almost indistingishable and it appears sketchy to me distingish between those in DOM.

I feel so! However, the spec does not say so (http://www.w3.org/TR/WebIDL/#es-DOMString and http://www.w3.org/TR/WebIDL/#es-interface-call). You can find the similar discussion here (https://bugs.webkit.org/show_bug.cgi?id=62288).

> > I guess that adding another policy would not solve this problem. The essence is that in the current implementation there is no way to create a V8Parameter object that expresses a null String (or AtomicString). This patch enables to create the V8Parameter object that expresses a null String.
> 
> If by null string you mean result of String() (or isNull()), then apparently V8Parameter<WithNullCheck> and V8Parameter<WithUndefinedOrNullCheck> provide exactly null strings in the proper conditions.  If you really like (but I honestly find it pretty much unJSy), you can implement <WithEmptyCheck> which will give you exactly your semantics.

Hmmm, sorry for the confusion but I still think that we cannot implement <WithEmptyCheck>.

The current implementation does not have a V8Parameter constructor with no arguments. Thus, the following code is not allowed:

(Code I):
    V8Parameter<> cssValue;
    if (args.Length() >= 1)
        cssValue = V8Parameter<>(args[0]);

Instead, we will need to write as follows:

(Code II):
    V8Parameter<> cssValue(args[0]);

Here, please note that args[0] returns v8::Undefined() if args[0] does not exist. args[0] also returns v8::Undefined() if args[0] exists and the value is undefined. Therefore, we cannot distinguish inside V8Parameter code whether the argument is omitted or the value of argument is undefined. To distinguish these two cases, I think that we need to allow a V8Parameter constructor with no arguments and write a code like the Code I.
Comment 14 Alexey Proskuryakov 2011-09-02 12:43:48 PDT
> This strikes me as very un-JSy: in JS foo() and foo(undefined) is almost indistinguishable

Historically, built-in JavaScript functions and HTML DOM ones had different conventions in this regard.
Comment 15 anton muhin 2011-09-07 04:06:39 PDT
Comment on attachment 106159 [details]
Patch

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

First of all, sorry for delay.  And thanks a lot for clarifications.  I still dare to suggest approach which I find more systematic, but feel free to ignore the suggestion.  I do not LGTM it yet as I have two separate question, but I think this is pretty much close to LGTM.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1419
> +        # Optional arguments with default values [Optional=CallWithDefaultValue] or [Optional=CallWithNullValue] should not generate an early call.

Are you sure it's a valid semantics?

For f([CallWithNullValue] a) we'll get

WebCoreTypeOfA a = convert(args[0]);

So we convert undefined to the object.  You'll generate exactly the same code for WithNullValue.  Are you sure only strings need this distinction between CallWithDefault and CallWithNull?  For example ToNumber(undefined) = Nan and that may apply to floats.

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1460
> +            if ($optional && $optional eq "CallWithNullValue") {

Sorry for being repetitive, but still one another suggestion.  Idea: if we need this distinction (missing parameter vs. undefined), let's make clear:

V8Binding.h:
enum Policy {
  MISSING_IS_UNDEFINED,
  MISSING_IS_EMPTY
}

v8::Handle<v8::Value> parameter(v8::Arguments& args, unsigned index, Policy policy) {
  if ((policy == MISSING_IS_EMPTY) && (index < args.Length()))
    return v8::Handle<v8::Value>();
  return args[index];
}

And now instead direct args[i] we use parameter(args, i, properPolicy).

WDYT?

> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1470
> +                    $parameterCheckString .= "    $nativeType $parameterName(args[$paramIndex], true);\n";

that's probably not what you want: if didn't pass CallWithNullValue'd DOMUserData, it will convert it to WebCore string "undefined"
Comment 16 anton muhin 2011-09-07 04:07:22 PDT
(In reply to comment #14)
> > This strikes me as very un-JSy: in JS foo() and foo(undefined) is almost indistinguishable
> 
> Historically, built-in JavaScript functions and HTML DOM ones had different conventions in this regard.

Thanks a lot, Alexey.
Comment 17 Kentaro Hara 2011-09-07 16:26:21 PDT
Created attachment 106670 [details]
Patch
Comment 18 Kentaro Hara 2011-09-07 16:27:04 PDT
Comment on attachment 106159 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1419
>> +        # Optional arguments with default values [Optional=CallWithDefaultValue] or [Optional=CallWithNullValue] should not generate an early call.
> 
> Are you sure it's a valid semantics?
> 
> For f([CallWithNullValue] a) we'll get
> 
> WebCoreTypeOfA a = convert(args[0]);
> 
> So we convert undefined to the object.  You'll generate exactly the same code for WithNullValue.  Are you sure only strings need this distinction between CallWithDefault and CallWithNull?  For example ToNumber(undefined) = Nan and that may apply to floats.

You are right. When CallWithNullValue is set, we need to handle a missing argument as null not only for DOMString but also other type objects. Fixed.

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1460

> 
> Sorry for being repetitive, but still one another suggestion.  Idea: if we need this distinction (missing parameter vs. undefined), let's make clear:
> 
> V8Binding.h:
> enum Policy {
>   MISSING_IS_UNDEFINED,
>   MISSING_IS_EMPTY
> }
> 
> v8::Handle<v8::Value> parameter(v8::Arguments& args, unsigned index, Policy policy) {
>   if ((policy == MISSING_IS_EMPTY) && (index < args.Length()))
>     return v8::Handle<v8::Value>();
>   return args[index];
> }
> 
> And now instead direct args[i] we use parameter(args, i, properPolicy).
> 
> WDYT?

Makes sense! Followed your idea. (I named the method 'maybeMissingParameter', since 'parameter' is already used in another place.)

>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1470
>> +                    $parameterCheckString .= "    $nativeType $parameterName(args[$paramIndex], true);\n";
> 
> that's probably not what you want: if didn't pass CallWithNullValue'd DOMUserData, it will convert it to WebCore string "undefined"

You are definitely right. We should handle a missing argument as null for all type objects. Fixed.
Comment 19 anton muhin 2011-09-08 07:51:12 PDT
Comment on attachment 106670 [details]
Patch

LGTM!

Although I am slightly concerned that we can hit empty path due to some bugs (see the comment you removed).  But that's definitely not blocking.

Thanks a lot for the nice patch.
Comment 20 Kentaro Hara 2011-09-08 08:17:25 PDT
(In reply to comment #19)
> (From update of attachment 106670 [details])
> LGTM!
> 
> Although I am slightly concerned that we can hit empty path due to some bugs (see the comment you removed).  But that's definitely not blocking.
> 
> Thanks a lot for the nice patch.

antonm: Thanks for the dedicated review.

abarth: Would you please take a look (as a reviewer)?
Comment 21 Kentaro Hara 2011-09-08 08:29:26 PDT
Created attachment 106740 [details]
Updated run-binding-tests results
Comment 22 Kentaro Hara 2011-09-08 11:09:45 PDT
Created attachment 106765 [details]
Rebased patch
Comment 23 Adam Barth 2011-09-09 10:09:08 PDT
Comment on attachment 106765 [details]
Rebased patch

Does this have any perf impact?  It looks like we're adding a bunch of function calls to a number of hot code paths.  Probably the Dromaeo DOM benchmark is a good one to run.
Comment 24 Kentaro Hara 2011-09-09 12:38:50 PDT
Created attachment 106904 [details]
Patch
Comment 25 Kentaro Hara 2011-09-09 12:39:55 PDT
(In reply to comment #23)
> (From update of attachment 106765 [details])
> Does this have any perf impact?  It looks like we're adding a bunch of function calls to a number of hot code paths.  Probably the Dromaeo DOM benchmark is a good one to run.

I changed the function to MAYBE_MISSING_PARAMETER() macro.
Comment 26 Adam Barth 2011-09-09 16:55:53 PDT
Comment on attachment 106904 [details]
Patch

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

> Source/WebCore/bindings/v8/V8Binding.h:477
> +        MISSING_IS_UNDEFINED,
> +        MISSING_IS_EMPTY

These names should be in CamelCase instead of ALL_CAPS.
Comment 27 Adam Barth 2011-09-09 16:57:04 PDT
Comment on attachment 106904 [details]
Patch

It seems like it's about time to add the JSC implementation of these attribute.  Is there some reason we haven't done it yet.
Comment 28 Kentaro Hara 2011-09-09 17:01:16 PDT
> These names should be in CamelCase instead of ALL_CAPS.

Sorry, I'll fix it.

(In reply to comment #27)
> (From update of attachment 106904 [details])
> It seems like it's about time to add the JSC implementation of these attribute.  Is there some reason we haven't done it yet.

Yes. After replacing the V8 custom constructor of SharedWorker, I am planning to implement [Constructor] in JSC. Thanks!
Comment 29 Roland Steiner 2011-09-09 17:28:11 PDT
Committed r94891: <http://trac.webkit.org/changeset/94891>
Comment 30 Kentaro Hara 2011-10-12 01:34:01 PDT
*** Bug 66276 has been marked as a duplicate of this bug. ***