Bug 112475 – Add IDL 'enum' support to CodeGeneratorJS.pm

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[arno.]

Hi,
this bug is to add enum support for JavaScriptCore bindings. It is similar to what has been done for V8 in bug #106553.

[arno.]

Created attachment 193394 [details]
patch proposal

[arno.]

Created attachment 193411 [details]
updated patch: check for exception after calling toString()

[Kentaro Hara]

Comment on attachment 193411 [details]
updated patch: check for exception after calling toString()

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2134
> +                                    push(@implContent, "    String string = value.isEmpty() ? String() : value.toString(exec)->value(exec);\n");

Don't you need to add 'if (exec->hadException()) return' ?

[arno.]

Created attachment 193586 [details]
updated patch: second check for exception after calling toString()

[arno.]

Created attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

[Kentaro Hara]

Comment on attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

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

LGTM, thanks!

> Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:1892
> +    if (args.Length() < 1)
> +        return throwNotEnoughArgumentsError(args.GetIsolate());
> +    TestObj* imp = V8TestObj::toNative(args.Holder());
> +    V8TRYCATCH_FOR_V8STRINGRESOURCE(V8StringResource<>, enumArg, args[0]);
> +    imp->methodWithEnumArg(enumArg);
> +    return v8Undefined();

If you have time, I'd be happy if you could support 'enum' in DOM methods in V8 as well.

[WebKit Review Bot]

Comment on attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

Clearing flags on attachment: 193621

Committed r146161: <http://trac.webkit.org/changeset/146161>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2152
> +                                    push(@implContent, "    String& string = value.isEmpty() ? String() : value.toString(exec)->value(exec);\n");

This would work with const String&, but won’t work with String& — it will likely simply fail to compile.

[Kentaro Hara]

Comment on attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2152
> +                                    push(@implContent, "    String& string = value.isEmpty() ? String() : value.toString(exec)->value(exec);\n");

Sorry, this is wrong. This should be String, not String&. If you are concerned about performance of String comparisons below, we should optimize it somehow. However, I think it is OK to just use String until we find any scenarios in which the String comparisons are bottlenecks.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2794
> +            push(@$outputArray, "    const String& ${name}(${argValue}.isEmpty() ? String() : ${argValue}.toString(exec)->value(exec));\n");

Ditto.

[Kentaro Hara]

(In reply to comment #9)
> (From update of attachment 193621 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193621&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2152
> > +                                    push(@implContent, "    String& string = value.isEmpty() ? String() : value.toString(exec)->value(exec);\n");
> 
> This would work with const String&, but won’t work with String& — it will likely simply fail to compile.

Ah, const String& sounds the best.

[Kentaro Hara]

arno: would you fix it please?

[Darin Adler]

Comment on attachment 193621 [details]
patch modification: use a reference to String to avoid a copy

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

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2152
>>> +                                    push(@implContent, "    String& string = value.isEmpty() ? String() : value.toString(exec)->value(exec);\n");
>> 
>> This would work with const String&, but won’t work with String& — it will likely simply fail to compile.
> 
> Sorry, this is wrong. This should be String, not String&. If you are concerned about performance of String comparisons below, we should optimize it somehow. However, I think it is OK to just use String until we find any scenarios in which the String comparisons are bottlenecks.

I don’t think that const String& will give better performance than String. In either case there is still a local String value. The use of the reference might seem to eliminate a copy, but in practice is does not. So please just change back to "String" for clarity.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2794
>> +            push(@$outputArray, "    const String& ${name}(${argValue}.isEmpty() ? String() : ${argValue}.toString(exec)->value(exec));\n");
> 
> Ditto.

Ditto.

[Kentaro Hara]

The fix is going to be landed in bug 112760.