[webkit-reviews] review granted: [Bug 16573] Remove redundant calls to setPrototype in WebCore : [Attachment 18249] take 2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 16573: Remove redundant calls to setPrototype in WebCore
http://bugs.webkit.org/show_bug.cgi?id=16573

Attachment 18249: take 2
http://bugs.webkit.org/attachment.cgi?id=18249&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I think the prototype parameter should be JSObject unless there's some reason
it can be null in some cases. It can be more efficient to have the
more-specific type, so I'd prefer that all the prototype code use JSObject
rather than JSValue.

+JSEventTargetNode::JSEventTargetNode(KJS::JSValue* prototype, Node* n)

You should not need an explicit KJS prefix here. Please leave them off except
in header files or in places where they are necessary.

For JSHTMLAudioElementConstructor you passed the ExecState and set up the
prototype inside the constructor, but for JSNamedNodesCollection, JSRGBColor,
and Navigator you passed the prototype in instead. I'm not sure why you did
them differently, since in all cases it's the object prototype.

+JSObject* XSLTProcessorConstructorImp::construct(ExecState* exec, const List&
args)

Elsewhere we call these functions "create", I think largely because we don't
want to confuse them with constructors. Would you consider calling it create
instead of construct?

Could the return type be JSXSLTProcessor* instead of JSObject*? In general
types should always be as specific as possible unless there's a problem.

We really need to take these "Imp" suffixes off things! Can you add a lot more
of them to do-webcore-rename?

+	 DOMObject(JSValue* prototype)

This needs to be marked explicit because we don't want it to be invoked
implicitly as a conversion (even though that seems unlikely).

+    private:
+	 DOMObject();

What is this for? Does someone need to construct a DOMObject this way? Can you
instead just omit this constructor?

If JSLocation's prototype is going to be null, then why change JSLocation so
that you can pass the prototype in? You should have just left its constructor
alone, I think.

r=me as is, because none of these problems are showstoppers

If you could take some of my requests/suggestions I'd be happy to review again.