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

[bugzilla-daemon at webkit.org]

Julien Chaffraix <julien.chaffraix at gmail.com> has asked  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 Julien Chaffraix <julien.chaffraix at gmail.com>
> +	   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.

Changed the name for "Add send() flag checks in XmlHttpRequest" which is more
explicit. I have also added a detailed ChangeLog

> +    m_state = Uninitialized;

That one should be kept as we do not update the readystate in abort (it is more
visible in the new patch)
> ...
> +    m_loader = 0;

Replaced by an ASSERT(!m_loader)

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

The m_state == Uninitialized assertion is a guard that was introduced to
replace a changeState(Uninitialized) that was moved in abort (see bug 13141).

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

Removed the FIXME

> 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 have merged the 2 methods and added a boolean parameter 'isJavascripAbort'
which triggers the abort behaviour as specified in the spec. The default value
does not change readystate (which was the behaviour before bug 13141).

>  void XMLHttpRequest::setRequestHeader(const String& name, const String&
value,
> ExceptionCode& ec)
>  {
> -    if (m_state != Open) {
> +    if (m_state != Open || m_loader) {
>
> to catch the new case, we would need to have called send() to set m_loader
but
> should not have finished send() because it would trigger m_state != Open. I
> will try to produce a test case but I do not know if it is possible to catch
> it.

I could not produce a test case for that part : send() is done completely
before continuing Javascript execution which trigger m_state != Open.