644476 - Move CORS implementation out of nsXMLHttpRequest.cpp

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently our CORS implementation lives partly in nsXMLHttpRequest.cpp and partially in nsCrossSiteListenerProxy.cpp.

We really should move the whole thing to nsCrossSiteListenerProxy.cpp for a couple of reasons. First of all it'd clean up nsXMLHttpRequest.cpp, which is big enough as it is.

Second, it will allow us to make the CORS implementation expose a smaller API to the outside world since all related classes live together and can reach each others guts.

Patches coming up

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Remove nsIXMLHttpRequest.openRequest

This isn't technically part of the CORS cleanup, but it was lower down in my patch queue so attaching it here.

Once the patch in bug 641706 has been checked in, there is no difference between nsIXMLHttpRequest::Open and nsIXMLHttpRequest::OpenRequest. So lets just remove the latter as its internal and C++-only.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 2: Move the CORS code to nsCrossSiteListenerProxy.cpp/h

This is the meat of it. This is largely just moving code without modifying it. The only modification to code is the new function NS_StartCORSPreflight, and the modifications to nsXMLHttpRequest::Send to call it rather than doing the work manually.

All other code has been simply moved with no modifications.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 3: Rename classes/variables

The old code was using poor naming in a number of instances. For example referring to the preflight cache as an AccessControlLRUCache. In general all references to "access control" or "ac" has been changed to "CORS". Internally in the (now renamed) nsCORSListenerProxy.cpp, the "CORS" prefixed is dropped on things not exposed externally.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 521412 [details] [diff] [review]
Part 2: Move the CORS code to nsCrossSiteListenerProxy.cpp/h

It is a bit annoying to review patches which move code.
There really should have better tools for reviewing such patches.

>+nsresult
>+NS_StartCORSPreflight(nsIChannel* aRequestChannel,
>+                      nsIStreamListener* aListener,
>+                      nsIPrincipal* aPrincipal,
>+                      PRBool aWithCredentials,
>+                      nsTArray<nsCString>& aACUnsafeHeaders,
>+                      nsIChannel** aPreflightChannel)
...
>+  rv = NS_NewChannel(aPreflightChannel, uri, nsnull,
>+                     loadGroup, nsnull, loadFlags);
>+  NS_ENSURE_SUCCESS(rv, rv);
Just to be safe, could you assign a valid non-null value
to aPreflightChannel only if the whole method succeeds.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 521413 [details] [diff] [review]
Part 3: Rename classes/variables

The file rename *must* wait http://mercurial.selenic.com/bts/issue1576
So r=me for other parts even now, and r=me for the file rename
once hg behaves sanely.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Bob, are you still working on to fix that hg bug?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in:

http://hg.mozilla.org/mozilla-central/rev/bcbbe188032f
http://hg.mozilla.org/mozilla-central/rev/c92bc89edaee
http://hg.mozilla.org/mozilla-central/rev/1aa0da948404

I'll file a separate bug on the rename