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)
Core
DOM: Core & HTML
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)
9.61 KB,
patch
|
Details | Diff | Splinter Review |
Our IDL does not default to null for these arguments. The specification does.
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
Hey,
Could you check please if my patch ok?
Comment 3•11 years ago
|
||
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!
Comment 4•11 years ago
|
||
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
Updated•11 years ago
|
Attachment #826313 -
Flags: review?(Ms2ger)
Comment 5•11 years ago
|
||
Hey Alex,
Thanks a lot for your links
It's not my first patch to Mozilla :)
You're welcom
Comment 6•11 years ago
|
||
Comment on attachment 826313 [details] [diff] [review]
XMLHttpRequestDefaultArgs.diff
Does this compile?
Attachment #826313 -
Flags: review?(Ms2ger)
Comment 7•11 years ago
|
||
Hey
I did exactly like here:
mxr.mozilla.org/mozilla-central/source/dom/webidl/Document.webidl#71
So I think it compile.
Thanks
![]() |
||
Comment 8•11 years ago
|
||
You think, or you compiled? Because I agree with Ms2ger: there is no way that patch should be compiling.
Comment 9•11 years ago
|
||
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.
Comment 10•11 years ago
|
||
Attachment #826313 -
Attachment is obsolete: true
Comment 11•11 years ago
|
||
This time I checked it, and it compiles :)
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 15•8 years ago
|
||
(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);
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
bugherder landing |
Comment 18•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
![]() |
||
Comment 19•8 years ago
|
||
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)
Assignee | ||
Comment 20•8 years ago
|
||
>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)
Updated•8 years ago
|
Keywords: dev-doc-needed
Comment 21•8 years ago
|
||
I've added an entry at:
https://developer.mozilla.org/en-US/Firefox/Releases/51#Others
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•