Summary: | V8/JS bindings should not perform type checks if the parameter has Callback attribute | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kinuko Yasuda <kinuko> | ||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, darin, dumi | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Attachments: |
|
Description
Kinuko Yasuda
2010-09-02 15:49:14 PDT
Created attachment 66450 [details]
Patch
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. Created attachment 66469 [details]
Patch
(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.) Comment on attachment 66469 [details]
Patch
r=me.
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!
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.
Comment on attachment 66469 [details]
Patch
Oops, I see now. A compilation error. My bad. Adding back Dumi’s review+.
(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. (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. Committed r66791: <http://trac.webkit.org/changeset/66791> |