Closed Bug 1285036 Opened 3 years ago Closed 3 years ago

Refactor some of the code in XMLHttpRequest

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: wisniewskit, Assigned: wisniewskit, Mentored)

References

Details

Attachments

(11 files, 40 obsolete files)

14.37 KB, patch
Details | Diff | Splinter Review
46.09 KB, patch
Details | Diff | Splinter Review
8.30 KB, patch
Details | Diff | Splinter Review
15.62 KB, patch
Details | Diff | Splinter Review
3.32 KB, patch
Details | Diff | Splinter Review
5.79 KB, patch
Details | Diff | Splinter Review
18.94 KB, patch
Details | Diff | Splinter Review
24.76 KB, patch
Details | Diff | Splinter Review
26.63 KB, patch
Details | Diff | Splinter Review
18.90 KB, patch
Details | Diff | Splinter Review
24.36 KB, patch
Details | Diff | Splinter Review
XMLHttpRequestMainThread could use a refactor, to not only make it cleaner and match the XHR2's wording more closely, but also get it into a shape where it can used as the only XHR implementation that's shared by workers and the main thread.

This bug will patch it step by side to bring it toward those goals, keeping each individual patch as simple as possible and self-contained (so tests don't break). Of course I'll also fix things that make more sense to fix in a given patch, rather than keeping existing buggy behavior.
1 - Use an enum for XHR ProgressEvent names and use consts for a few other event strings.

Note that this is the same patch awaiting review in bug 918703, but it's a refactoring patch rather than one specific to that bug.
Assignee: nobody → wisniewskit
Attachment #8768570 - Flags: review?(amarchesini)
2 - Split XMLHttpRequest.mState up into mState (an enum) and a series of mFlags (bools).
Attachment #8768572 - Flags: review?(amarchesini)
3 - Invert mFlagAsynchronous as mFlagSynchronous, to match the spec's wording.
Attachment #8768573 - Flags: review?(amarchesini)
4 - Remove the "sent" pseudo-state and just add support for the spec's send() flag instead.
Attachment #8768574 - Flags: review?(amarchesini)
5 - Change CreateReadyStateChange() to FireReadyStateChange(), since it's not an event type that varies, and this is closer to the spec's wording.
Attachment #8768575 - Flags: review?(amarchesini)
6 - Add CString versions of ASCIIToLower/Upper, as ASCIIToLower will be needed soon, and I might as well add CString variants for both functions while I'm here.
Attachment #8768576 - Flags: review?(amarchesini)
7 - Make SetRequestHeader() and related header code follow the spec much more closely. This means not actually setting the headers on the channel/request until the Send() method is called, but thankfully that only breaks one mochitest that doesn't require or test that non-standard behavior. In return, the code is more straightforward and a bunch of web platform tests now pass.
Attachment #8768577 - Flags: review?(amarchesini)
Attached patch 1285036-8-refactor_Open.diff (obsolete) — Splinter Review
8 - Make Open() follow the spec, passing more WPTs. I factor the code to initialize the channel out into its own method. I also have the various API endpoints for Open() all call the same internal _Open() method to keep things as consistent as possible.
Attachment #8768578 - Flags: review?(amarchesini)
9 - Refactor the Send() API endpoints, including the RequestBody class and GetRequestBody methods. Using template classes as in the patch should be cleaner and easier to manage without bloating the code. The Send(nsIVariant) endpoint also unwraps its argument right away and delegates it to the appropriate RequestBody type, simplifying that code a bit.
Attachment #8768582 - Flags: review?(amarchesini)
Apologies for the review flood, :baku!

Successful try run for the first 9 patches (just two unrelated intermittents): https://treeherder.mozilla.org/#/jobs?repo=try&revision=66e3b44d3136

I'll continue chipping away at this...
Comment on attachment 8768570 [details] [diff] [review]
1285036 - part 1 - enum-and-constify-event-strings.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +103,5 @@
> +    NS_LITERAL_STRING("abort"),
> +    NS_LITERAL_STRING("timeout"),
> +    NS_LITERAL_STRING("load"),
> +    NS_LITERAL_STRING("loadend")
> +  };

Add static assertion about having ProgressEventTypeStrings and ProgressEventType always in sync.
Attachment #8768570 - Flags: review?(amarchesini) → review+
Comment on attachment 8768572 [details] [diff] [review]
1285036-2-use_an_enum_for_state_and_bools_for_flags.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1129,5 @@
>  
>    // If the state is UNSENT or OPENED,
>    // return the empty string and terminate these steps.
> +  if (mState == State::unsent ||
> +      mState == State::opened || mState == State::sent) {

Indent this better.

@@ +1194,5 @@
>    if (!httpChannel) {
>      // If the state is UNSENT or OPENED,
>      // return null and terminate these steps.
> +    if (mState == State::unsent ||
> +        mState == State::opened || mState == State::sent) {

here too.

@@ +3096,2 @@
>  {
> +  bool droppingFromSENTtoOPENED = mState == State::sent &&

Yeah.. I know. I have to finish the other review.

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +698,1 @@
>  

I love this approach. Thanks for doing it. What I would like to see here is comments!
Can you write some documentation for each on of these flags?
Attachment #8768572 - Flags: review?(amarchesini) → review+
Attachment #8768573 - Flags: review?(amarchesini) → review+
Comment on attachment 8768574 [details] [diff] [review]
1285036-4-remove-sent-pseudostate.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3001,5 @@
> +      return LOADING;
> +    case State::done:
> +      return DONE;
> +    default:
> +      NS_ASSERTION(false, "Unknown State!");

MOZ_CRASH("Unknown state");

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +693,5 @@
>    bool mFlagHadUploadListenersOnSend;
>    bool mFlagACwithCredentials;
>    bool mFlagTimedOut;
>    bool mFlagDeleted;
> +  bool mFlagSend;

Document this as well.
Attachment #8768574 - Flags: review?(amarchesini) → review+
Attachment #8768575 - Flags: review?(amarchesini) → review+
Attachment #8768576 - Flags: review?(amarchesini) → review+
Comment on attachment 8768577 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

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

This is patch 6.
Attachment #8768577 - Flags: review?(amarchesini)
Added static assertion. Carrying over r=baku.
Attachment #8768570 - Attachment is obsolete: true
Review comments addressed. Carrying over r+.

I added basic comments in the header for the states, as well as a link to the relevant part of the spec.

I'm definitely planning on documenting the various flags once I figure out which ones need to remain and why. I'm hoping to remove as many as possible, and make sure our non-spec behavior is as well-documented and factored out as I can get it.
Attachment #8768572 - Attachment is obsolete: true
Re-attaching rebased version with correct filename (part 3, not 2). Carrying over r+.
Attachment #8768573 - Attachment is obsolete: true
Review comments addressed. Carrying over r+.
Attachment #8768574 - Attachment is obsolete: true
Actually carry over the r+ in the checkin comment this time...
Attachment #8768829 - Attachment is obsolete: true
Might as well rebase this while I'm here. Carrying over r+.
Attachment #8768575 - Attachment is obsolete: true
>This is patch 6.

Huh. Not sure how that happened. Here's the real patch 7.

By the by, would it be alright to request check-in on (at least some of) the patches you've already reviewed, or should I hold off on doing that for now?
Attachment #8768577 - Attachment is obsolete: true
Attachment #8768835 - Flags: review?(amarchesini)
Sure, just add the keyword "leave-open".
Alright, thanks.

I'm requesting checkin for the reviewed patches, 1 to 6.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e94b77f805b
Part 1: Use an enum for ProgressEvent types and use consts for the event name strings. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/3036026722de
Part 2: Split mState up into an enum and a set of bool flags. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/b93efbaf1d7a
Part 3: Flip the asynchronous flag to synchronous, to match the spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/0390a7afb12d
Part 4: Remove the 'sent' pseudostate and add support for the spec's send() flag instead. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae182434a6a
Part 5: Consolidate the readystatechange-firing code into a function named FireReadyStateChange(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7174077bd92
Part 6: Add CString variants of ASCIIToLower/Upper() for the next patch. r=baku
Keywords: checkin-needed
Backed out for debug bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=31402431&repo=mozilla-inbound#L635

If you could, please also revise the commit messages similar to how the patches were landed (i.e. including the bug #, formatting cleanups, etc).
Backout by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f5af40eb365
Backed out 6 changesets for bustage on a CLOSED TREE.
Indeed, I missed a debug assert that I shouldn't have missed: a good reminder for me to keep the debug mode flag turned on while I'm developing.

Removing that out-dated assert is all I'm changing in the related patch, so I don't suspect re-review is necessary. I'll also revise the commit messages... thanks for the heads-up about formatting!
Attachment #8768804 - Attachment is obsolete: true
Attachment #8768820 - Attachment is obsolete: true
Attachment #8768831 - Attachment is obsolete: true
Attachment #8768833 - Attachment is obsolete: true
Attachment #8768576 - Attachment is obsolete: true
Alright, I've rebased the first 6 patches (with the aforementioned fix for debug breakage and the commit message changes). They should be good to go.
Keywords: checkin-needed
Two more patches incoming. A try run for all the patches including both of these was successful (though with both of these new patches combined into one, before I split them up): https://treeherder.mozilla.org/#/jobs?repo=try&revision=afda7a56d9f5

This first patch mostly just pulls all the non-XHR-specific request header code into its own class named RequestHeader, to increase readability.

It also cleans up the chunk of logic in the Send() method related to picking the correct content-type/encoding for the request. It now matches spec step 4 more clearly.
Attachment #8769528 - Flags: review?(amarchesini)
This patch factors all the Gecko specific channel-setup gunk out of the Send() method, so that it's easier to tell what it's really doing when it comes time to make it match the spec.

Note that I split the channel-setup code out into XHR-specific and non-XHR-specific functions. I suspect this will make it easier to unify it with the Fetch API's code, since the two APIs share the same "fetch request" logic in the spec.
Attachment #8769529 - Flags: review?(amarchesini)
Attached patch 1285036-8-refactor_Open.diff (obsolete) — Splinter Review
Also here's a new patch #8 which fixes a couple of debug assertions in the worker XHR code that don't expect a couple of changes related to spec-alignment.
Attachment #8768578 - Attachment is obsolete: true
Attachment #8768578 - Flags: review?(amarchesini)
Attachment #8769548 - Flags: review?(amarchesini)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e42827ec6521
Part 1: Use an enum for ProgressEvent types and use consts for the event name strings. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7274bdcd99
Part 2: Split mState up into an enum and a set of bool flags. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf25c8b6af9f
Part 3: Flip the asynchronous flag to synchronous, to match the spec. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/18a736ed2156
Part 4: Remove the 'sent' pseudostate and add support for the spec's send() flag instead. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fc0549bed0
Part 5: Consolidate the readystatechange-firing code into a function named FireReadyStateChange(). r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cd5236715f
Part 6: Add CString variants of ASCIIToLower/Upper() for the next patch. r=baku
Keywords: checkin-needed
Rebased patch 7.
Attachment #8768835 - Attachment is obsolete: true
Attachment #8768835 - Flags: review?(amarchesini)
Attachment #8771700 - Flags: review?(amarchesini)
Rebased patch 9.
Attachment #8768582 - Attachment is obsolete: true
Attachment #8768582 - Flags: review?(amarchesini)
Attachment #8771713 - Flags: review?(amarchesini)
Rebased patch 10.
Attachment #8769528 - Attachment is obsolete: true
Attachment #8769528 - Flags: review?(amarchesini)
Attachment #8771714 - Flags: review?(amarchesini)
Rebased patch 11.
Attachment #8769529 - Attachment is obsolete: true
Attachment #8769529 - Flags: review?(amarchesini)
Attachment #8771715 - Flags: review?(amarchesini)
A fresh try-run for rebased patches 7-11 seems ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b0be446311a
Comment on attachment 8771700 [details] [diff] [review]
1285036-7-refactor_SetRequestHeader.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3159,5 @@
> +XMLHttpRequestMainThread::GetAuthorRequestHeaderValue(const char* aName,
> +                                                      nsACString& outValue)
> +{
> +  for (RequestHeader& header : mAuthorRequestHeaders) {
> +    if (header.name.Equals(aName)) {

EqualsLiteral
Attachment #8771700 - Flags: review?(amarchesini) → review+
Comment on attachment 8769548 [details] [diff] [review]
1285036-8-refactor_Open.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +1366,2 @@
>    }
> +  return _Open(aMethod, aUrl, async, username, password);

call it OpenInternal

@@ +1455,5 @@
> +    parsedURL->GetHost(host);
> +    if (!host.IsEmpty()) {
> +      nsAutoCString userpass;
> +      if (aUsername.WasPassed()) {
> +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec

Are you going to do something here as well?

@@ +1459,5 @@
> +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec
> +      }
> +      userpass.AppendLiteral(":");
> +      if (aPassword.WasPassed()) {
> +        AppendUTF16toUTF8(aPassword.Value(), userpass); // todo: percent-escape as per spec

same here.

@@ +1483,1 @@
>    mFlagSend = false;

Why this is here and not it's part of step 11 ?

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +588,5 @@
>    void StartProgressEventTimer();
>  
>    nsresult OnRedirectVerifyCallback(nsresult result);
>  
> +  nsresult _Open(const nsACString& aMethod, const nsACString& aUrl,

OpenInternal
Attachment #8769548 - Flags: review?(amarchesini) → review+
Comment on attachment 8771713 [details] [diff] [review]
1285036-9-refactor_Send_endpoints_and_RequestBody.diff

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

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2140,5 @@
> +
> +    // Make sure to use the encoding we'll send
> +    rv = serializer->SerializeToStream(domdoc, output, aCharset);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +

extra line

@@ +2150,5 @@
> +  rv = storStream->GetLength(&length);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  *aContentLength = length;
> +
> +  return storStream->NewInputStream(0, aResult);

rv = storStream...
NS_ERROR_FAILURE(rv, rv);

return NS_OK;

@@ +2163,5 @@
> +  aCharset.AssignLiteral("UTF-8");
> +
> +  nsCString converted = NS_ConvertUTF16toUTF8(*mBody);
> +  *aContentLength = converted.Length();
> +  return NS_NewCStringInputStream(aResult, converted);

same here

@@ +2176,5 @@
> +  aCharset.Truncate();
> +
> +  nsresult rv = mBody->Available(aContentLength);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +

nsCOMPtr<nsIInputStream> stream(mBody);
stream.forget(aResult);

return NS_OK;

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +243,5 @@
>    public:
> +    virtual nsresult GetAsStream(nsIInputStream** aResult,
> +                                 uint64_t* aContentLength,
> +                                 nsACString& aContentType,
> +                                 nsACString& aCharset) const

virtual nsresult GetAsStream(nsIInputStream** aResult,
                                 uint64_t* aContentLength,
                                 nsACString& aContentType,
                                 nsACString& aCharset) const = 0; ?

@@ +251,5 @@
> +    }
> +  };
> +
> +  template<typename Type>
> +  class RequestBody : public RequestBodyBase

final

@@ +266,3 @@
>    };
>  
> +  nsresult _Send(const RequestBodyBase* aBody);

SendInternal
Attachment #8771713 - Flags: review?(amarchesini) → review+
> EqualsLiteral

I tried that already, but it doesn't compile with EqualsLiteral (GC 5.4.0):

>if (header.name.EqualsLiteral(aName)) {
>error: mismatched types ‘const char [N]’ and ‘const char*’



> > +        CopyUTF16toUTF8(aUsername.Value(), userpass); // todo: percent-escape as per spec
> Are you going to do something here as well?

I'll remove these comments and file a follow-up bug, since this bug doesn't have to fix everything.



> >    mFlagSend = false;
> Why this is here and not it's part of step 11 ?

No real reason. I'll move it down a few lines.



I'll also make the other changes you suggest.
Incoming updated versions of patches 7-9, addressing the review comments and carrying over r+.

A try run with all 3 still seems clear: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1410cfdc028d
Attachment #8771700 - Attachment is obsolete: true
Attached patch 1285036-8-refactor_Open.diff (obsolete) — Splinter Review
Attachment #8769548 - Attachment is obsolete: true
Attachment #8771713 - Attachment is obsolete: true
Requesting checkin of patches 7, 8 and 9.
Keywords: checkin-needed
part 8 has problems like 2 out of 2 hunks FAILED -- saving rejects to file dom/xhr/XMLHttpRequestWorker.cpp.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-open-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-open-send.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-open-sync-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-open-sync-send.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-send-open.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-send-open.htm.ini.rej
unable to find 'testing/web-platform/meta/XMLHttpRequest/open-sync-open-send.htm.ini' for patching
1 out of 1 hunks FAILED -- saving rejects to file testing/web-platform/meta/XMLHttpRequest/open-sync-open-send.htm.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1285036-8-refactor_Open.diff
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
My bad, I wasn't paying enough attention to whether my checkin-requests would clash. I'll slow it down.
Flags: needinfo?(wisniewskit)
Rebased patches incoming, carrying over r+ for 7-9.
Attachment #8772254 - Attachment is obsolete: true
Attachment #8772255 - Attachment is obsolete: true
Attachment #8772256 - Attachment is obsolete: true
Attachment #8771714 - Attachment is obsolete: true
Attachment #8771714 - Flags: review?(amarchesini)
Attachment #8772983 - Flags: review?(amarchesini)
Attachment #8771715 - Attachment is obsolete: true
Attachment #8771715 - Flags: review?(amarchesini)
Attachment #8772986 - Flags: review?(amarchesini)
A fresh try run for the rebased patches still seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=62475446d2ff

Re-requesting checkin for patches 7, 8 and 9.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16fefebdbb50
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/abbef296a82f
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a00db57d77a
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku
Keywords: checkin-needed
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5695eb78dcdc
Backed out changeset 8a00db57d77a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1f9e16f3e5a
Backed out changeset abbef296a82f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/316baefffba9
Backed out changeset 16fefebdbb50 for static build bustage
Ugh. I'll bet it's my old friend "explicit" to the rescue once again. Doing a try-run now, hope I picked the right machine on the TryChooser Syntax Builder...

Thanks Tomcat!
Flags: needinfo?(wisniewskit)
Patch 9, now with more explicit. Carrying over r+.
Attachment #8772982 - Attachment is obsolete: true
Patch 10, now with more explicit. Carrying over r+.
Attachment #8772983 - Attachment is obsolete: true
Attachment #8772983 - Flags: review?(amarchesini)
Attachment #8773302 - Flags: review?(amarchesini)
Requesting checkin once again for patches 7, 8, 9. Hopefully they'll stick this time.

Try showing the static-check success: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a3c4c26de40
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b51b69f723e
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6462cd2ea249
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fd18c03c696
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku
Keywords: checkin-needed
Thomas, is there a chance that this work would fix bug 827243?
Flags: needinfo?(wisniewskit)
Thanks Ryan, I'll investigate the failures.

@gandalf:
>is there a chance that this work would fix bug 827243?

It seems doubtful, based on bz's comments in that bug. I'm trying to limit this refactor just to the XHR (and possibly fetch) code, while leaving necko's code alone if possible.

I don't mind investigating that bug when I have some time, though.
Flags: needinfo?(wisniewskit)
Alright, here's a new patch 7 which fixes the issue with browser_net_resend.js. A try run shows those bugs are gone, and only things that seem truly unrelated are showing up: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0abe754cf0e8

The changes I had to make were again nothing I'd expect needs a review; I just had to compensate for the tests expecting header names from XHRs to be a specific case, when they're really not case-sensitive fields.

While here, I also modified the curl test so that it doesn't expect the shell command it gets to have its parameters in a particular order, since their order doesn't matter (and my local system gets them in a slightly different order than the try servers do, for whatever reason).

At any rate, I'm once again requesting check-in of patches 7, 8 and 9, since things seem clear.
Attachment #8772979 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/41a51d368f38
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/de078c2e0991
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6d55b02d19
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku
Keywords: checkin-needed
Wow is that test ever fragile. I'll have to run it through every machine on try before I try to commit it again, just in case.

Thanks for the backout!
Flags: needinfo?(wisniewskit)
Comment on attachment 8772986 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff

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

r- mainly for the use of nsCOMPtr. Plus, what about lower/upper case comparison of methods?
I'll be in PTO for 2 weeks. In the meantime, maybe you could send review requests to smaug. Ask him first. He is often very busy!
Thanks for the awesome work!

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2274,2 @@
>  nsresult
> +CreateChannelForFetch(nsCOMPtr<nsIChannel>& aChannel,

nsIChannel** aChannel

@@ +2274,5 @@
>  nsresult
> +CreateChannelForFetch(nsCOMPtr<nsIChannel>& aChannel,
> +                      const nsCOMPtr<nsILoadGroup>& aLoadGroup,
> +                      const nsCOMPtr<nsIPrincipal>& aPrincipal,
> +                      const nsCOMPtr<nsIDocument>& aResponsibleDocument,

just pass the pointers.

@@ +2312,4 @@
>    nsresult rv;
> +  if (aResponsibleDocument &&
> +      aResponsibleDocument->NodePrincipal() == aPrincipal) {
> +    rv = NS_NewChannel(getter_AddRefs(aChannel),

rv = NS_NewChannel(aChannel, ...

@@ +2321,5 @@
>                         nullptr,   // aCallbacks
>                         loadFlags);
>    } else {
>      // Otherwise use the principal.
> +    rv = NS_NewChannel(getter_AddRefs(aChannel),

rv = NS_NewChannel(aChannel, ...

@@ +2332,5 @@
>                         loadFlags);
>    }
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(aChannel));

nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(*aChannel));

@@ +2373,5 @@
>  }
>  
> +// Non-XMLHttpRequest-specific channel setup goes here.
> +nsresult
> +SetupChannelForFetch(const nsCOMPtr<nsIChannel>& aChannel,

Ditto.

@@ +2721,2 @@
>    if (aBody && httpChannel &&
> +      !mRequestMethod.EqualsLiteral("GET") &&

do we convert this in upper case somewhere else?

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +234,5 @@
>    // states
>    virtual uint16_t ReadyState() const override;
>  
>    // request
> +  nsresult Fetch(nsCOMPtr<nsIInputStream>& aUploadStream, int64_t aUploadLength);

nsIInputStream** aUploadStream

@@ +238,5 @@
> +  nsresult Fetch(nsCOMPtr<nsIInputStream>& aUploadStream, int64_t aUploadLength);
> +
> +  nsresult CreateChannelForFetch();
> +
> +  nsresult SetupChannelForFetch(const nsCOMPtr<nsIChannel>& aChannel,

nsIChannel* aChannel, nsPIDOMWindowInner* aOwner, nsIPrincipal* aPrincipal, .... nsIInputStream** aUploadStream
Attachment #8772986 - Flags: review?(amarchesini) → review-
Comment on attachment 8773302 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

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

Smaug, can you take this?
Attachment #8773302 - Flags: review?(amarchesini) → review?(bugs)
Comment on attachment 8773302 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff


>+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
>@@ -2458,19 +2458,16 @@ XMLHttpRequestMainThread::SendInternal(c
>   //     an asynchronous call.
> 
>   // Ignore argument if method is GET, there is no point in trying to
>   // upload anything
>   nsAutoCString method;
>   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
> 
>   if (httpChannel) {
>-    // Spec step 5
>-    SetAuthorRequestHeadersOnChannel(httpChannel);
I don't understand this change. Please explain.

>-    nsAutoCString charset;
>-    nsAutoCString defaultContentType;
>+    nsAutoCString contentType;
>+    contentType.SetIsVoid(true);
>+
>+    nsAutoCString encoding;
>+    encoding.SetIsVoid(true);
>+
Why the SetIsVoid calls?
You pass contentType and encoding to GetAsStream anyhow, so it should set the values to something reasonable when
success value is returned, I think. Though, I don't know what GetAsStream does. It is not in the tree right now. Does some patch here add it?


>+public:
>+  class CharsetIterator {
Nit, { goes to its own line

This is a bit complicated (largely because I haven't reviewed other related patches ,I think). I need to re-read.

But please fix those and explain and ask review again.
Attachment #8773302 - Flags: review?(bugs) → review-
Thanks for the reviews, guys, I'll address the comments for those patches as soon as I can.

For now, I'm at least ready to retry landing the already-reviewed patches. New versions of 7 and 9 are incoming (8 was still fine). Carrying over r+, as there are no functional changes, just updates to the aforementioned test to make it less fragile.

A try run hasn't yet revealed anything worrying (though I can't seem to get try to run WinXP tests, it stalls forever... even the Win8 debug ones spend many hours in a queue): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8f330e1fbe9
Attachment #8775421 - Attachment is obsolete: true
And here's the new patch 9.

Requesting check-in again for patches 7, 8 and 9.
Attachment #8773301 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/15171e9168c4
Part 7: Change SetRequestHeader() and related header code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd7ab32846c2
Part 8: Change XHR open() and related code to follow the spec more closely. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a87074952ee
Part 9: Clean up the XHR send() API endpoints, and how nsIVariants are handled. r=baku
Keywords: checkin-needed
Duplicate of this bug: 918763
(In reply to Olli Pettay [:smaug] from comment #78)

Here's a rebased version of the patch.


> >   if (httpChannel) {
> >-    // Spec step 5
> >-    SetAuthorRequestHeadersOnChannel(httpChannel);
> I don't understand this change. Please explain.

I essentially just moved this line down to where the rest of spec step 5 (the actual request-fetch) is done. Sorry that wasn't clearer.


> Why the SetIsVoid calls?
> You pass contentType and encoding to GetAsStream anyhow, so it should set
> the values to something reasonable when
> success value is returned, I think.

I figured it would be safer to just have the GetAsStream methods assume they only have to set the values if they need to, leaving them null otherwise. This also matches the spec's current logic/wording (for send step 4), but I certainly don't mind changing it.


>Though, I don't know what GetAsStream does. It is not in
> the tree right now. Does some patch here add it?

Yes, this was from the previous patch in the set (#9), which has since landed. Essentially, I factored out the logic to get the request body stream into a series of template classes, RequestBody<type>, each with a GetAsStream method.


> >+public:
> >+  class CharsetIterator {
> Nit, { goes to its own line

Done.


> This is a bit complicated (largely because I haven't reviewed other related
> patches ,I think). I need to re-read.

Sure, no rush. If you'd like I can lurk on an IRC channel of your choice in case you'd like more direct feedback.
Attachment #8773302 - Attachment is obsolete: true
Attachment #8779054 - Flags: review?(bugs)
Actually sorry, the patch you were looking at was this one I'm updating with this comment (I've updated both 10 and 11).

The patch in my previous comment addresses :baku's concerns in comment 76. In answer to his question about case-sensitivity, it shouldn't be an issue because the request method has already been case-normalized in the open() method as per spec (in a previous patch that's already landed).
Attachment #8772986 - Attachment is obsolete: true
Attachment #8779055 - Flags: review?(bugs)
Oh, wait... you actually *were* reviewing patch 10, baku had just skipped to 11. My bad, sorry for any undue confusion.
Blocks: 1269102
Hello Thomas, I'm working on devtools bug 1269102 and bug 1270096 and I have a few questions about the changes you did in the "part 7" patch:

1. When I do xhr.setRequestHeader("X-Custom", "value"), the header name is converted to lowercase "x-custom" and send as such. Why this change? Is it because of some recent change in the spec, namely [1]? I don't see any other browsers implementing this (Chrome, Safari), so I'm a bit confused.

2. Why the EXPECTED_RESULT.toString() call at [2]? EXPECTED_RESULT is already a string.

3. By the way, the set-matching algorithm in browser_net_copy_as_curl.js doesn't check the "--compressed" parameter (added when Accept-Encoding header is encountered). I'll fix that as part of bug 1269102.

[1] https://fetch.spec.whatwg.org/#concept-header-list-combine
[2] https://dxr.mozilla.org/mozilla-central/source/devtools/client/netmonitor/test/browser_net_copy_as_curl.js#67
Flags: needinfo?(wisniewskit)
1. Yes, the change was made to match the spec (and HTTP header names aren't supposed to be case-sensitive to begin with). But if this is causing compatibility issues, then I can change it back. It would be best for us to raise any issues with the spec authors, though.

2. Good point. I just forgot to remove it. If it works without the toString, then feel free to remove it while you update the code.

3. Thanks!
Flags: needinfo?(wisniewskit)
(In reply to Thomas Wisniewski from comment #88)
> 1. Yes, the change was made to match the spec (and HTTP header names aren't
> supposed to be case-sensitive to begin with). But if this is causing
> compatibility issues, then I can change it back. It would be best for us to
> raise any issues with the spec authors, though.
If others aren't doing that, I think we should revert the change, and get the spec fixed.
And if the patch doing that is already in branches, better to make sure it is backed out also there.
So, file a separate bug to deal all that.


Did some of the patches land before the previous merge? If some patches in a bug land for version X and some others X+1, that makes the regression hunting and tracking way more difficult. 
So better to split stuff to separate bugs in the future.
Not sure what to do with this bug though.
(In reply to Thomas Wisniewski from comment #84)
> > Why the SetIsVoid calls?
> > You pass contentType and encoding to GetAsStream anyhow, so it should set
> > the values to something reasonable when
> > success value is returned, I think.
> 
> I figured it would be safer to just have the GetAsStream methods assume they
> only have to set the values if they need to, leaving them null otherwise.
> This also matches the spec's current logic/wording (for send step 4), but I
> certainly don't mind changing it.
So is this changing the behavior? refactoring shouldn't change behavior, such changes should happen in separate bugs.

> Sure, no rush. If you'd like I can lurk on an IRC channel of your choice in
> case you'd like more direct feedback.
I'm in #content and #developers at least
Comment on attachment 8779054 [details] [diff] [review]
1285036-10-refactor_request_header_handling.diff

>     // If the user hasn't overridden the Accept header, set it to */* as per spec
>-    nsAutoCString acceptHeader;
>-    GetAuthorRequestHeaderValue("accept", acceptHeader);
>-    if (acceptHeader.IsVoid()) {
>-      httpChannel->SetRequestHeader(NS_LITERAL_CSTRING("Accept"),
>-                                    NS_LITERAL_CSTRING("*/*"),
>-                                    false);
>+    if (!mAuthorRequestHeaders.Has("accept")) {
>+      mAuthorRequestHeaders.Set("accept", NS_LITERAL_CSTRING("*/*"));
So the old code sets "Accept", new one "accept". No such changes in refactoring patches please.



Sorry, I will need to read this still couple of times to ensure there are no functionality changes.
Attachment #8779054 - Flags: review?(bugs) → review-
Comment on attachment 8779055 [details] [diff] [review]
1285036-11-factor_channel_setup_code_out_of_send.diff


+// Non-XMLHttpRequest-specific channel init goes here.
 nsresult
-XMLHttpRequestMainThread::InitChannel()
+CreateChannelForFetch(nsIChannel** aChannel,
+                      const nsCOMPtr<nsILoadGroup>& aLoadGroup,
+                      const nsCOMPtr<nsIPrincipal>& aPrincipal,
+                      const nsCOMPtr<nsIDocument>& aResponsibleDocument,
+                      const nsACString& aMethod,
+                      nsCOMPtr<nsIURI>& aURL,
+                      bool aIsMozAnon,
+                      bool aIsPrivileged)
I don't understand the reason for this method, which is just called in otherwise small
XMLHttpRequestMainThread::CreateChannelForFetch().
Merge them. And if we later want to have a generic CreateChannelForFetch, do that split in a separate bug.


>-  nsresult InitChannel();
>+  nsresult Fetch(nsIInputStream* aUploadStream, int64_t aUploadLength);
Fetch as a method name makes one think something happens synchronously if there is more work happening after Fetch() call, like there is in 
XMLHttpRequestMainThread::SendInternal.
Rename Fetch to InitiateFetch or some such.

>+
>+  nsresult CreateChannelForFetch();
>+
>+  nsresult SetupChannelForFetch(nsIChannel* aChannel,
>+                                nsPIDOMWindowInner* aOwner,
>+                                nsIPrincipal* aPrincipal,
>+                                nsACString& aMethod,
>+                                RequestHeaders& aHeaders,
>+                                nsIInputStream* aUploadStream,
>+                                int64_t aUploadLength,
>+                                bool aIsPrivileged,
>+                                bool aIsMozAnon,
>+                                bool aWithCredentials,
>+                                bool aForcePreflight,
>+                                bool aWantProgressNotifications);
Not sure why these methods have Fetch in their name. Wouldn't CreateChannel and SetupChannel be simpler and easier to understand.
Attachment #8779055 - Flags: review?(bugs) → review-
>So is this changing the behavior?

No, those parts weren't about behavioral changes, just refactoring. The patch that caused the header case-sensitivity issue was #7, and I'm currently working on a patch to fix that bit (and any other behavioral change I may have introduced in that patchset).


>Did some of the patches land before the previous merge?

Yes, patches 1-9 have already landed in several batches. At this point it will be easiest to revert the changes manually, as my memory is still fresh regarding these changes.


>Sorry, I will need to read this still couple of times to ensure there are no functionality changes.

No, it's my bad for mixing refactoring with behavioral changes. After fixing the above, I'll take another look at these two unlanded patches again as well.


>I don't understand the reason for this method

That was just to pull out the XHR-specific channel-setting bits from the more general stuff. I'll merge those two methods together as you suggest.


>Rename Fetch to InitiateFetch or some such.

Will do.


>Not sure why these methods have Fetch in their name.

Sure, I'll rename them as you recommend.


Thanks for the review effort, I'll also try my best to keep any subsequent patches as small as I can. I hope the next big part (converting send() to using the Fetch driver in /dom/fetch) won't be a horrible nightmare...
Depends on: 1293881
Alright, I've set up bug 1293881 to deal with reverting the behavioral change to header-name casing introduced in patch 7.

I've also changed patch 10 to simply be about factoring the request header code into its own class, rather than also trying to match the spec in the send() method. That should hopefully make it more straightforward to review, as it's basically just moving around code (the accept=*/* code included). Here's hoping the CharsetIterator stuff isn't too subtle!

(I'm not sure whether setting r+ again is necessary, so I apologize in advance if it's a nuisance at this point.)
Attachment #8779054 - Attachment is obsolete: true
Attachment #8779602 - Flags: review?(bugs)
Here's a new version of patch 11. Hopefully this one will be easier to review. It's still just moving code around, but I agree that there's no real point in having so many channel-setup methods. This version just moves all that stuff into InitiateFetch instead. Only minor changes to the moved code were made to accommodate that, rename a couple of things, reformat a few lines for 80 chars, and add/update a couple of comments.

For the change that involves it, bear in mind that mRequestMethod uppercases GET/POST and other values when it's set in the Open method (which of course happens well before Send calls the relevant code).
Attachment #8779055 - Attachment is obsolete: true
Attachment #8779796 - Flags: review?(bugs)
Comment on attachment 8779602 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>exporting patch:
># HG changeset patch
># User Thomas Wisniewski <wisniewskit@gmail.com>
># Date 1470797759 14400
>#      Tue Aug 09 22:55:59 2016 -0400
># Node ID c86d824d9ed027636f9c4378515e6afe4ec1dd34
># Parent  c29c58b018ec75ca8d9a44f08736ef58e1e3855a
>Bug 1285036 - Part 10: Factor the request-header XHR code out into its own class.
>
>diff --git a/dom/xhr/XMLHttpRequestMainThread.cpp b/dom/xhr/XMLHttpRequestMainThread.cpp
>--- a/dom/xhr/XMLHttpRequestMainThread.cpp
>+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
>@@ -2525,37 +2525,33 @@ XMLHttpRequestMainThread::SendInternal(c
>   //     an asynchronous call.
> 
>   // Ignore argument if method is GET, there is no point in trying to
>   // upload anything
>   nsAutoCString method;
>   nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(mChannel));
> 
>   if (httpChannel) {
>+    // If the user hasn't overridden the Accept header, set it to */* as per spec
>+    if (!mAuthorRequestHeaders.Has("accept")) {
>+      mAuthorRequestHeaders.Set("Accept", NS_LITERAL_CSTRING("*/*"));
>+    }
>+
>     // Spec step 5
>-    SetAuthorRequestHeadersOnChannel(httpChannel);
>+    mAuthorRequestHeaders.ApplyToChannel(httpChannel);
Doesn't this end up changing the ordering of headers? Accept might be there earlier than without the patch, right?
Is this change compatible what other browsers do?

Actually, I don't see anything in Fetch spec which says implicit "Accept" should be applied before author headers. 
Fix, or explain where the spec requires this ordering
Attachment #8779602 - Flags: review?(bugs) → review-
Nope. If I perform this XHR:

  var client = new XMLHttpRequest()
  client.open("GET", "", false)
  client.setRequestHeader("X-junk", "x-some-value")
  client.send(null)
  assert_equals(client.responseText, "*/*")
  client.open("GET", "", false)

Here are the headers I get with my patch:

  Host: localhost:8001
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: */*
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8001/
  x-junk: x-some-value

Here are the headers I get without my patch:

  Host: localhost:8001
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: */*
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8001/
  x-junk: x-some-value

And here are the headers from both versions when I also setRequestHeader("accept", "whatever"):

  Host: localhost:8080
  User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
  Accept: whatever
  Accept-Language: en-US,en;q=0.5
  Accept-Encoding: gzip, deflate
  Referer: http://localhost:8080/
  x-junk: x-something/vague, text/html5

So it seems necko always emits the Accept header in the same place regardless.
I should also say that I don't mind omitting that change and just continuing to just set the header on the channel directly. Either way is fine, so I'll update the patch accordingly as your review progresses.
ok, thanks for testing. If necko anyhow orders the headers somehow, then fine.
Attachment #8779602 - Flags: review- → review?(bugs)
But, any reason why you order header setting differently to what the spec does?
No, but I'm not 100% sure what you mean. The XHR spec actually wants us to call the internal Fetch API to actually make the channel and set the headers, but we're doing things so much differently that I doubt we match the spec in many respects. Still, if you feel I could move those lines to another spot to kinda match the spec a bit better, I certainly don't mind doing so.

(Incidentally, I'm working on a proof-of-concept patch to see whether we could just switch over to using the Fetch driver in this code, with minimal changes/wrappers over that API as necessary for compat... but of course that's a whole other can of worms).
I mean, per spec author headers are added first and then "Accept", if author headers didn't contain it, but for some reason you do it in different order.
Oh, I see. No, my code will still end up adding accept after the user's specified headers. It only adds the Accept header if it isn't there to begin with, which will ultimately place at the end of RequestHeaders.mHeaders (an array). ApplyToChannel will then apply them in array order, so it will be set last.

Still, if this is reading that subtly then I'd rather not make the change at all, and just keep it overt (that is, just do mAuthorRequestHeaders.ApplyToChannel, and then consider whether Accept must be set with a subsequent channel.SetRequestHeader).
Comment on attachment 8779602 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>+void
>+RequestHeaders::Get(const nsACString& aName, nsACString& outValue) const
>+{
Why you don't use Find to implement Get?
RequestHeader* header = Find(aName);
if (header) {
  aValue = header->mValue;
} else {
  aValue.SetIsVoid(true);
}




>+RequestHeaders::Merge(const char* aName, const nsACString& aValue)
>+{
>+  Merge(nsDependentCString(aName), aValue);
>+}
>+
>+void
>+RequestHeaders::Merge(const nsACString& aName, const nsACString& aValue)
>+{
>+  RequestHeader* header = Find(aName);
>+  if (header) {
>+    header->value.AppendLiteral(", ");
>+    header->value.Append(aValue);
>+  } else {
>+    RequestHeader newHeader = {
>+      nsCString(aName), nsCString(aValue)
>+    };
>+    mHeaders.AppendElement(newHeader);
>+  }
>+}
Could you rename both Merge methods to MergeOrSet or some such
since just Merge isn't clear what happens when there isn't old value.



>+RequestHeaders::CharsetIterator::Next()
>+{
>+  int32_t start, end;
>+  nsAutoCString charset;
>+  NS_ExtractCharsetFromContentType(Substring(mSource, 0, mCutoff),
>+                                   charset, &mValid, &start, &end);
>+
>+  if (!mValid) {
>+    return false;
>+  }
>+
>+  // Everything after the = sign is the part of the charset we want.
>+  mCurPos = mSource.FindChar('=', start) + 1;
>+  mCurLen = end - mCurPos;
>+
>+  // Special case: the extracted charset is quoted with single quotes.
>+  // For the purpose of preserving what was set we want to handle them
>+  // as delimiters (although they aren't really).
>+  if (charset.Length() >= 2 &&
>+      charset.First() == '\'' &&
>+      charset.Last() == '\'') {
>+    ++mCurPos;
>+    mCurLen -= 2;
>+  }
>+
>+  mCutoff = start;
So this new code misses the rather critical comment how mCutoff is used.
The old code has 
"    ... limiting the
  // search to only look for charsets before the current charset, to
  // prevent finding the same charset twice."
Please keep that comment in some form



>+class RequestHeaders
>+{
>+  struct RequestHeader
>+  {
>+    nsCString name;
>+    nsCString value;
mName and mValue
new code should use the normal coding style.


>+  bool Has(const char* aName) const;
>+  bool Has(const nsACString& aName) const;
>+  void Get(const char* aName, nsACString& outValue) const;
aValue


>+  void Get(const nsACString& aName, nsACString& outValue) const;
aValue


>+  void GetCORSUnsafeHeaders(nsTArray<nsCString>& outArray) const;
aArray

Also method definitions should use aFoo naming
Attachment #8779602 - Flags: review?(bugs) → review-
That accept handling can be as it is in the patch.
Comment on attachment 8779796 [details] [diff] [review]
1285036-11-factor-fetch-code-out-into-InitiateFetch.diff

ok, mostly code move.
Attachment #8779796 - Flags: review?(bugs) → review+
>Why you don't use Find to implement Get?

Just an oversight. I've changed it to do so.


>Could you rename both Merge methods to MergeOrSet
>Please keep that comment in some form
>new code should use the normal coding style.

Done.


Here's an updated patch with those changes.
Attachment #8779602 - Attachment is obsolete: true
Attachment #8779924 - Flags: review?(bugs)
Comment on attachment 8779924 [details] [diff] [review]
1285036-10-factor-request-header-code-into-its-own-class.diff

>+RequestHeaders::Find(const nsACString& aName)
>+{
>+  const nsCaseInsensitiveCStringComparator ignoreCase;
>+  for (RequestHeaders::RequestHeader& header : mHeaders) {
>+    if (header.mName.Equals(aName, ignoreCase)) {
>+      return &header;
>+    }
>+  }
>+  return nullptr;
>+}
>+
>+bool
>+RequestHeaders::Has(const char* aName) const
>+{
>+  return Has(nsDependentCString(aName));
>+}
>+
>+bool
>+RequestHeaders::Has(const nsACString& aName) const
>+{
>+  const nsCaseInsensitiveCStringComparator ignoreCase;
>+  for (const RequestHeader& header : mHeaders) {
>+    if (header.mName.Equals(aName, ignoreCase)) {
>+      return true;
>+    }
>+  }
>+  return false;
Couldn't 'Has' actually be just
  return !!Find(aName);


>+RequestHeaders::CharsetIterator::Next()
>+{
>+  int32_t start, end;
>+  nsAutoCString charset;
>+  NS_ExtractCharsetFromContentType(Substring(mSource, 0, mCutoff),
>+                                   charset, &mValid, &start, &end);
>+
>+  if (!mValid) {
>+    return false;
>+  }
>+
>+  // Everything after the = sign is the part of the charset we want.
>+  mCurPos = mSource.FindChar('=', start) + 1;
>+  mCurLen = end - mCurPos;
>+
>+  // Special case: the extracted charset is quoted with single quotes.
>+  // For the purpose of preserving what was set we want to handle them
>+  // as delimiters (although they aren't really).
>+  if (charset.Length() >= 2 &&
>+      charset.First() == '\'' &&
>+      charset.Last() == '\'') {
>+    ++mCurPos;
>+    mCurLen -= 2;
>+  }
>+
>+  mCutoff = start;
mCutoff must be still documented. It is really hard to understand without the comment I mentioned in the previous review.

with those, r+
Attachment #8779924 - Flags: review?(bugs) → review+
Alright, here's a patch addressing those comments. Carrying over r+.

I'm doing another try run for both patches before requesting check-in, just in case: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b142533deeba
Attachment #8779924 - Attachment is obsolete: true
It turned out that there was one bug with the patches, but it just involved nudging an nsCOMPtr declaration up a couple of lines so that it doesn't get freed prematurely.

Since there weren't any substantial changes, and try seems clear this time, I'm carrying over r+. Try results are here, just the usual unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c83a5fb27876

Requesting checking of patches 10 and 11.
Attachment #8779796 - Attachment is obsolete: true
Keywords: checkin-needed
Actually, I'll upload the patches rebased to inbound, since they might not apply cleanly as-is as they were based to central. Here is 10.
Attachment #8779933 - Attachment is obsolete: true
And here is 11.
Attachment #8782700 - Attachment is obsolete: true
Lots of crashes in that Try push.
Keywords: checkin-needed
Agreed, for the old patch versions from comment 109. But I addressed those crashes in the latest patches as of comment 110, no? Or am I missing something obvious in that try run in comment 110? It only looks like known (yet unrelated) intermittent failures for that version to me, but I'm still pretty new here...
Flags: needinfo?(ryanvm)
Sorry, I missed the link in comment 110.
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
Oh no problem, I just wanted to make sure I wasn't missing something important.

I'll bear in mind to re-summarize the situation in my last comment next time, for the sake of expediency and sanity.
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84dcc03312fd
Part 10: Factor the request-header XHR code out into its own class. r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ce797e5463b
Part 11: Factor the actual fetch-related code out of XHR Send() and into its own function, InitiateFetch(). r=smaug
Keywords: checkin-needed
Given that bug 1293881 reverted part of the code landed here, what is the status of this bug at this point?
That revert didn't really affect the refactoring that was done here, it only reverted a change that went a bit too far for a refactor.

Overall I think that the low-hanging fruit is refactored, but there are still refactoring efforts that probably deserve their own bugs now, like merging the worker and main thread implementations, using the fetch driver like the spec does rather than a separate implementation, and possibly converting over to using whatwg streams (once we have a working implementation of those).

Based on that I think we can either close this bug, or maybe convert it into a meta-bug to track other efforts that should definitely be in their own bug (like the ones listed above). Even if there are other smaller refactoring bits that can be done, they might as well go in their own tickets.
Given that we landed a whole bunch of code in the scope of this bug I think I'd prefer closing. It seems bug 726433 is the meta XMLHttpRequest bug.
Sure, I'll also re-title the bug as well to make it clear it was a partial refactor.
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Refactor the code for XMLHttpRequest → Refactor some of the code in XMLHttpRequest
Depends on: 1334877
You need to log in before you can comment on or make changes to this bug.