9.61 KB, patch
|Details | Diff | Splinter Review|
Our IDL does not default to null for these arguments. The specification does.
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
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?
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.
Created attachment 826774 [details] [diff] [review] XMLHttpRequest2.diff
Attachment #826313 - Attachment is obsolete: true
This time I checked it, and it compiles :)
Created attachment 8789627 [details] [diff] [review] 933759-match-the-spec-IDL-for-the-XHR-open-method.diff 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
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+
Created attachment 8790267 [details] [diff] [review] 933759-match-the-spec-IDL-for-the-XHR-open-method.diff 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
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 firstname.lastname@example.org: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b544237b263 Match the spec's IDL for the XHR open method. r=baku
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
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?
>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!
I've added an entry at: https://developer.mozilla.org/en-US/Firefox/Releases/51#Others
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.