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

[bugzilla-request-daemon at macosforge.org]

Darin Adler <darin at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 11610: XMLHttpRequest.onreadystatechange doesn't provide access to the
request object
http://bugs.webkit.org/show_bug.cgi?id=11610

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

------- Additional Comments from Darin Adler <darin at apple.com>
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?

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

Can someone rename intenalHeaderContent to internalHeaderContent?

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

+	 if (!element || (evt->target() &&
element->contains(evt->target()->toNode())))

Why this new check for a nil target? When can that happen?

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

+    using Shared<XMLHttpRequest>::ref;
+    using Shared<XMLHttpRequest>::deref;

I believe you can just say using Shared::ref here.

I'm going to say review+ here, but feel free to clear the review flag if you
want a review of a new version of the patch.