Bug 35630 - Refactoring: XMLHttpRequest.open() should have all overloaded implementations
Summary: Refactoring: XMLHttpRequest.open() should have all overloaded implementations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-02 22:36 PST by Hajime Morrita
Modified: 2019-02-06 09:03 PST (History)
5 users (show)

See Also:


Attachments
patch v0 (3.70 KB, patch)
2010-03-02 22:48 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
patch v1; fix style violation (3.71 KB, patch)
2010-03-02 23:15 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v2; added v8 binding change (5.47 KB, patch)
2010-03-03 01:26 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v3; follow feedback (8.92 KB, patch)
2010-03-03 21:11 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v4; fix test to pass on ff (8.83 KB, patch)
2010-03-03 23:55 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff
v5; rearrange test description (8.83 KB, patch)
2010-03-04 18:33 PST, Hajime Morrita
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hajime Morrita 2010-03-02 22:36:27 PST
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).
Comment 1 Hajime Morrita 2010-03-02 22:39:14 PST
correct component.
Comment 2 Hajime Morrita 2010-03-02 22:48:43 PST
Created attachment 49880 [details]
patch v0
Comment 3 WebKit Review Bot 2010-03-02 22:52:54 PST
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.
Comment 4 Yaar Schnitman 2010-03-02 23:09:50 PST
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.
Comment 5 Hajime Morrita 2010-03-02 23:15:45 PST
Created attachment 49883 [details]
patch v1; fix style violation
Comment 6 Hajime Morrita 2010-03-02 23:16:48 PST
(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.
Comment 7 Hajime Morrita 2010-03-03 01:26:00 PST
Created attachment 49886 [details]
v2; added v8 binding change
Comment 8 Alexey Proskuryakov 2010-03-03 16:45:45 PST
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.
Comment 9 Yaar Schnitman 2010-03-03 17:31:25 PST
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.
Comment 10 Hajime Morrita 2010-03-03 21:11:05 PST
Created attachment 49980 [details]
v3; follow feedback
Comment 11 Hajime Morrita 2010-03-03 21:13:37 PST
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.
Comment 12 Alexey Proskuryakov 2010-03-03 22:28:09 PST
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.
Comment 13 Hajime Morrita 2010-03-03 23:55:15 PST
Created attachment 49992 [details]
v4; fix test to pass on ff
Comment 14 Hajime Morrita 2010-03-03 23:56:54 PST
> 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.
Comment 15 Alexey Proskuryakov 2010-03-04 10:39:36 PST
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
Comment 16 Hajime Morrita 2010-03-04 18:33:49 PST
Created attachment 50077 [details]
v5; rearrange test description
Comment 17 Hajime Morrita 2010-03-04 18:37:29 PST
Comment on attachment 50077 [details]
v5; rearrange test description

Thank you for reviewing again!
I fixed confusing test description, and set commit-queue:?
Comment 18 WebKit Commit Bot 2010-03-04 21:35:44 PST
Comment on attachment 50077 [details]
v5; rearrange test description

Clearing flags on attachment: 50077

Committed r55569: <http://trac.webkit.org/changeset/55569>
Comment 19 WebKit Commit Bot 2010-03-04 21:35:49 PST
All reviewed patches have been landed.  Closing bug.
Comment 20 Lucas Forschler 2019-02-06 09:03:04 PST
Mass moving XML DOM bugs to the "DOM" Component.