Closed Bug 933759 Opened 11 years ago Closed 8 years ago

Consider implementing username/password argument defaulting of XMLHttpRequest.prototype.open

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: annevk, Assigned: wisniewskit)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

Our IDL does not default to null for these arguments. The specification does.
Attached patch XMLHttpRequestDefaultArgs.diff (obsolete) — Splinter Review
Hey,
Could you check please if my patch ok?
Riadh, how that's usually done is to ask a peer of the XMLHttpRequest code for a review.
Please read https://wiki.mozilla.org/Bugzilla:Review

Also, since you probably don't have check-in privileges,
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Is this your first patch for Mozilla?  We're glad to have your help!
Oops, I didn't realize that first link was a Bugzilla-specific review page. :(  Here's a better link:
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Attachment #826313 - Flags: review?(Ms2ger)
Hey Alex,
Thanks a lot for your links
It's not my first patch to Mozilla :)
You're welcom
Comment on attachment 826313 [details] [diff] [review]
XMLHttpRequestDefaultArgs.diff

Does this compile?
Attachment #826313 - Flags: review?(Ms2ger)
Hey
I did exactly like here:
mxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#71
So I think it compile.
Thanks
You think, or you compiled?  Because I agree with Ms2ger: there is no way that patch should be compiling.
I tried to compiled it but it doesn't work.
So you are right guys. Sorry for that.
I'm trying to solve the issue right now.
Attached patch XMLHttpRequest2.diff (obsolete) — Splinter Review
Attachment #826313 - Attachment is obsolete: true
This time I checked it, and it compiles :)
Here's an updated patch in case we want to fix this.

It's mostly changing Optional<nsString>&s into nsString&s, but I also ended up making OpenInternal public for the worker XHR code to be able to call it directly (there wasn't much of a reason to keep it protected).

Try seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14095b3f9930
Attachment #826774 - Attachment is obsolete: true
Attachment #8789627 - Flags: review?(amarchesini)
Comment on attachment 8789627 [details] [diff] [review]
933759-match-the-spec-IDL-for-the-XHR-open-method.diff

Review of attachment 8789627 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1398,5 @@
>  
>  // This case is hit when the async parameter is outright omitted, which
>  // should set it to true (and the username and password to null).
>  void
>  XMLHttpRequestMainThread::Open(const nsACString& aMethod, const nsAString& aUrl,

Do we use this anyhow? Probably we can remove it. If we can, file a follow up bug.

::: dom/xhr/XMLHttpRequestWorker.h
@@ +106,5 @@
> +    OpenInternal(aMethod, aUrl, aAsync, username, password, aRv);
> +  }
> +
> +  void
> +  OpenInternal(const nsACString& aMethod, const nsAString& aUrl,

call it Open as on the main-thread implementation.
Attachment #8789627 - Flags: review?(amarchesini) → review+
Here's a new patch addressing the review comments. Carrying over r+ and requesting check-in.
Assignee: nobody → wisniewskit
Attachment #8789627 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Keywords: checkin-needed
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Andrea Marchesini [:baku] from comment #13)
> ::: dom/xhr/XMLHttpRequestMainThread.cpp
> @@ +1398,5 @@
> >  
> >  // This case is hit when the async parameter is outright omitted, which
> >  // should set it to true (and the username and password to null).
> >  void
> >  XMLHttpRequestMainThread::Open(const nsACString& aMethod, const nsAString& aUrl,
> 
> Do we use this anyhow? Probably we can remove it. If we can, file a follow
> up bug.

Unfortunately we do need it, as it's there to match the corresponding WebIDL operation:

> [Throws] void open(ByteString method, DOMString url);
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b544237b263
Match the spec's IDL for the XHR open method. r=baku
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9b544237b263
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
So before we used to treat null, not passed, and "" the same way, right?

Now we just got rid of the "not passed" state but steal treat "" and null identically?

1)  Is there a followup bug to actually update to what the spec says to do with these arguments (null and "" are not supposed to be treated identically per spec)?

2)  Why do we still have the internal method taking Optional?  What purpose does that serve?
Flags: needinfo?(wisniewskit)
>null and "" are not supposed to be treated identically per spec

True, I'm surprised there were no web platform tests covering that case which tripped. I'll file a new bug to make sure we follow the spec there (and to add a web platform test).


>why do we still have the internal method taking Optional?

I suppose there is no reason anymore. I might as well remove it in the follow-up.


Thanks!
Flags: needinfo?(wisniewskit)
Blocks: 1302620
Blocks: 1302623
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.