Add a stub implementation of the URLLoaderFactory for AppCache.

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

[ananta, 2017-05-17 15:21:38]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[ananta, 2017-05-17 15:35:59]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 15:36:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 15:48:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 15:48:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-05-17 15:59:25]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 15:59:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 16:13:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 16:13:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jam, 2017-05-17 16:51:12]

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:89:
mojo::MakeRequest(&network_loader_request_), routing_id_, request_id_,
I think you want to call Unbind() here and pass this on. or even, is there a
reason to bind the mojom::URLLoaderAssociatedRequest that came in to the
constructor, instead of holding on to it and passing it to the factories here
and below?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:176: void
AppCacheURLLoaderFactory::StartURLLoader(
nit: what's the purpose of this separate method?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:11: #include
"content/public/common/resource_type.h"
nit: unneeded?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:12: #include
"mojo/public/cpp/bindings/binding_set.h"
ditto

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:21: class CONTENT_EXPORT
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
nit: are you sure you need export?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:41: 
nit: no blank line between overrridden methods (also per convention can add
"mojom::URLLoaderFactory implementation:" before overridden methods of an
interface)

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:104: if
(appcache_handle_core) {
doesn't have to be here, but we should really only be calling to appcache
asynchronously if there's a chance that there's an appcache.

can we change the appcache interface so that it can return synchronously when
the in-memory data structure doesn't have an entry for that origin?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:532: std::move(network_factory),
partition->appcache_service_.get());
We're going to need a bunch of stuff out of StoragePartitionImpl:
-network_context()
-GetAppCacheService()

And in future CLs, blob/filesystem and other getters out of it.

Can you make Initialize take a StoragePartitionImpl* and move these few lines
above inside of it? Then as we add more support to network service code path we
don't have to change this file or the Initialize() method

[kinuko, 2017-05-17 16:54:29]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2017-05-17 16:54:30]

Hi-- I want to sync-up before we proceed further, could we chat a bit today?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:71: // The
AppCacheNetworkServiceHandler instance is deleted on return from this
The comment looks stale now

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:183: // This will get
deleted when the connection is dropped by the client.
It doesn't look using StrongBinding, is it true?

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:105: factory =
url_loader_factory_getter->GetAppCacheFactory()->get();
So we almost always take this path after this patch lands.

[ananta, 2017-05-17 18:14:09]

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:71: // The
AppCacheNetworkServiceHandler instance is deleted on return from this
On 2017/05/17 16:54:29, kinuko wrote:
> The comment looks stale now

Done.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:89:
mojo::MakeRequest(&network_loader_request_), routing_id_, request_id_,
On 2017/05/17 16:51:12, jam wrote:
> I think you want to call Unbind() here and pass this on. or even, is there a
> reason to bind the mojom::URLLoaderAssociatedRequest that came in to the
> constructor, instead of holding on to it and passing it to the factories here
> and below?

I ran into a number of problems trying to do that. This is a work in progress at
the moment.
Would it be reasonable to attempt that in a future patchset which should be
coming in soon.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:176: void
AppCacheURLLoaderFactory::StartURLLoader(
On 2017/05/17 16:51:12, jam wrote:
> nit: what's the purpose of this separate method?

Thanks removed.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:183: // This will get
deleted when the connection is dropped by the client.
On 2017/05/17 16:54:30, kinuko wrote:
> It doesn't look using StrongBinding, is it true?

We delete the object in the connection error. Please see the AppCacheURLLoader
class above.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:11: #include
"content/public/common/resource_type.h"
On 2017/05/17 16:51:12, jam wrote:
> nit: unneeded?

Removed.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:12: #include
"mojo/public/cpp/bindings/binding_set.h"
On 2017/05/17 16:51:12, jam wrote:
> ditto

Removed

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:21: class CONTENT_EXPORT
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/05/17 16:51:12, jam wrote:
> nit: are you sure you need export?

We don't need it now. I put that in for tests. If you feel strongly I will
remove it.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:41: 
On 2017/05/17 16:51:12, jam wrote:
> nit: no blank line between overrridden methods (also per convention can add
> "mojom::URLLoaderFactory implementation:" before overridden methods of an
> interface)

Done.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:104: if
(appcache_handle_core) {
On 2017/05/17 16:51:12, jam wrote:
> doesn't have to be here, but we should really only be calling to appcache
> asynchronously if there's a chance that there's an appcache.
> 
> can we change the appcache interface so that it can return synchronously when
> the in-memory data structure doesn't have an entry for that origin?

That is going to be touch tricky to do at the moment. Plan to revisit this once
the pieces fall into place.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:105: factory =
url_loader_factory_getter->GetAppCacheFactory()->get();
On 2017/05/17 16:54:30, kinuko wrote:
> So we almost always take this path after this patch lands.

Yes.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/storage...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:532: std::move(network_factory),
partition->appcache_service_.get());
On 2017/05/17 16:51:12, jam wrote:
> We're going to need a bunch of stuff out of StoragePartitionImpl:
> -network_context()
> -GetAppCacheService()
> 
> And in future CLs, blob/filesystem and other getters out of it.
> 
> Can you make Initialize take a StoragePartitionImpl* and move these few lines
> above inside of it? Then as we add more support to network service code path
we
> don't have to change this file or the Initialize() method

Done.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/storage...
content/browser/storage_partition_impl.cc:532: std::move(network_factory),
partition->appcache_service_.get());
On 2017/05/17 16:51:12, jam wrote:
> We're going to need a bunch of stuff out of StoragePartitionImpl:
> -network_context()
> -GetAppCacheService()
> 
> And in future CLs, blob/filesystem and other getters out of it.
> 
> Can you make Initialize take a StoragePartitionImpl* and move these few lines
> above inside of it? Then as we add more support to network service code path
we
> don't have to change this file or the Initialize() method

Done.

[ananta, 2017-05-17 18:15:23]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 18:16:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:59:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-17 19:59:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[ananta, 2017-05-17 20:45:27]

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:104: if
(appcache_handle_core) {
On 2017/05/17 18:14:09, ananta wrote:
> On 2017/05/17 16:51:12, jam wrote:
> > doesn't have to be here, but we should really only be calling to appcache
> > asynchronously if there's a chance that there's an appcache.
> > 
> > can we change the appcache interface so that it can return synchronously
when
> > the in-memory data structure doesn't have an entry for that origin?
> 
> That is going to be touch tricky to do at the moment. Plan to revisit this
once
> the pieces fall into place.

Scratch that. I added code in the AppCacheURLLoader to call the network service
if we don't find the origin in the usage map.

[ananta, 2017-05-17 20:45:38]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 20:47:27]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 00:11:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-18 00:11:41]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jam, 2017-05-18 14:16:14]

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
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:
> On 2017/05/17 16:51:12, jam wrote:
> > I think you want to call Unbind() here and pass this on. or even, is there a
> > reason to bind the mojom::URLLoaderAssociatedRequest that came in to the
> > constructor, instead of holding on to it and passing it to the factories
here
> > and below?
> 
> I ran into a number of problems trying to do that. This is a work in progress
at
> the moment.
> Would it be reasonable to attempt that in a future patchset which should be
> coming in soon.

Which problems did you run into? I think we should try to fix them before this
lands, otherwise non-appcache requests will go through this class when URLLoader
interface is called.

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:21: class CONTENT_EXPORT
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/05/17 18:14:08, ananta wrote:
> On 2017/05/17 16:51:12, jam wrote:
> > nit: are you sure you need export?
> 
> We don't need it now. I put that in for tests. If you feel strongly I will
> remove it.

please add it when it's needed

https://codereview.chromium.org/2891773002/diff/80001/content/browser/url_loa...
File content/browser/url_loader_factory_getter.cc (right):

https://codereview.chromium.org/2891773002/diff/80001/content/browser/url_loa...
content/browser/url_loader_factory_getter.cc:28:
partition->GetAppCacheService())));
nit: indentation

[ananta, 2017-05-18 19:13:52]

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:21: class CONTENT_EXPORT
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/05/18 14:16:14, jam wrote:
> On 2017/05/17 18:14:08, ananta wrote:
> > On 2017/05/17 16:51:12, jam wrote:
> > > nit: are you sure you need export?
> > 
> > We don't need it now. I put that in for tests. If you feel strongly I will
> > remove it.
> 
> please add it when it's needed

Done.

https://codereview.chromium.org/2891773002/diff/80001/content/browser/url_loa...
File content/browser/url_loader_factory_getter.cc (right):

https://codereview.chromium.org/2891773002/diff/80001/content/browser/url_loa...
content/browser/url_loader_factory_getter.cc:28:
partition->GetAppCacheService())));
On 2017/05/18 14:16:14, jam wrote:
> nit: indentation

Done.

[ananta, 2017-05-18 19:14:03]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 19:15:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-05-19 00:03:51]

lgtm with the one comment about request ID

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2891773002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:89:
mojo::MakeRequest(&network_loader_request_), routing_id_, request_id_,
On 2017/05/18 14:16:14, jam wrote:
> On 2017/05/17 18:14:08, ananta wrote:
> > On 2017/05/17 16:51:12, jam wrote:
> > > I think you want to call Unbind() here and pass this on. or even, is there
a
> > > reason to bind the mojom::URLLoaderAssociatedRequest that came in to the
> > > constructor, instead of holding on to it and passing it to the factories
> here
> > > and below?
> > 
> > I ran into a number of problems trying to do that. This is a work in
progress
> at
> > the moment.
> > Would it be reasonable to attempt that in a future patchset which should be
> > coming in soon.
> 
> Which problems did you run into? I think we should try to fix them before this
> lands, otherwise non-appcache requests will go through this class when
URLLoader
> interface is called.

ok, nvm mind this comment for now, Ananta/Yuzhu/me met to discuss this and put a
summary in https://bugs.chromium.org/p/chromium/issues/detail?id=724323

https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:185:
request_id_--;
this class is created for each navigation request, so all of them would have the
same ID (RDH is global, so it ends up creating different ones). So either hard
code -2 below, as that what is effectively happening, or move this to be a
global.

[ananta, 2017-05-19 00:23:13]

https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader...
File content/browser/loader/navigation_url_loader_network_service.cc (right):

https://codereview.chromium.org/2891773002/diff/120001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:185:
request_id_--;
On 2017/05/19 00:03:50, jam wrote:
> this class is created for each navigation request, so all of them would have
the
> same ID (RDH is global, so it ends up creating different ones). So either hard
> code -2 below, as that what is effectively happening, or move this to be a
> global.

Thanks. I made it a global

[ananta, 2017-05-19 00:23:24]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-05-19 00:23:25]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2891773002/#ps140001
(title: "Address final round of comments")

[commit-bot: I haz the power, 2017-05-19 00:24:35]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 04:08:36]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495153404733780,
"parent_rev": "c4de0303f3d7699d558f8042f946a2825e2d248a", "commit_rev":
"2e65213dd03a7a0f277c5d16d460ae22d391f33c"}

[commit-bot: I haz the power, 2017-05-19 04:08:50]

Description was changed from

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

to

==========
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/+/2e65213dd03a7a0f277c5d16d460...
==========

[commit-bot: I haz the power, 2017-05-19 04:08:51]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2e65213dd03a7a0f277c5d16d460...