Closed Bug 644476 Opened 10 years ago Closed 10 years ago

Move CORS implementation out of nsXMLHttpRequest.cpp


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

Not set





(Reporter: sicking, Assigned: sicking)




(3 files)

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
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.
Assignee: nobody → jonas
Attachment #521410 - Flags: review?(Olli.Pettay)
Attachment #521410 - Attachment description: Remove nsIXMLHttpRequest.openRequest → Part 1: Remove nsIXMLHttpRequest.openRequest
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.
Attachment #521412 - Flags: review?(Olli.Pettay)
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.
Attachment #521413 - Flags: review?(Olli.Pettay)
Attachment #521410 - Flags: review?(Olli.Pettay) → review+
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.

>+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.
Attachment #521412 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 521413 [details] [diff] [review]
Part 3: Rename classes/variables

The file rename *must* wait
So r=me for other parts even now, and r=me for the file rename
once hg behaves sanely.
Attachment #521413 - Flags: review?(Olli.Pettay) → review+
Bob, are you still working on to fix that hg bug?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.