[Webkit-unassigned] [Bug 16573] Remove redundant calls to setPrototype in WebCore

[bugzilla-daemon at webkit.org]

http://bugs.webkit.org/show_bug.cgi?id=16573





------- Comment #8 from sam at webkit.org  2008-01-02 23:32 PDT -------
> 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.

I used JSValue because in one place (for the JSLocation object) we still use
jsNull() as the prototype (thought this is a bug) and because the prototype
ends up being converted to a JSValue when it is stored in _proto.  (Also, in a
complete cop-out, Geoff told me too :) ).

> +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.

Fixed.

> 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.

I only changed the objects that represent real wrapped objects to take a
prototype instead of an execState.   The objects excluded were Constructors,
like JSHTMLAudioElementConstructor and prototypes themselves.  I tried to be
consistent in this manner.

> +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.

This is actually an implementation of the [[Construct]] internal property and
is an implementation of the method defined in JSObject.  This cannot be
changed.

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

Sure!

> +        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).

Fixed.

> +    private:
> +        DOMObject();
>
> What is this for? Does someone need to construct a DOMObject this way? Can you
> instead just omit this constructor?

I added this to try and make it impossible to create a derived class that
didn't pass a prototype.  Will this not work as intended?

> 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.

I changed it to be uniform among the constructors and because I want to fix the
bug shortly.

I don't think it is worth posting a new patch with just the 2 changes mentioned
above, but I will wait for your response before going forward.  Thanks for the
timely review!


-- 
Configure bugmail: http://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.