Bug 45143 – V8/JS bindings should not perform type checks if the parameter has Callback attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Kinuko Yasuda]

It causes compile error if an overloaded function has callback parameters.

[Kinuko Yasuda]

Created attachment 66450 [details]
Patch

[Dumitru Daniliuc]

Comment on attachment 66450 [details]
Patch

> WebCore/bindings/scripts/CodeGeneratorJS.pm:1137
> +        if ($codeGenerator->IsStringType($type)) {
i think it might be cleaner to format this code as:

if (IsStringType($type)) {
  ...
} elsif ($parameter->extendedAttributes->{"Callback"}) {
  ...
} elsif (!IsNativeType($type)) {
  ...
}

same comment for CodeGeneratorV8.pm.


> WebCore/bindings/scripts/test/V8/V8TestObj.cpp:-981
> -    if ((args.Length() == 2 && (args[0]->IsNull() || V8TestObj::HasInstance(args[0])) && (args[1]->IsNull() || args[1]->IsUndefined() || args[1]->IsString() || args[1]->IsObject())))
i believe you need to change JS/JSTestObj.cpp too.

[Kinuko Yasuda]

Created attachment 66469 [details]
Patch

[Kinuko Yasuda]

(In reply to comment #2)
> (From update of attachment 66450 [details])
> > WebCore/bindings/scripts/CodeGeneratorJS.pm:1137
> > +        if ($codeGenerator->IsStringType($type)) {
> i think it might be cleaner to format this code as:
> 
> if (IsStringType($type)) {
>   ...
> } elsif ($parameter->extendedAttributes->{"Callback"}) {
>   ...
> } elsif (!IsNativeType($type)) {
>   ...
> }
> 
> same comment for CodeGeneratorV8.pm.

Fixed.

Wasn't very sure if we want to check that for native types... but maybe it would contribute to readability.

> > WebCore/bindings/scripts/test/V8/V8TestObj.cpp:-981
> > -    if ((args.Length() == 2 && (args[0]->IsNull() || V8TestObj::HasInstance(args[0])) && (args[1]->IsNull() || args[1]->IsUndefined() || args[1]->IsString() || args[1]->IsObject())))
> i believe you need to change JS/JSTestObj.cpp too.

Fixed.  (The update in the previous patch was an error actually - this time I added a new overloaded method with callback arg and updated both {V8,JS}TestObj.cpp.)

[Dumitru Daniliuc]

Comment on attachment 66469 [details]
Patch

r=me.

[Darin Adler]

Comment on attachment 66469 [details]
Patch

Why did this test go in without any actual JavaScript regression tests? Presumably this was done to change real-world behavior, not just to change the bindings source code in a way that is not visible from web pages.

We don’t want bug fixes going in without regression tests. I don’t see any reason for this to be an exception!

[Darin Adler]

Comment on attachment 66469 [details]
Patch

I’m going to change this to a review- until I find out why there are no regression tests.

[Darin Adler]

Comment on attachment 66469 [details]
Patch

Oops, I see now. A compilation error. My bad. Adding back Dumi’s review+.

[Kinuko Yasuda]

(In reply to comment #8)
> (From update of attachment 66469 [details])
> Oops, I see now. A compilation error. My bad. Adding back Dumi’s review+.

Thanks for examining this... ok to go now?   Currently there's no such IDL that has overloaded function with callbacks (I was going to add one and got a compilation error).   I'll double check that this doesn't change anything for existing IDLs.

[Darin Adler]

(In reply to comment #9)
> Thanks for examining this... ok to go now?

I have no objection. I didn’t review the patch, but I see no reason to reverse Dumi’s review+ now that I understand.

[Kinuko Yasuda]

Committed r66791: <http://trac.webkit.org/changeset/66791>