Bug 178540 - Move AppCache loading to the NetworkProcess
Summary: Move AppCache loading to the NetworkProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 182861
Blocks: 183192
  Show dependency treegraph
 
Reported: 2017-10-19 13:45 PDT by Alex Christensen
Modified: 2018-03-15 15:58 PDT (History)
14 users (show)

See Also:


Attachments
Patch (25.75 KB, patch)
2017-10-19 13:45 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (24.66 KB, patch)
2017-10-20 14:57 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1004.89 KB, application/zip)
2017-10-20 16:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.78 MB, application/zip)
2017-10-20 16:15 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (13.59 MB, application/zip)
2017-10-20 16:41 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.27 MB, application/zip)
2017-10-20 19:52 PDT, Build Bot
no flags Details
Patch (44.62 KB, patch)
2018-02-21 12:35 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2017-10-19 13:45:04 PDT
Move AppCache loading to the NetworkProcess
Comment 1 Alex Christensen 2017-10-19 13:45:56 PDT
Created attachment 324279 [details]
Patch
Comment 2 Alex Christensen 2017-10-20 14:57:51 PDT
Created attachment 324448 [details]
Patch
Comment 3 Build Bot 2017-10-20 16:03:45 PDT
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
Comment 4 Build Bot 2017-10-20 16:03:47 PDT
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 5 Build Bot 2017-10-20 16:15:24 PDT
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
Comment 6 Build Bot 2017-10-20 16:15:25 PDT
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 7 Build Bot 2017-10-20 16:41:48 PDT
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
Comment 8 Build Bot 2017-10-20 16:41:50 PDT
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 9 Build Bot 2017-10-20 19:52:37 PDT
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
Comment 10 Build Bot 2017-10-20 19:52:38 PDT
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 11 Brent Fulgham 2017-12-28 14:18:51 PST
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.
Comment 12 Radar WebKit Bug Importer 2018-02-01 09:46:24 PST
<rdar://problem/37119346>
Comment 13 youenn fablet 2018-02-21 12:35:39 PST
Created attachment 334404 [details]
Patch
Comment 14 Alex Christensen 2018-02-21 13:36:19 PST
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
Comment 15 youenn fablet 2018-02-21 14:05:02 PST
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.
Comment 16 youenn fablet 2018-02-21 14:14:53 PST
> > 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 17 WebKit Commit Bot 2018-02-21 14:37:32 PST
Comment on attachment 334404 [details]
Patch

Clearing flags on attachment: 334404

Committed r228901: <https://trac.webkit.org/changeset/228901>
Comment 18 WebKit Commit Bot 2018-02-21 14:37:33 PST
All reviewed patches have been landed.  Closing bug.