Bug 157837

Created attachment 279247 [details] Patch

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

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

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

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

Created attachment 279369 [details] Patch

Created attachment 279376 [details] Fixing style

Created attachment 279388 [details] Handling at DocumentThreadableLoader

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?

(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...)

Created attachment 279486 [details] Handling at CachedResource

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.

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?

(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.

Created attachment 279566 [details] Adding cache test

Comment on attachment 279566 [details] Adding cache test This looks good to me. Is there a reason there's no r?

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

Created attachment 279633 [details] Patch for landing

Comment on attachment 279633 [details] Patch for landing Clearing flags on attachment: 279633 Committed r201324: <http://trac.webkit.org/changeset/201324>

All reviewed patches have been landed. Closing bug.