Summary: | Implement [Constructor] IDL for JSC | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kentaro Hara <haraken> | ||||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, dominicc, donggwan.kim, sam, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 65839 | ||||||||||||
Attachments: |
|
Description
Kentaro Hara
2011-10-14 05:17:48 PDT
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. |