Bug 178540

Summary: Move AppCache loading to the NetworkProcess
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: 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 Flags
Patch
none
Patch
none
Archive of layout-test-results from ews100 for mac-elcapitan
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Patch none

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.