Move AppCache loading to the NetworkProcess
Created attachment 324279 [details] Patch
Created attachment 324448 [details] Patch
Comment on attachment 324448 [details] Patch Attachment 324448 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4937462 New failing tests: http/tests/appcache/remove-cache.html http/tests/appcache/update-cache.html
Created attachment 324457 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324448 [details] Patch Attachment 324448 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4937504 New failing tests: http/tests/appcache/remove-cache.html http/tests/appcache/update-cache.html
Created attachment 324461 [details] Archive of layout-test-results from ews114 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 324448 [details] Patch Attachment 324448 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4937585 New failing tests: http/tests/appcache/remove-cache.html http/tests/appcache/update-cache.html
Created attachment 324466 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 324448 [details] Patch Attachment 324448 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4939455 New failing tests: http/tests/appcache/remove-cache.html http/tests/appcache/update-cache.html
Created attachment 324482 [details] Archive of layout-test-results from ews105 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 324448 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=324448&action=review Bots aren't happy with this patch, so r-. I'm also unclear on the firstRequest/originalRequest changes and what those mean. Maybe a comment in the changelog would help. > Source/WebCore/ChangeLog:3 > + Move AppCache loading to the NetworkProcess Yes please! > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:484 > + auto handleOrError = m_cachedResourceLoader->requestRawResource({ResourceRequest(request), { SendCallbacks, DoNotSniffContent, BufferData, StoredCredentialsPolicy::Use, ClientCredentialPolicy::MayAskClientForCredentials, FetchOptions::Credentials::Include, DoSecurityCheck, FetchOptions::Mode::NoCors, DoNotIncludeCertificateInfo, ContentSecurityPolicyImposition::DoPolicyCheck, DefersLoadingPolicy::AllowDefersLoading, CachingPolicy::DisallowCaching }}); Isn't 'request' already a ResourceRequest? Why does it need to be wrapped again like this? > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:563 > + m_pendingEntries.remove(m_currentHandle->originalRequest()->url()); Was using firstRequest always wrong in the original code? I'm a little confused by the various 'firstRequest' versus 'originalRequest' code changes.
<rdar://problem/37119346>
Created attachment 334404 [details] Patch
Comment on attachment 334404 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334404&action=review > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:273 > + // FIXME: Add support for ApplicationCache in inspector. This hopefully won't be worth it because ApplicationCache is on its way out. > Source/WebCore/loader/SubstituteResource.h:52 > + ResourceResponse& resourceResponse() { return m_response; } It seems like there could be a cleaner way to set the source than a non-const resourceResponse accessor. Maybe just a setter for the source. > Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.cpp:33 > +RefPtr<ApplicationCacheResourceLoader> ApplicationCacheResourceLoader::create(unsigned type, CachedResourceLoader& loader, ResourceRequest&& request, CompletionHandler<void(ResourceOrError&&)>&& callback) I think type should be a ApplicationCacheResource::Type instead of an unsigned. > Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.h:62 > + unsigned m_type; ApplicationCacheResource::Type
Thanks for the review. > > Source/WebCore/loader/SubstituteResource.h:52 > > + ResourceResponse& resourceResponse() { return m_response; } > > It seems like there could be a cleaner way to set the source than a > non-const resourceResponse accessor. Maybe just a setter for the source. Ideally, SubstituteResource should take a ResourceResponse&&, in which case we would pass the modified response with the right source. I'll see if that adds too much code in this patch and will file a second patch if needed. > > Source/WebCore/loader/appcache/ApplicationCacheResourceLoader.cpp:33 > > +RefPtr<ApplicationCacheResourceLoader> ApplicationCacheResourceLoader::create(unsigned type, CachedResourceLoader& loader, ResourceRequest&& request, CompletionHandler<void(ResourceOrError&&)>&& callback) > > I think type should be a ApplicationCacheResource::Type instead of an > unsigned. I am not sure what is ApplicationCacheResource::Type. This is more a bit-mask thing than an actual enum type. Current code is using unsigned to store it in m_pendingEntries and are doing some binary operations on them.
> > Source/WebCore/loader/SubstituteResource.h:52 > > + ResourceResponse& resourceResponse() { return m_response; } > > It seems like there could be a cleaner way to set the source than a > non-const resourceResponse accessor. Maybe just a setter for the source. Will fix it as a follow-up
Comment on attachment 334404 [details] Patch Clearing flags on attachment: 334404 Committed r228901: <https://trac.webkit.org/changeset/228901>
All reviewed patches have been landed. Closing bug.