[webkit-reviews] review denied: [Bug 54162] XHR inconsistent in handling access to response attributes after an error : [Attachment 81933] Proposed patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Jarred Nicholls
<jarred.nicholls at gmail.com>'s request for review:
Bug 54162: XHR inconsistent in handling access to response attributes after an
error
https://bugs.webkit.org/show_bug.cgi?id=54162

Attachment 81933: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=81933&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
I'm almost convinced. However, a quick web search shows that people do care
about status and statusText after abort() for various reasons, see e.g.
<http://groups.google.com/group/jquery-dev/browse_thread/thread/3d8f7ac78c9b011
7>. Reuse of XHR object seems to be a common theme in posts I found.

Also, when we changed abort() to reset status a few years ago, someone quickly
filed a bug. One of the tests you modified (status-after-abort.html) was there
specifically to make sure that we don't break this again! Please use svn log to
check history of tests like this.

I guess that I should also mention Dashboard widgets, which are only tested
with WebKit. But there isn't much we can do about those now, except for adding
quirks if problems are found.

-	 return jsUndefined();
+	 return jsNull();

These unrelated changes should be in a separate patch.

-    return String();
+    // Return empty string, not a null String()
+    return "";

This, too. Might be best to do smaller changes like that first.

> status and statusText will never throw an exception, so they are not tested
for an expected exception.

Please modify XMLHttpRequest.idl to not require exception handling for methods
that can never raise an exception.

I'm not sure why you check m_error in functions like returnXML and returnBlob.
Aren't these supposed to be empty already? It's good to match algorithms from
spec for clarity, but not when that's confusingly redundant with other code
that we have. An ASSERT would be the best way to document that we respect the
spec here.

r- for the necessary IDL modification. I've e-mailed the person who filed a bug
about status after abort(), so I may have more information later.