Bug 179498 – Add support for service worker generated redirections

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

[youenn fablet]

Add support for service worker generated redirections

[youenn fablet]

Created attachment 326472 [details]
Patch

[youenn fablet]

Created attachment 326567 [details]
Patch

[youenn fablet]

Created attachment 326962 [details]
Patch

[Build Bot]

Comment on attachment 326962 [details]
Patch

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

New failing tests:
http/tests/workers/service/service-worker-redirection-fetch.https.html

[Build Bot]

Created attachment 326967 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 326962 [details]
Patch

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

New failing tests:
fast/parser/xml-error-adopted.xml

[Build Bot]

Created attachment 326968 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

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

[youenn fablet]

Created attachment 326992 [details]
Patch

[youenn fablet]

Created attachment 327016 [details]
Patch

[youenn fablet]

Created attachment 327026 [details]
Patch

[Alex Christensen]

Comment on attachment 327026 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.cpp:130
> +ResourceRequest ResourceRequestBase::redirectedRequest(const ResourceResponse& redirectResponse) const

This feels really hacky and fragile to me, kind of like synthesizeRedirectResponseIfNecessary but worse.  I don't think it's designed well.  Is there a relevant specification?

[youenn fablet]

(In reply to Alex Christensen from comment #11)
> Comment on attachment 327026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327026&action=review
> 
> > Source/WebCore/platform/network/ResourceRequestBase.cpp:130
> > +ResourceRequest ResourceRequestBase::redirectedRequest(const ResourceResponse& redirectResponse) const
> 
> This feels really hacky and fragile to me, kind of like
> synthesizeRedirectResponseIfNecessary but worse.  I don't think it's
> designed well.  Is there a relevant specification?

The closest spec is https://fetch.spec.whatwg.org/#http-redirect-fetch.
We could centralize the handling of redirections currently done in various ResourceHandle/NetworkDataTask places.

[youenn fablet]

Created attachment 327224 [details]
Patch

[youenn fablet]

Improved a little bit the computation of redirection response based on CFNetwork code.

[Darin Adler]

Comment on attachment 327224 [details]
Patch

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

> Source/WebCore/platform/network/ResourceRequestBase.h:176
> +    WEBCORE_EXPORT static bool compare(const ResourceRequest&, const ResourceRequest&);

A function named "compare" is confusing. What does "true" mean for "compare"?

> Source/WebCore/platform/network/ResourceResponseBase.h:50
> +    static bool isRedirectionStatusCode(int httpStatusCode) { return httpStatusCode == 301 || httpStatusCode == 302 || httpStatusCode == 303 || httpStatusCode == 307 || httpStatusCode == 308; }

I think the argument could just be named "code" and this line would be a lot shorter. And if HTTP is important then it should be in the function name; not just the argument name.

> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:133
> +        m_redirectionStatus = RedirectionStatus::None;
> +        ASSERT_NOT_REACHED();

I suggest putting ASSERT_NOT_REACHED first.

[youenn fablet]

Comment on attachment 327224 [details]
Patch

Thanks for the review.

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

>> Source/WebCore/platform/network/ResourceRequestBase.h:176
>> +    WEBCORE_EXPORT static bool compare(const ResourceRequest&, const ResourceRequest&);
> 
> A function named "compare" is confusing. What does "true" mean for "compare"?

Will change to equal.
We should probably change platformCompare as well at some point.

>> Source/WebCore/platform/network/ResourceResponseBase.h:50
>> +    static bool isRedirectionStatusCode(int httpStatusCode) { return httpStatusCode == 301 || httpStatusCode == 302 || httpStatusCode == 303 || httpStatusCode == 307 || httpStatusCode == 308; }
> 
> I think the argument could just be named "code" and this line would be a lot shorter. And if HTTP is important then it should be in the function name; not just the argument name.

"code" is indeed fine.

>> Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp:133
>> +        ASSERT_NOT_REACHED();
> 
> I suggest putting ASSERT_NOT_REACHED first.

OK

[youenn fablet]

Created attachment 327898 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 327898 [details]
Patch for landing

Rejecting attachment 327898 [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', 'apply-attachment', '--no-update', '--non-interactive', 327898, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ing file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/http/tests/workers/service/resources/service-worker-redirection-fetch-worker.js
patching file LayoutTests/http/tests/workers/service/service-worker-redirection-fetch.https-expected.txt
patching file LayoutTests/http/tests/workers/service/service-worker-redirection-fetch.https.html

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

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

[youenn fablet]

Created attachment 327900 [details]
Rebasing

[WebKit Commit Bot]

Comment on attachment 327900 [details]
Rebasing

Clearing flags on attachment: 327900

Committed r225297: <https://trac.webkit.org/changeset/225297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35758489>