[webkit-reviews] review denied: [Bug 16989] Add send() flag checks in XmlHttpRequest : [Attachment 18887] updated patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 16989: Add send() flag checks in XmlHttpRequest
http://bugs.webkit.org/show_bug.cgi?id=16989

Attachment 18887: updated patch
http://bugs.webkit.org/attachment.cgi?id=18887&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
This looks great to me. I have some suggestions on how to improve it.

Instead of adding the boolean parameter to abort, I suggest we create a private
function with a name like cancel() or kill() or stop() and use abort() only for
the true public XMLHttpRequest.abort. Then abort() can call the other function
first, then do the work you put inside your isJavascriptAbort if statement.

The language is JavaScript, not Javascript.

+    // Do not check for m_loader as required by the spec as it cannot be non
null

I don't understand this comment. Are you saying that we could assert m_loader
is not 0? If so, then please add that assert. Another thing I'd ask is: "How
can the spec require this?" Since m_loader is an internal variable the
specification should have nothing to say about it directly; if it says anything
it would be indirect and I'd like to know what it says. I think you need a
better comment, and I want to more clearly understand why we don't need the if
statement any more.

 void XMLHttpRequest::setRequestHeader(const String& name, const String& value,
ExceptionCode& ec)
 {
-    if (m_state != Open) {
+    if (m_state != Open || m_loader) {

You say that this cannot be covered by test cases. If not, then what is the
check for? Why is this change needed? Why can't we test it? We can easily
construct a load that won't complete with our test HTTP server, I believe.

I'm going to say review- for now, but this looks really good to me.