Bug 157837 – [Fetch API] Implement Fetch redirect mode

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Implement https://fetch.spec.whatwg.org/#http-fetch step 5

[youenn fablet]

Created attachment 279247 [details]
Patch

[Build Bot]

Comment on attachment 279247 [details]
Patch

Attachment 279247 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1343096

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html

[Build Bot]

Created attachment 279251 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 279247 [details]
Patch

Attachment 279247 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1343095

New failing tests:
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-location.html
imported/w3c/web-platform-tests/fetch/api/redirect/redirect-mode.html

[Build Bot]

Created attachment 279254 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[youenn fablet]

Created attachment 279369 [details]
Patch

[youenn fablet]

Created attachment 279376 [details]
Fixing style

[youenn fablet]

Created attachment 279388 [details]
Handling at DocumentThreadableLoader

[Alex Christensen]

Comment on attachment 279388 [details]
Handling at DocumentThreadableLoader

View in context: https://bugs.webkit.org/attachment.cgi?id=279388&action=review

> Source/WebCore/loader/DocumentThreadableLoader.cpp:197
> +        request = ResourceRequest();

{ }

> Source/WebCore/loader/DocumentThreadableLoader.cpp:298
> +        ResourceResponse actualResponse = response;

I know response is const, but is there any way we could adding another ResourceResponse copying for every response?  Like make the type mutable.  I don't know if that would be a bad idea, but I think there are too many places in the loading code where we need to change something small so we make a completely new copy of the whole object to pass on, and that's becoming less efficient.

> Source/WebCore/platform/network/ResourceResponseBase.h:131
> +    enum class Type { Basic, Cors, Default, Error, Opaque, Opaqueredirect };

OpaqueRedirect.
Will we need to add uses of the other values here?

[youenn fablet]

(In reply to comment #9)
> Comment on attachment 279388 [details]
> Handling at DocumentThreadableLoader
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279388&action=review
> 
> > Source/WebCore/loader/DocumentThreadableLoader.cpp:197
> > +        request = ResourceRequest();
> 
> { }

OK

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:298
> > +        ResourceResponse actualResponse = response;
> 
> I know response is const, but is there any way we could adding another
> ResourceResponse copying for every response?  Like make the type mutable.  I
> don't know if that would be a bad idea, but I think there are too many
> places in the loading code where we need to change something small so we
> make a completely new copy of the whole object to pass on, and that's
> becoming less efficient.

Agreed, this is another indication that fetch options may be best addressed further below, at CachedResource/CachedResourceLoader level typically.
I'll try that.

> > Source/WebCore/platform/network/ResourceResponseBase.h:131
> > +    enum class Type { Basic, Cors, Default, Error, Opaque, Opaqueredirect };
> 
> OpaqueRedirect.

This maps ResponseType JS enumeration, which is "opaqueredirect".
To avoid some boilerplate code, this type is also used in FetchResponse and thus in binding generated code, which then expects Opaqueredirect.

> Will we need to add uses of the other values here?

Yes, when adding support for fetch mode (cors, no-cors...)

[youenn fablet]

Created attachment 279486 [details]
Handling at CachedResource

[Alex Christensen]

Comment on attachment 279486 [details]
Handling at CachedResource

View in context: https://bugs.webkit.org/attachment.cgi?id=279486&action=review

> Source/WebCore/loader/SubresourceLoader.cpp:178
> +        if (options().fetchOptions().redirect != FetchOptions::Redirect::Follow) {

This will only happen for fetch requests, right?

> Source/WebCore/loader/SubresourceLoader.cpp:185
> +            didFinishLoading(0);

This shouldn't be 0.

[youenn fablet]

I need to check cache impact. Probably opaqueredirect responses should not be reused as a first step.

The plan would be to progressively replace loader options with fetch options.

(In reply to comment #12)
> Comment on attachment 279486 [details]
> Handling at CachedResource
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279486&action=review
> 
> > Source/WebCore/loader/SubresourceLoader.cpp:178
> > +        if (options().fetchOptions().redirect != FetchOptions::Redirect::Follow) {
> 
> This will only happen for fetch requests, right?

Afaik, no spec is using a different option.

> 
> > Source/WebCore/loader/SubresourceLoader.cpp:185
> > +            didFinishLoading(0);
> 
> This shouldn't be 0.

Should it be current time then?

[Alex Christensen]

(In reply to comment #13)
> > > Source/WebCore/loader/SubresourceLoader.cpp:185
> > > +            didFinishLoading(0);
> > 
> > This shouldn't be 0.
> 
> Should it be current time then?
I think so.  I think resource timing might use this time.

[youenn fablet]

Created attachment 279566 [details]
Adding cache test

[Alex Christensen]

Comment on attachment 279566 [details]
Adding cache test

This looks good to me.  Is there a reason there's no r?

[WebKit Commit Bot]

Comment on attachment 279566 [details]
Adding cache test

Rejecting attachment 279566 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 279566, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 201275 = d203567a81a107ea42ccf2beaa1793fad5ac14a5
r201287 = 1c84931541e360a58a9cc8eebba09b08761f0d95
r201288 = 18c7977d24f4e88e92d042c1fc98bbb076bb95c8
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/1370134

[youenn fablet]

Created attachment 279633 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 279633 [details]
Patch for landing

Clearing flags on attachment: 279633

Committed r201324: <http://trac.webkit.org/changeset/201324>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.