| Summary: | Add CPP bindings generator | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Component: | WebCore Misc. | Assignee: | Nikolas Zimmermann <zimmermann> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, ap, dglazkov, dumi, krit, lists.bugzilla, mjs, sam, staikos, webkit.review.bot, zecke, zimmermann | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Nikolas Zimmermann
2010-04-28 13:22:36 PDT
Created attachment 54614 [details]
Initial patch
Initial version of the CPP bindings generator.
Attachment 54614 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1882123 Comment on attachment 54614 [details]
Initial patch
Cool! Can you please integrate with run-bindings-tests so we can test the code generator included in this patch?
Ah great idea! Forgot about this nifty new tool, glad you wrote it Adam. New patch coming, this time also building on Chromium. Created attachment 54683 [details]
Updated patch
Integrated with run-bindings-test, should build on Chromium as well. Let's see...
Attachment 54683 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:29: "WebDOMTestObj.h" already included at WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:22 [build/include] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:33: Alphabetical sorting problem. [build/include_order] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:391: More than one command on the same line [whitespace/newline] [4]
WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:30: Code inside a namespace should not be indented. [whitespace/indent] [4]
Total errors found: 4 in 43 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > Attachment 54683 [details] did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" > exit_code: 1 > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:29: "WebDOMTestObj.h" > already included at WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:22 > [build/include] [4] > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:33: Alphabetical sorting > problem. [build/include_order] [4] > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:391: More than one command > on the same line [whitespace/newline] [4] > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:30: Code inside a namespace > should not be indented. [whitespace/indent] [4] > Total errors found: 4 in 43 files I guesss these style errors are fine. I _could_ tweak code generation to make this file pass, but: a) it's not worth it - other files would have other problems b) the other generated TestObj* files, have style issues as well All other files don't show errors, so please ignore the red style bot. > I guesss these style errors are fine. I _could_ tweak code generation to make
> this file pass, but:
> a) it's not worth it - other files would have other problems
> b) the other generated TestObj* files, have style issues as well
We've been fixing style errors is the generated JSC and V8 bindings as we've been finding them. GObject seems to be in Gtk style (not WebKit style). I think this fall squarely in the "nice to have" category.
Comment on attachment 54683 [details] Updated patch Looks pretty neat. These are mostly questions and things to think about. WebCore/bindings/cpp/WebDOMAttrCustom.cpp:26 + WebDOMString WebDOMAttr::value() const Why can't this be autogenerated? WebCore/bindings/cpp/WebDOMAttrCustom.cpp:34 + void WebDOMAttr::setValue(const WebDOMString& value) Ditto. WebCore/bindings/cpp/WebDOMCString.cpp:34 + m_private->deref(); Manual ref counting is sad face. WebCore/bindings/cpp/WebDOMCString.h:78 + WebDOMCStringPrivate* m_private; Can't this be a RefPtr? WebCore/bindings/cpp/WebDOMElementCustom.cpp:29 + void WebDOMElement::setAttribute(const WebDOMString& name, const WebDOMString& value) Again, seems generatable. WebCore/bindings/cpp/WebDOMEventListenerCustom.cpp:35 + WebDOMEventListener webKit(WebUserEventListener* value) Maybe toWebKit? This name seems very generic to put in the global namespace. WebCore/bindings/cpp/WebDOMEventTarget.cpp:33 + WebCore::EventTarget* webCore(const WebDOMEventTarget&) Also a very generic name. WebCore/bindings/cpp/WebDOMEventTarget.h:29 + class WebDOMEventTarget { I wonder if you don't want to put all of this into some namespace... WebCore/bindings/cpp/WebDOMHTMLDocumentCustom.cpp:28 + void WebDOMHTMLDocument::open() This really seems autogeneratable. WebCore/bindings/cpp/WebDOMHTMLFrameElementCustom.cpp:26 + WebDOMString WebDOMHTMLFrameElement::src() const No security check, huh? Interesting. WebCore/bindings/cpp/WebDOMHTMLFrameElementCustom.cpp:42 + void WebDOMHTMLFrameElement::setLocation(const WebDOMString& newLocation) I think you need an attribute in the IDL that's like CPPNotCustom so you know that you can autogenerate all this this stuff even though other bindings need custom implementations. WebCore/bindings/cpp/WebDOMNodeFilterCustom.cpp:26 + short WebDOMNodeFilter::acceptNode(const WebDOMNode& n) I think this recently became not [Custom]. You might need to check at TOT. WebCore/bindings/cpp/WebDOMObject.h:36 + // UTF-16 character type This seems like the wrong place for this declaration. WebCore/bindings/cpp/WebDOMString.cpp:80 + m_private->ref(); Again, manual ref counting. WebCore/bindings/cpp/WebExceptionHandlers.cpp:26 + // FIXME: Implement exception handling I don't really see how you're going to do this. WebCore/bindings/cpp/WebNativeEventListener.cpp:30 + m_listener->ref(); :*( WebCore/bindings/scripts/CodeGeneratorCPP.pm:126 + # Start actual generation.. Extra . here. WebCore/bindings/scripts/CodeGeneratorCPP.pm:148 + return "WebDOMString" if $codeGenerator->IsStringType($name) or $name eq "SerializedScriptValue"; Hum... Representing SerializedScriptValue as a string is an interesting choice. WebCore/bindings/scripts/CodeGeneratorCPP.pm:203 + # TODO: We don't generate bindings for SVG related interfaces yet FIXME instead of TODO. WebCore/bindings/scripts/CodeGeneratorCPP.pm:329 + sub GenerateHeader This function is too large. Please split into smaller functions. WebCore/bindings/scripts/CodeGeneratorCPP.pm:487 + push(@headerContent, " ${className}Private* m_d;\n"); Is it best to hold a raw pointer here? WebCore/bindings/scripts/CodeGeneratorCPP.pm:589 + push(@implContent, " ${className}Private($implClassNameWithNamespace* _impl = 0)\n"); This name is out of style. WebCore/bindings/scripts/CodeGeneratorCPP.pm:590 + push(@implContent, " : impl(_impl)\n"); Maybe m_impl? WebCore/bindings/scripts/CodeGeneratorCPP.pm:593 + push(@implContent, " RefPtr<$implClassNameWithNamespace> impl;\n"); Ah, I see. You're using a raw pointer to an object that has a RefPtr. Why not just hold the RefPtr in the public class? WebCore/bindings/scripts/CodeGeneratorCPP.pm:614 + push(@implContent, " m_d = copy.impl() ? new ${className}Private(copy.impl()) : 0;\n"); Manual new/delete is sadness. WebCore/bindings/scripts/CodeGeneratorCPP.pm:658 + # legacy behavior. (see http://bugs.webkit.org/show_bug.cgi?id=10889) Why do we have legacy behavior in a new bindings layer? WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:99 + if (!impl()) Calling impl() is slow. You might consider caching it in a local variable, like we do in the JSC bindings. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:121 + void WebDOMTestObj::setLongLongAttr(const long long& newLongLongAttr) const long long& ? That's pretty strange. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:166 + return webKit(WTF::getPtr(impl()->testObjAttr())); toWebKit or toCPP seems more consistent with toJS in the JSC bindings. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:304 + impl()->withDynamicFrame(); You don't implement this attribute yet, but that's fine. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:360 + void WebDOMTestObj::methodWithOptionalArg(const long& opt) You don't seem to implement [Optional] either. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:391 + { WEBDOM_ASSERT_MAIN_THREAD(); WebCoreThreadViolationCheckRoundOne(); }; These shouldn't be in the same line. WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:99 + #endif Missing namespace comment on #endif WebCore/page/AbstractView.idl:35 + readonly attribute StyleMedia styleMedia; Is this change correct? (In reply to comment #9) > (From update of attachment 54683 [details]) > Looks pretty neat. These are mostly questions and things to think about. > > WebCore/bindings/cpp/WebDOMAttrCustom.cpp:26 > + WebDOMString WebDOMAttr::value() const > Why can't this be autogenerated? > > WebCore/bindings/cpp/WebDOMAttrCustom.cpp:34 > + void WebDOMAttr::setValue(const WebDOMString& value) > Ditto. Will investigate. > > WebCore/bindings/cpp/WebDOMCString.cpp:34 > + m_private->deref(); > Manual ref counting is sad face. > > WebCore/bindings/cpp/WebDOMCString.h:78 > + WebDOMCStringPrivate* m_private; > Can't this be a RefPtr? No, we can't use WTF API in those public headers. Note that WebDOMCString & WebDOMString are copied from chromium, which handles it exactly the same way, see WebKit/chromium/public/. But I agree manual refcounting is a PITA, fortunately this is a rare place. > WebCore/bindings/cpp/WebDOMElementCustom.cpp:29 > + void WebDOMElement::setAttribute(const WebDOMString& name, const > WebDOMString& value) > Again, seems generatable. Will investigate. > > WebCore/bindings/cpp/WebDOMEventListenerCustom.cpp:35 > + WebDOMEventListener webKit(WebUserEventListener* value) > Maybe toWebKit? This name seems very generic to put in the global namespace. Oh, chosen to work like ObjC api, which uses the same naming convention (even simpler: core/kit, and I prefixed it with 'web' to avoid these very generic names). If you prefer we could add a 'to' prefix though. I'm for it :-) > > WebCore/bindings/cpp/WebDOMEventTarget.cpp:33 > + WebCore::EventTarget* webCore(const WebDOMEventTarget&) > Also a very generic name. See above. > > WebCore/bindings/cpp/WebDOMEventTarget.h:29 > + class WebDOMEventTarget { > I wonder if you don't want to put all of this into some namespace... Hm, need to think about it. George, what do you say? > > WebCore/bindings/cpp/WebDOMHTMLDocumentCustom.cpp:28 > + void WebDOMHTMLDocument::open() > This really seems autogeneratable. True! Must be an old leftover... > > WebCore/bindings/cpp/WebDOMHTMLFrameElementCustom.cpp:26 > + WebDOMString WebDOMHTMLFrameElement::src() const > No security check, huh? Interesting. We handle security checks in another layer on top of this.... > WebCore/bindings/cpp/WebDOMHTMLFrameElementCustom.cpp:42 > + void WebDOMHTMLFrameElement::setLocation(const WebDOMString& newLocation) > I think you need an attribute in the IDL that's like CPPNotCustom so you know > that you can autogenerate all this this stuff even though other bindings need > custom implementations. Aha good idea. > > WebCore/bindings/cpp/WebDOMNodeFilterCustom.cpp:26 > + short WebDOMNodeFilter::acceptNode(const WebDOMNode& n) > I think this recently became not [Custom]. You might need to check at TOT. Yeah, I have an updated patch. Will upload it after I fixed all the other issues :-) > > WebCore/bindings/cpp/WebDOMObject.h:36 > + // UTF-16 character type > This seems like the wrong place for this declaration. Yeah, but I did not want another header. This is just a convenience place to share between the two string classes. > WebCore/bindings/cpp/WebDOMString.cpp:80 > + m_private->ref(); > Again, manual ref counting. Same reason as for WebDOMCString. > > WebCore/bindings/cpp/WebExceptionHandlers.cpp:26 > + // FIXME: Implement exception handling > I don't really see how you're going to do this. We could provide a "WebUserExceptionHandler" that gets called here, aka. a callback. Did not yet think about that. > > WebCore/bindings/cpp/WebNativeEventListener.cpp:30 > + m_listener->ref(); > :*( I know, I know ;-) > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:126 > + # Start actual generation.. > Extra . here. Will remove. > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:148 > + return "WebDOMString" if $codeGenerator->IsStringType($name) or $name > eq "SerializedScriptValue"; > Hum... Representing SerializedScriptValue as a string is an interesting > choice. Same as ObjC, IIRC. > WebCore/bindings/scripts/CodeGeneratorCPP.pm:203 > + # TODO: We don't generate bindings for SVG related interfaces yet > FIXME instead of TODO. Right. > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:329 > + sub GenerateHeader > This function is too large. Please split into smaller functions. Will check. > WebCore/bindings/scripts/CodeGeneratorCPP.pm:487 > + push(@headerContent, " ${className}Private* m_d;\n"); > Is it best to hold a raw pointer here? Yeah, definately. We're not sharing the d-ptrs across other WebDOM* objects. > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:589 > + push(@implContent, " > ${className}Private($implClassNameWithNamespace* _impl = 0)\n"); > This name is out of style. What do you mean? > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:590 > + push(@implContent, " : impl(_impl)\n"); > Maybe m_impl? m_d->m_impl looks odd IMHO, that's why I've used the impl(_impl) hackery here once, instead of deploying m_d->m_impl everywhere. What do you think, okay this way? > WebCore/bindings/scripts/CodeGeneratorCPP.pm:593 > + push(@implContent, " RefPtr<$implClassNameWithNamespace> > impl;\n"); > Ah, I see. You're using a raw pointer to an object that has a RefPtr. Why not > just hold the RefPtr in the public class? For the reason mention above, to hide WTF usage. (We can't use any of WTF/WebCore in olympia, from the outside - WebDOM* API must be self-contained) > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:614 > + push(@implContent, " m_d = copy.impl() ? new > ${className}Private(copy.impl()) : 0;\n"); > Manual new/delete is sadness. :-) > > WebCore/bindings/scripts/CodeGeneratorCPP.pm:658 > + # legacy behavior. (see > http://bugs.webkit.org/show_bug.cgi?id=10889) > Why do we have legacy behavior in a new bindings layer? Copy/Paste :-) > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:99 > + if (!impl()) > Calling impl() is slow. You might consider caching it in a local variable, > like we do in the JSC bindings. Depends on the compiler, but yes, will investigate. > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:121 > + void WebDOMTestObj::setLongLongAttr(const long long& newLongLongAttr) > const long long& ? That's pretty strange. Well, we're always using "const Type&" everywhere. It may look strange, but it's completly fine. Wheter we're using "long long" or "const long long&" here doesn't matter. But I could definately change that, if desired. Would like to hear Georges voice as well here! > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:166 > + return webKit(WTF::getPtr(impl()->testObjAttr())); > toWebKit or toCPP seems more consistent with toJS in the JSC bindings. Okay. As I said, it's from the ObjC bindings which use kit/core. But agreed, it's too confusing. > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:304 > + impl()->withDynamicFrame(); > You don't implement this attribute yet, but that's fine. Ok. > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:360 > + void WebDOMTestObj::methodWithOptionalArg(const long& opt) > You don't seem to implement [Optional] either. Right. > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.cpp:391 > + { WEBDOM_ASSERT_MAIN_THREAD(); WebCoreThreadViolationCheckRoundOne(); > }; > These shouldn't be in the same line. Okay. Copy/Paste from ObjC generator. > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:99 > + #endif > Missing namespace comment on #endif On purpose. It's not a style rule anymore IIRC. And I dislike it. > > WebCore/page/AbstractView.idl:35 > + readonly attribute StyleMedia styleMedia; > Is this change correct? Yes "Media.h" does not exist. The generated JS bindings etc won't compile - but no one compiles JSAbstractView, so no one noticed. Thanks A LOT for the review! Didn't expect a fast reply like this! Going to prepare a new patch. (In reply to comment #10) > > WebCore/bindings/scripts/test/CPP/WebDOMTestObj.h:99 > > + #endif > > Missing namespace comment on #endif > On purpose. It's not a style rule anymore IIRC. And I dislike it. And some compilers warn about it which is annoying. Okay, I take back my comments regarding the style problems: nikolaszimmermann ~/Coding/WebKit/WebCore/bindings/cpp > check-webkit-style generated/*.* Total errors found: 0 in 476 files Can now generate all files w/o any style problem. Also lots more can be autogenerated, glad for all the comments.... going to upload a new patch soon. Created attachment 54796 [details]
Updated patch v2
Fixed most issues Adam raised, except splitting up GenerateHeader/Implementation in smaller pieces.
I'd love to do that in a follow-up patch, cleaning up the whole CodeGeneratorCPP.pm module, together with preparing the addition of adding bindings generation for SVG. Would be great if you'd be fine with that.
While I was at it:
* Implemented a callback mechanism to handle exceptions (webInstallExceptionHandler)
* Implement WebDOMEventTarget for real. Only thing missing is a way to find out what's the underlying object (Node/XMLHttpRequest etc..) -> left for a follow-up patch, because it needs discussion first with other RIM folks
* Fixed all style issues in generated code
nikolaszimmermann ~/Coding/WebKit/WebCore/bindings/cpp > check-webkit-style *.cpp *.h generated/*.cpp generated/*.h
Total errors found: 0 in 491 files
Awesome thanks. We should probably have one more bindings expert look at your patch. Also, I didn't review the Perl code in much detail because my understanding of Perl is "primitive" to say the least. Do the .h and .cpp files need to have the svn:executable property? Could you consider naming it c++? I am not sure if I am the only one associating CPP with the C Pre Processor and the environment variable which is written in capital letters as well. (In reply to comment #15) > Do the .h and .cpp files need to have the svn:executable property? Oops, they don't. Will remove them before landing. (In reply to comment #16) > Could you consider naming it c++? I am not sure if I am the only one > associating CPP with the C Pre Processor and the environment variable which is > written in capital letters as well. I don't want to risk breaking win platforms. Naming it cpp is safer than c++. I'd vote to leave it as-is. Any news on this patch? Sam, do you have some time to check it? Comments: - You should probably try to ignore the "Custom" extended attribute to begin with, we have so far gotten away with not having any custom bindings the Objective-C generator. - Keep it CPP. - I think the first iteration should try and avoid adding new methods to the IDL. We should follow up with those additions and consider generalizing them (I am referring to functions like isMutationEvent()) - I am not a fan of the name m_d, can we use m_impl? - I think you want IDL long to map to int - I don't think you pass primitives as const references (void setLongLongAttr(const long long&);) - Why are you doing the threading violation checks? Is that to match objc? Niko, can you move the following lines from WebExceptionHandler.h into a separate .h file? + typedef int WebDOMExceptionCode; + typedef void (*WebExceptionHandler)(WebDOMExceptionCode); + + // Used from the outside to register a callback that gets fired whenever an exception is raised + void webInstallExceptionHandler(WebExceptionHandler); Otherwse outside code needing to register an exception handler will need to include WebExceptionHandler.h, which includes webkit internals like wtf/Assertions.h and wtf/Threading.h. (In reply to comment #19) > Comments: > > - You should probably try to ignore the "Custom" extended attribute to begin with, we have so far gotten away with not having any custom bindings the Objective-C generator. I've only implemented the few cases, that we actually need in Olympia. > - Keep it CPP. Done. > - I think the first iteration should try and avoid adding new methods to the IDL. We should follow up with those additions and consider generalizing them (I am referring to functions like isMutationEvent()) I'd rather generalize the other way round, landing with those functions. It has no impact on other generators, I think it's just fine for now, no? > - I am not a fan of the name m_d, can we use m_impl? Fixed. > - I think you want IDL long to map to int Fixed. > - I don't think you pass primitives as const references (void setLongLongAttr(const long long&);) Looking at it. > - Why are you doing the threading violation checks? Is that to match objc? Yeah, they should be removed. (In reply to comment #20) > Niko, can you move the following lines from WebExceptionHandler.h into a separate .h file? > > + typedef int WebDOMExceptionCode; > + typedef void (*WebExceptionHandler)(WebDOMExceptionCode); > + > + // Used from the outside to register a callback that gets fired whenever an exception is raised > + void webInstallExceptionHandler(WebExceptionHandler); > > Otherwse outside code needing to register an exception handler will need to include WebExceptionHandler.h, which includes webkit internals like wtf/Assertions.h and wtf/Threading.h. I have removed the thread violation checks, it's not needed for us, and legacy code from the ObjC generator (which was the base for the cpp bindings). The WebExceptionHandler.h file doesn't include any wtf/ code anymore. That should fix the problem. Created attachment 57076 [details]
Updated patch v3
Fixed all issues reported by Sam and Kats, except one. I still export the isMutationEvent etc. helper methods, as they're in use at the moment within Olympia.
We'd like to generalize this, though I'd really love to see the patch upstreamed first. We'll come back to it for sure. Hope that's okay, as it does not affect other ports?
Sam, any news on this patch? :-) Comment on attachment 57076 [details]
Updated patch v3
Looks good. But I don't see how these files are getting built. Otherwise, r=me.
Landed in r60784. |