Summary: | Move AppCache loading to the NetworkProcess | ||
---|---|---|---|
Product: | WebKit | Reporter: | Alex Christensen <achristensen> |
Component: | New Bugs | Assignee: | youenn fablet <youennf> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | bfulgham, buildbot, cdumez, commit-queue, dbates, ddkilzer, ews-watchlist, japhet, joepeck, mkwst, pvollan, rniwa, webkit-bug-importer, youennf |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=183020 https://bugs.webkit.org/show_bug.cgi?id=183684 |
||
Bug Depends on: | 182861 | ||
Bug Blocks: | 183192 | ||
Attachments: |
Description
Alex Christensen
2017-10-19 13:45:04 PDT
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. 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. |