[webkit-reviews] review denied: [Bug 38279] Add CPP bindings generator : [Attachment 54683] Updated patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 38279: Add CPP bindings generator
https://bugs.webkit.org/show_bug.cgi?id=38279

Attachment 54683: Updated patch
https://bugs.webkit.org/attachment.cgi?id=54683&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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?