Bug 178540 – Move AppCache loading to the NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Move AppCache loading to the NetworkProcess

[Alex Christensen]

Created attachment 324279 [details]
Patch

[Alex Christensen]

Created attachment 324448 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brent Fulgham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/37119346>

[youenn fablet]

Created attachment 334404 [details]
Patch

[Alex Christensen]

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

[youenn fablet]

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.

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 334404 [details]
Patch

Clearing flags on attachment: 334404

Committed r228901: <https://trac.webkit.org/changeset/228901>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.