[webkit-reviews] review denied: [Bug 16989] XmlHttpRequest should use the send() flag algorithm : [Attachment 18633] Patch using m_loader as ap pointed out

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Julien Chaffraix
<julien.chaffraix at gmail.com>'s request for review:
Bug 16989: XmlHttpRequest should use the send() flag algorithm
http://bugs.webkit.org/show_bug.cgi?id=16989

Attachment 18633: Patch using m_loader as ap pointed out
http://bugs.webkit.org/attachment.cgi?id=18633&action=edit

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
+	 bug 16989: XmlHttpRequest should use the send() flag algorithm

It probably makes sense to rename this bug to make it clear what was fixed, as
the send flag already worked. A detailed per-function ChangeLog would also help
tremendously, since these changes are relatively risky, and will be studied by
others many times if regressions come up.

+    m_state = Uninitialized;
...
+    m_loader = 0;

It seems that these changes result from a direct translation of the spec, and
are not needed. For example, open() already asserts that m_state ==
Uninitialized, because this is ensured by other methods. Similarly, I do not
see how m_loader can be non-null here.

-    if (m_state != Open) {
+    if (m_state != Open || m_loader) {

There is a check (with a FIXME) for m_loader just below this line; it should be
removed.

-void XMLHttpRequest::abort()
+void XMLHttpRequest::abortRequest()

It's not clear from the name what the difference between these functions is;
this will undoubtedly confuse someone sooner or later. Perhaps, it would be
more straightforward to add a boolean flag with a default value to
differentiate the two cases.

I'm not sure if I fully understood all the changes without a detailed
ChangeLog, but looks like there are still a few improvements  to be made to
this patch; r- for now.