Bug 70101

Created attachment 111010 [details] Patch

Comment on attachment 111010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111010&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3266 > + # We do not also support a constructor [Opitional] argument without CallWithDefaultValue. Opitional <-- typo > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:111 > + ScriptExecutionContext* scriptContext = static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext(); > + if (!scriptContext) > + return JSValue::encode(jsUndefined()); Why is this variable check needed? It's not used in this function. > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:122 > + RefPtr<TestInterface> object = TestInterface::create(str1, str2, context, ec); This seems to be passing context at the end instead of the beginning. > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.h:91 > +class JSTestInterfaceConstructor : public DOMConstructorObject { I don't quite understand when this moved from the cpp file to the h file.

Created attachment 111177 [details] Patch

(In reply to comment #2) > (From update of attachment 111010 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111010&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3266 > > + # We do not also support a constructor [Opitional] argument without CallWithDefaultValue. > > Opitional <-- typo Fixed. > > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:111 > > + ScriptExecutionContext* scriptContext = static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext(); > > + if (!scriptContext) > > + return JSValue::encode(jsUndefined()); > > Why is this variable check needed? It's not used in this function. Removed this check. > > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:122 > > + RefPtr<TestInterface> object = TestInterface::create(str1, str2, context, ec); > > This seems to be passing context at the end instead of the beginning. Sorry, fixed.

Created attachment 111179 [details] Patch

Comment on attachment 111010 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=111010&action=review >> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.h:91 >> +class JSTestInterfaceConstructor : public DOMConstructorObject { > > I don't quite understand when this moved from the cpp file to the h file. Currently, the declaration of XXXXConstructor exists on the h file if the constructor is a custom constructor, and on the cpp file if the constructor is not a custom constructor. This is inconsistent and I think that all declarations should exist on the h file. Practical problem: For example, JSDOMWindowCustom.cpp assumes that the declaration of EventSourceConstructor exists on the h file. Currently, JSDOMWindowCustom.cpp can be compiled, since EventSourceConstructor is written as a custom constructor and thus it is declared on the h file. However, in a coming patch, I am planning to generate EventSourceConstructor by [Constructor] IDL, which will put the declaration to the cpp file (unless we move the declaration from the cpp file to the h file by this patch). This makes JSDOMWindowCustom.cpp fail to compile.

Created attachment 111180 [details] Rebased patch for review

Comment on attachment 111180 [details] Rebased patch for review ok!

Comment on attachment 111180 [details] Rebased patch for review Clearing flags on attachment: 111180 Committed r97578: <http://trac.webkit.org/changeset/97578>

All reviewed patches have been landed. Closing bug.