1130924 - Add method of chrome only settings to fetch API

Status: RESOLVED WONTFIX

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

Description

[Marcos Caceres [:marcosc]]

> I think it might be easier to add a 3-arg form of fetch(), where the 3rd arg is a dictionary of chrome only settings. Then make the 3-arg form [ChromeOnly]. That way fetch could directly override some settings of Request.

Comment 1

[Boris Zbarsky [:bzbarsky]]

You can't have different exposure settings on different overloads, unfortunately.  That is, you can't mark one overload as [ChromeOnly].

Which is good, because doing it that way would mean that the chrome-only version constrained the way the API can evolve, no?

We may want a [ChromeOnly] function with a different name that takes the settings dictionary in question....

Comment 2

[Marcos Caceres [:marcosc]]

(In reply to Boris Zbarsky [:bz] from comment #1)
> You can't have different exposure settings on different overloads,
> unfortunately.  That is, you can't mark one overload as [ChromeOnly].
> 
> Which is good, because doing it that way would mean that the chrome-only
> version constrained the way the API can evolve, no?

Yeah, I was thinking the same. 

> We may want a [ChromeOnly] function with a different name that takes the
> settings dictionary in question....

Sounds good. Will do that.

Comment 3

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1130924-Add-method-of-chrome-only-settings-to-fe.patch

So I've managed to implement the method, but I think nsCORSListenerProxy is intervening (blocking requests explicitly marked as "no-cors"). It FailWithNetworkError() on line 620 in the "OnStartRequest" method. 

Any guidance on how to proceed would be appreciated.

Comment 4

[Marcos Caceres [:marcosc]]

Also, I was thinking we can then also dump `setContext()` before anyone it in favor of this more extensbile solution.

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 8566384 [details] [diff] [review]
0001-Bug-1130924-Add-method-of-chrome-only-settings-to-fe.patch

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

::: dom/fetch/FetchDriver.cpp
@@ +278,5 @@
> +    bool corsFlag = true;
> +    if (mRequest->Mode() == RequestMode::No_cors){ 
> +      corsFlag = false;
> +    }
> +    return HttpFetch(corsFlag);

Why did you change this block? As in, I don't understand the intent.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Could you describe the situation or add a small test case where the fetch failed? What was the request, from which page/chrome was it called?

Comment 7

[Marcos Caceres [:marcosc]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #5)
> Comment on attachment 8566384 [details] [diff] [review]
> 0001-Bug-1130924-Add-method-of-chrome-only-settings-to-fe.patch
> 
> Review of attachment 8566384 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/FetchDriver.cpp
> @@ +278,5 @@
> > +    bool corsFlag = true;
> > +    if (mRequest->Mode() == RequestMode::No_cors){ 
> > +      corsFlag = false;
> > +    }
> > +    return HttpFetch(corsFlag);
> 
> Why did you change this block? As in, I don't understand the intent.

IIRC, you had it set to always be "true". That would mean that overriding with "no-cors" would do nothing.

Comment 8

[Marcos Caceres [:marcosc]]

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #6)
> Could you describe the situation or add a small test case where the fetch
> failed? What was the request, from which page/chrome was it called?

Will send a patch with the tests in a bit (in the middle of building :)), but it's basically this: 

```
  var url = "http://test1.example.org:80/tests/dom/tests/mochitest/fetch/fetch_chrome_helper.html";
  var r = new w.Request(url);
  r.overrideSettings({mode: "no-cors"});
  w.fetch(r).then(res=> console.log(res), (err)=> console.log(err)); 
```

That gives shows me a "blocked by CORS" error in the JS console.

Comment 9

[Marcos Caceres [:marcosc]]

Attachment: 0001-Bug-1130924-Tests-for-overrideSettings.patch

These are the tests thus far...

Comment 10

[Marcos Caceres [:marcosc]]

Might not need this particular functionality after all (as we might just force CORS by default)... I'll reopen depending on what happens with the web manifest spec.