Bug 67458 – Generate a WebKitCSSMatrix constructor of V8 using the IDL 'Constructor' extended attribute

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

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 106072 [details]
Patch

[Kentaro Hara]

Created attachment 106074 [details]
Rebased Patch

[Adam Barth]

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.

[Adam Barth]

@antonm: Thoughts?

[Kentaro Hara]

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().

[anton muhin]

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.

[Kentaro Hara]

Created attachment 106159 [details]
Patch

[Kentaro Hara]

(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.

[Kentaro Hara]

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.

[Kentaro Hara]

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);
}

[anton muhin]

(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.

[anton muhin]

(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.

[Kentaro Hara]

> > - 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.

[Alexey Proskuryakov]

> 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.

[anton muhin]

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"

[anton muhin]

(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.

[Kentaro Hara]

Created attachment 106670 [details]
Patch

[Kentaro Hara]

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.

[anton muhin]

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.

[Kentaro Hara]

(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)?

[Kentaro Hara]

Created attachment 106740 [details]
Updated run-binding-tests results

[Kentaro Hara]

Created attachment 106765 [details]
Rebased patch

[Adam Barth]

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.

[Kentaro Hara]

Created attachment 106904 [details]
Patch

[Kentaro Hara]

(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.

[Adam Barth]

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.

[Adam Barth]

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.

[Kentaro Hara]

> 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!

[Roland Steiner]

Committed r94891: <http://trac.webkit.org/changeset/94891>

[Kentaro Hara]

*** Bug 66276 has been marked as a duplicate of this bug. ***