Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(601)

Issue 2891773002: Add a stub implementation of the URLLoaderFactory for AppCache. (Closed)

Created:
7 months ago by ananta
Modified:
6 months, 1 week ago
Reviewers:
kinuko, jam
CC:
chromium-reviews, michaeln, wjmaclean, jam, Randy Smith (Not in Mondays), darin-cc_chromium.org, loading-reviews_chromium.org, mmenke
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a stub implementation of the URLLoaderFactory for AppCache. The AppCache factory has one instance per Storage partition and is maintained by the URLLoaderFactoryGetter class. The AppCache factory maintains a reference to the URLLoaderFactoryGetter instance and uses it to retrieve the network service to issue unhandled requests to it. Currently all requests are issued to the network service. Next step is to add functionality to serve AppCache responses from the AppCache. We no longer need the AppCacheNetworkServiceHandler class which was added in the last patchset. Additionally the corresponding methods to initialize the AppCache factory in the AppCacheRequestHandler class are also no longer needed. BUG=715632 Review-Url: https://codereview.chromium.org/2891773002 Cr-Commit-Position: refs/heads/master@{#473081} Committed: https://chromium.googlesource.com/chromium/src/+/2e65213dd03a7a0f277c5d16d460ae22d391f33c

Patch Set 1 #

Patch Set 2 : Remove includes and address presubmit #

Patch Set 3 : Fix compile failures #

Total comments: 28

Patch Set 4 : Address review comments #

Patch Set 5 : Call the network service if the origin is not in the usage map in the AppCacheStorage instance #

Total comments: 2

Patch Set 6 : Address review comments #

Patch Set 7 : Generate a request_id on the same lines as RDHI to prevent DCHECKs in metrics code which validate t… #

Total comments: 2

Patch Set 8 : Address final round of comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+298 lines, -218 lines) Patch
M content/browser/BUILD.gn View 2 chunks +2 lines, -2 lines 0 comments Download
D content/browser/appcache/appcache_network_service_handler.h View 1 chunk +0 lines, -83 lines 0 comments Download
D content/browser/appcache/appcache_network_service_handler.cc View 1 chunk +0 lines, -68 lines 0 comments Download
M content/browser/appcache/appcache_request_handler.h View 1 3 chunks +0 lines, -17 lines 0 comments Download
M content/browser/appcache/appcache_request_handler.cc View 2 chunks +0 lines, -18 lines 0 comments Download
A content/browser/appcache/appcache_url_loader_factory.h View 1 2 3 4 5 1 chunk +63 lines, -0 lines 0 comments Download
A content/browser/appcache/appcache_url_loader_factory.cc View 1 2 3 4 5 1 chunk +185 lines, -0 lines 0 comments Download
M content/browser/loader/navigation_url_loader_network_service.cc View 1 2 3 4 5 6 7 6 chunks +12 lines, -18 lines 0 comments Download
M content/browser/storage_partition_impl.cc View 1 2 3 1 chunk +1 line, -5 lines 0 comments Download
M content/browser/url_loader_factory_getter.h View 1 2 3 4 5 3 chunks +14 lines, -3 lines 0 comments Download
M content/browser/url_loader_factory_getter.cc View 1 2 3 4 5 2 chunks +21 lines, -4 lines 0 comments Download

Messages

Total messages: 35 (24 generated)
ananta
7 months ago (2017-05-17 15:21:39 UTC) #2
jam
https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc File content/browser/appcache/appcache_url_loader_factory.cc (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc#newcode89 content/browser/appcache/appcache_url_loader_factory.cc:89: mojo::MakeRequest(&network_loader_request_), routing_id_, request_id_, I think you want to call ...
7 months ago (2017-05-17 16:51:12 UTC) #11
kinuko
Hi-- I want to sync-up before we proceed further, could we chat a bit today? ...
7 months ago (2017-05-17 16:54:30 UTC) #13
ananta
https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc File content/browser/appcache/appcache_url_loader_factory.cc (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc#newcode71 content/browser/appcache/appcache_url_loader_factory.cc:71: // The AppCacheNetworkServiceHandler instance is deleted on return from ...
7 months ago (2017-05-17 18:14:09 UTC) #14
ananta
https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/navigation_url_loader_network_service.cc File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/navigation_url_loader_network_service.cc#newcode104 content/browser/loader/navigation_url_loader_network_service.cc:104: if (appcache_handle_core) { On 2017/05/17 18:14:09, ananta wrote: > ...
7 months ago (2017-05-17 20:45:27 UTC) #19
jam
https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc File content/browser/appcache/appcache_url_loader_factory.cc (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc#newcode89 content/browser/appcache/appcache_url_loader_factory.cc:89: mojo::MakeRequest(&network_loader_request_), routing_id_, request_id_, On 2017/05/17 18:14:08, ananta wrote: > ...
7 months ago (2017-05-18 14:16:14 UTC) #24
ananta
https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.h File content/browser/appcache/appcache_url_loader_factory.h (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.h#newcode21 content/browser/appcache/appcache_url_loader_factory.h:21: class CONTENT_EXPORT AppCacheURLLoaderFactory : public mojom::URLLoaderFactory { On 2017/05/18 ...
7 months ago (2017-05-18 19:13:52 UTC) #25
jam
lgtm with the one comment about request ID https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc File content/browser/appcache/appcache_url_loader_factory.cc (right): https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcache/appcache_url_loader_factory.cc#newcode89 content/browser/appcache/appcache_url_loader_factory.cc:89: mojo::MakeRequest(&network_loader_request_), ...
7 months ago (2017-05-19 00:03:51 UTC) #28
ananta
https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader/navigation_url_loader_network_service.cc File content/browser/loader/navigation_url_loader_network_service.cc (right): https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader/navigation_url_loader_network_service.cc#newcode185 content/browser/loader/navigation_url_loader_network_service.cc:185: request_id_--; On 2017/05/19 00:03:50, jam wrote: > this class ...
7 months ago (2017-05-19 00:23:13 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2891773002/140001
7 months ago (2017-05-19 00:24:35 UTC) #32
commit-bot: I haz the power
7 months ago (2017-05-19 04:08:51 UTC) #35
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2e65213dd03a7a0f277c5d16d460...

Powered by Google App Engine
This is Rietveld 0eb02b776