Bug 35630 – Refactoring: XMLHttpRequest.open() should have all overloaded implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Currently XMLHTTPRequest::open() miss a overloaded method that W3C IDL has, and glue code emulate that overload. 
We should have XMLHTTPRequest::open(method, url, ec) to comprehend overloads 
to eliminate vague overload emulation from each glues (both JSC and V8).

[Hajime Morrita]

correct component.

[Hajime Morrita]

Created attachment 49880 [details]
patch v0

[WebKit Review Bot]

Attachment 49880 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/xml/XMLHttpRequest.cpp:297:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yaar Schnitman]

Looking good. You probably also want to include a fix to V8HttpXMLRequestCustom in this patch.

I think Dimitri Glazkov or Nate Chapin would want to review this.

[Hajime Morrita]

Created attachment 49883 [details]
patch v1; fix style violation

[Hajime Morrita]

(In reply to comment #4)
> Looking good. You probably also want to include a fix to V8HttpXMLRequestCustom
> in this patch.
Thank you for reviewing! I'll do it shortly.

[Hajime Morrita]

Created attachment 49886 [details]
v2; added v8 binding change

[Alexey Proskuryakov]

Comment on attachment 49886 [details]
v2; added v8 binding change

+        No new tests. No functional change.

This is not accurate. This patch changes the behavior of open("GET", url, undefined) - we used to treat the last argument as false, and now it's true. This may or may not be desired, depending on what the spec says, and what other browsers do. It also changes the behavior of open("GET", url, undefined, user, password), in a way that seems clearly incorrect.

Regardless of whether you decide to keep or drop this change, please add a regression test that would have caught it.

An existing test xmlhttprequest/basic-auth.html covers undefined and missing values for user/password, and the patch seems to be fine in this regard.

[Yaar Schnitman]

http://www.w3.org/TR/XMLHttpRequest/#the-open-method says that the default for async is true. However, when async is passed, anything other then JS true should be treated as false. You should omit the !args.at(2).isUndefined() checks to achieve that effect.

(In reply to comment #8)
> (From update of attachment 49886 [details])
> +        No new tests. No functional change.
> 
> This is not accurate. This patch changes the behavior of open("GET", url,
> undefined) - we used to treat the last argument as false, and now it's true.
> This may or may not be desired, depending on what the spec says, and what other
> browsers do. It also changes the behavior of open("GET", url, undefined, user,
> password), in a way that seems clearly incorrect.
> 
> Regardless of whether you decide to keep or drop this change, please add a
> regression test that would have caught it.
> 
> An existing test xmlhttprequest/basic-auth.html covers undefined and missing
> values for user/password, and the patch seems to be fine in this regard.

[Hajime Morrita]

Created attachment 49980 [details]
v3; follow feedback

[Hajime Morrita]

Alexey and Yarr, thank you for review and advice!
I'll 

(In reply to comment #9)
> > This is not accurate. This patch changes the behavior of open("GET", url,
> > undefined) - we used to treat the last argument as false, and now it's true.
I fixed that problem and added the test case for it.

[Alexey Proskuryakov]

Comment on attachment 49980 [details]
v3; follow feedback

Does this test fully pass in Firefox?

Expecting exactly two req.onreadystatechange events seems fragile. Perhaps you could just check readyState or status after calling send()?

Code changes look good to me.

[Hajime Morrita]

Created attachment 49992 [details]
v4; fix test to pass on ff

[Hajime Morrita]

> Expecting exactly two req.onreadystatechange events seems fragile. Perhaps you
> could just check readyState or status after calling send()?
True!
And I fixed it to pass also on FF.
Thanks.

[Alexey Proskuryakov]

Comment on attachment 49992 [details]
v4; fix test to pass on ff

> +PASS: if async argument is something defined, send() should behave like as async=true

This is not what the current code implements. The argument is converted to boolean, so e.g. null or "" become false.

Please change "if async argument is something defined, send() should behave like as async=true" to "if async argument is a non-empty string, send() should behave like as async=true", because that's what the subtest verifies. One can make this change when landing, or you could submit a new patch if you want to use commit-queue.

r=me

[Hajime Morrita]

Created attachment 50077 [details]
v5; rearrange test description

[Hajime Morrita]

Comment on attachment 50077 [details]
v5; rearrange test description

Thank you for reviewing again!
I fixed confusing test description, and set commit-queue:?

[WebKit Commit Bot]

Comment on attachment 50077 [details]
v5; rearrange test description

Clearing flags on attachment: 50077

Committed r55569: <http://trac.webkit.org/changeset/55569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.