Add support for service worker generated redirections
Created attachment 326472 [details] Patch
Created attachment 326567 [details] Patch
Created attachment 326962 [details] Patch
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
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
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
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
Created attachment 326992 [details] Patch
Created attachment 327016 [details] Patch
Created attachment 327026 [details] Patch
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?
(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.
Created attachment 327224 [details] Patch
Improved a little bit the computation of redirection response based on CFNetwork code.
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.
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
Created attachment 327898 [details] Patch for landing
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
Created attachment 327900 [details] Rebasing
Comment on attachment 327900 [details] Rebasing Clearing flags on attachment: 327900 Committed r225297: <https://trac.webkit.org/changeset/225297>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35758489>