[webkit-reviews] review requested: [Bug 11610] XMLHttpRequest.onreadystatechange doesn't provide access to the request object : [Attachment 11865] proposed fix

[bugzilla-request-daemon at macosforge.org]

Alexey Proskuryakov <ap at webkit.org> has asked  for review:
Bug 11610: XMLHttpRequest.onreadystatechange doesn't provide access to the
request object
http://bugs.webkit.org/show_bug.cgi?id=11610

Attachment 11865: proposed fix
http://bugs.webkit.org/attachment.cgi?id=11865&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
(In reply to comment #8)
> I don't like the fact that adding any new classes derived from EventTarget
will
> require changes to the toJS function in kjs_dom.cpp. Is there any way we can
do
> better?

  I have made it more symmetrical by adding a toXMLHttpRequest() method to
EventTarget. I don't see how it could automatically support new subclasses -
and other toJS functions (for Document, Node, etc.) have same or similar
design.

> Also, is there a way we can avoid adding code to kjs_dom.cpp -- where's the
> newfangled place for code like this?

  All other toJS() functions are also in kjs_* files now, except for a single
static one in JSCanvasRenderingContext2DCustom.cpp.

> Can someone rename intenalHeaderContent to internalHeaderContent?

  OK.

> As for all the functions in EventTarget that call isNode, why not use virtual

> functions for that?
> 
> If you don't want EventTarget's ref() and deref() themselves to be virtual
you
> can have them be inlines that call pure virtual functions. I think if you
> design EventTarget properly you won't have to have any code except for the
> JavaScript binding that uses isNode().

  I have actually removed isNode() in favor of directly calling
toNode()/toXMLHttpRequest().

  Implemented ref()/deref() as discussed on IRC, but actually defined them in
the header, even though they won't be inlined. I think that this will improve
readability, since the design is not particularly obvious.

> I haven't yet given up hope of sharing some of the event dispatch code, but
> that can wait.
> 
> Since toNode returns 0 if it's not a node, I think functions that call both
> isNode and toNode should perhaps be changed to just call toNode and check for

> nil.

  Done (removed isNode() altogether).

> +	   if (!element || (evt->target() &&
> element->contains(evt->target()->toNode())))
> 
> Why this new check for a nil target? When can that happen?

  The check used to be in HTMLElement::contains(); I don't know if this can
happen, but I wanted to preserve the semantics intact.

> +	   Event* evt = new Event(readystatechangeEvent, true, true);
> 
> This should be a RefPtr, and you should call release when calling
handleEvent.
> It's not good to rely on the fact that setTarget and setCurrentTarget won't
> ref/deref. This idiom occurs two more times a few lines later.

  Done (but with get() instead of release() - handleEvent() doesn't take
ownership AFAICT).

> +    using Shared<XMLHttpRequest>::ref;
> +    using Shared<XMLHttpRequest>::deref;
> 
> I believe you can just say using Shared::ref here.

  I think this shortcut can only be used for template parameters of the class
itself, not for its superclasses. I tried, but it didn't work.