Get main frame and subframe AppCache loads to work.

Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame and 
subframe. Subresource loads will be addressed in a later patchset. With this change
the w3schools.com AppCache sample here https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService() function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request. The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound to the
   AppCacheJob base class as these implementations could be shared between the URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

6. Add WeakPtrFactory to the AppCacheStorage class and provide a getter to return a weak pointer reference to the instance.

BUG=715632
Review-Url: https://codereview.chromium.org/2902653002
Cr-Commit-Position: refs/heads/master@{#478459}
Committed: https://chromium.googlesource.com/chromium/src/+/a2c8ec6149d02d7cfe9389d9739236ed06571a6a

[ananta, 2017-05-23 01:53:47]

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

[ananta, 2017-05-23 01:53:56]

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

[commit-bot: I haz the power, 2017-05-23 01:54:17]

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-23 02:07:01]

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

[commit-bot: I haz the power, 2017-05-23 02:07:02]

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

[ananta, 2017-05-23 02:24:36]

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

[commit-bot: I haz the power, 2017-05-23 02:25:07]

Dry run: CQ is trying da patch.

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

[ananta, 2017-05-23 02:27:24]

Description was changed from

==========
Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
the URL.

The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

I reverted portions of the previous change AppCacheURLLoaderRequest which was to
hold on to a unique_ptr for the ResourceRequest. We now hold
on to a copy of the request. The CreateLoaderAndStart() method on the factory
gives us a copy of the request.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

to

==========
Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
the URL.

The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
unique_ptr for the ResourceRequest. We now hold
on to a copy of the request. The CreateLoaderAndStart() method on the factory
gives us a copy of the request.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

[commit-bot: I haz the power, 2017-05-23 03:34:25]

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

[commit-bot: I haz the power, 2017-05-23 03:34:26]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-05-23 20:41:41]

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

[ananta, 2017-05-23 20:41:42]

+jam for content changes

[michaeln, 2017-05-24 00:59:32]

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:30: AppCacheHost* g_host
= nullptr;
I don't think we should add this, i'd vote to take the time, in the context of
these early code reviews, to figure out how to more cleanly structure things.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:61: if
(appcache_service_->storage()->usage_map()->find(
we need to test IsInitTaskComplete() for this check to be valid

Another place for this test might be
AppCacheRequestHandler::MaybeLoadMainResource(), that way the current
net::URLRequest based impl would get the benefit of this early return too.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:70:
appcache_service_->storage()->FindResponseForMainRequest(request_.url,
Since this class is only used the main resource, maybe we can name accordingly?
AppCacheMainResourceURLLoader

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:84: void
OnMainResponseFound(const GURL& url,
When we talk a couple weeks ago, we talked about reusing AppCacheRequestHandler
logic, having this new code be a consumer of that existing code.

This class could internally use an instance of the AppCacheRquestHandler to
perform the work of figuring out what resource to load.  Start could call
handler->MaybeLoadResource() and have the handler would do the work of looking
for the main resource. I think we could end up with more code reuse and greater
compat between the new and old impls.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:141: // We don't own the
AppCacheHost pointer. This is safe as the host outlives
what guarantees that?

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:19: class
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
How about we make this class specific to MainResource loads too.

Why does this class that exposes mojo methods? I think the consumer of this
factory is navigation code that runs in the same process.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:66: int
renderer_process_id_;
If AppCacheURLLoaderFactory is a per-storagepartition object (i think it is), I
don't think this datamember belongs here.

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:111: factory =
url_loader_factory_getter->GetAppCacheFactory()->get();
I think I see a problem with this function. It seems like there's an assumption
that the appcache and serviceworker systems can synchronously determine IF they
will produce a response for the request. But figuring out which system (sw,
appcache, network) the request will be loaded from is an asyncrhonous process.
As coded, if the sw system returns a factory_ptr, the appcache system will not
get a chance.

Q: Can the |url_loader_request| be left unsatisfied upon return from this
function with the expectation that it will be satisfied asyncly? What if we
don't CreateLoaderAndStart prior to return?

Why does "factory_from_ui" take precedence over appacache but not over
serviceworkers What is the use case for the "factory_from_ui". Oh, I see, its
for WebUI handling for chrome:urls. Maybe we could call this webui_factory to
better connect the dots, i thought "ui" was referring to it being posted over
from the UI thread

I think its useful to keep appcache and serviceworker impls as parallel as
possible where the function is the same. I'd like to see this logic buried put
into appacache specific function more similar to that one sketched out for the
serviceworker.

[ananta, 2017-05-24 02:14:03]

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:30: AppCacheHost* g_host
= nullptr;
On 2017/05/24 00:59:32, michaeln wrote:
> I don't think we should add this, i'd vote to take the time, in the context of
> these early code reviews, to figure out how to more cleanly structure things.

This code is going to change once kinuko lands her changes to not use factories
in the browser navigation code path. We will have a much cleaner C++ call to
pass the host in then. This is just to get us started on the route to use
AppCacheRequestHandler to initiate requests.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:61: if
(appcache_service_->storage()->usage_map()->find(
On 2017/05/24 00:59:32, michaeln wrote:
> we need to test IsInitTaskComplete() for this check to be valid
> 
> Another place for this test might be
> AppCacheRequestHandler::MaybeLoadMainResource(), that way the current
> net::URLRequest based impl would get the benefit of this early return too.

Added a virtual function IsInitialized() in AppCacheStorage which internally
calls this in the AppCacheStorageImpl class.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:70:
appcache_service_->storage()->FindResponseForMainRequest(request_.url,
On 2017/05/24 00:59:32, michaeln wrote:
> Since this class is only used the main resource, maybe we can name
accordingly?
> AppCacheMainResourceURLLoader

This class should be handling main and sub resources both. Added a check for the
same.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:84: void
OnMainResponseFound(const GURL& url,
On 2017/05/24 00:59:32, michaeln wrote:
> When we talk a couple weeks ago, we talked about reusing
AppCacheRequestHandler
> logic, having this new code be a consumer of that existing code.
> 
> This class could internally use an instance of the AppCacheRquestHandler to
> perform the work of figuring out what resource to load.  Start could call
> handler->MaybeLoadResource() and have the handler would do the work of looking
> for the main resource. I think we could end up with more code reuse and
greater
> compat between the new and old impls.

Yes. The plan is to do that. I was going to call
AppCacheRequestHandler::MaybeLoadResource here. I see your point though. I will
look into this in a subsequent patchset when we start implementing the core
logic for the AppCache read

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.cc:141: // We don't own the
AppCacheHost pointer. This is safe as the host outlives
On 2017/05/24 00:59:32, michaeln wrote:
> what guarantees that?

Changed to take the host in the Start() function. We will eventually pass it to
the AppCacheRequestHandler instance

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:19: class
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/05/24 00:59:32, michaeln wrote:
> How about we make this class specific to MainResource loads too.
> 
> Why does this class that exposes mojo methods? I think the consumer of this
> factory is navigation code that runs in the same process.

This class exposes mojo methods because it implements the URLLoaderFactory
mojom. Once kinuko lands her changes to not use the factory in the browser, we
will revisit this. This factory will be 
passed to the renderer for subresources.

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:66: int
renderer_process_id_;
On 2017/05/24 00:59:32, michaeln wrote:
> If AppCacheURLLoaderFactory is a per-storagepartition object (i think it is),
I
> don't think this datamember belongs here.

Needs to be revisited after kinuko's changes come in. If needed we will have a
new class which handles navigation side code for AppCache with the factory only
used for subresources.

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/loader/...
content/browser/loader/navigation_url_loader_network_service.cc:114:
AppCacheURLLoaderFactory::SetAppCacheHost(appcache_handle_core->host());
The plan for now is to have each factory call the next guy in the chain if it
does not handle it. kinuko's change will basically restart the request if one
handler decides to not handle the request. I think we are ok for now here.
This is again a WIP

[ananta, 2017-05-24 02:21:55]

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

[commit-bot: I haz the power, 2017-05-24 02:23:25]

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-24 02:34:51]

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

[commit-bot: I haz the power, 2017-05-24 02:34:52]

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

[kinuko, 2017-05-24 03:03:20]

On 2017/05/24 02:14:03, ananta wrote:
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> File content/browser/appcache/appcache_url_loader_factory.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.cc:30: AppCacheHost*
g_host
> = nullptr;
> On 2017/05/24 00:59:32, michaeln wrote:
> > I don't think we should add this, i'd vote to take the time, in the context
of
> > these early code reviews, to figure out how to more cleanly structure
things.
> 
> This code is going to change once kinuko lands her changes to not use
factories
> in the browser navigation code path. We will have a much cleaner C++ call to
> pass the host in then. This is just to get us started on the route to use
> AppCacheRequestHandler to initiate requests.
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.cc:61: if
> (appcache_service_->storage()->usage_map()->find(
> On 2017/05/24 00:59:32, michaeln wrote:
> > we need to test IsInitTaskComplete() for this check to be valid
> > 
> > Another place for this test might be
> > AppCacheRequestHandler::MaybeLoadMainResource(), that way the current
> > net::URLRequest based impl would get the benefit of this early return too.
> 
> Added a virtual function IsInitialized() in AppCacheStorage which internally
> calls this in the AppCacheStorageImpl class.
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.cc:70:
> appcache_service_->storage()->FindResponseForMainRequest(request_.url,
> On 2017/05/24 00:59:32, michaeln wrote:
> > Since this class is only used the main resource, maybe we can name
> accordingly?
> > AppCacheMainResourceURLLoader
> 
> This class should be handling main and sub resources both. Added a check for
the
> same.
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.cc:84: void
> OnMainResponseFound(const GURL& url,
> On 2017/05/24 00:59:32, michaeln wrote:
> > When we talk a couple weeks ago, we talked about reusing
> AppCacheRequestHandler
> > logic, having this new code be a consumer of that existing code.
> > 
> > This class could internally use an instance of the AppCacheRquestHandler to
> > perform the work of figuring out what resource to load.  Start could call
> > handler->MaybeLoadResource() and have the handler would do the work of
looking
> > for the main resource. I think we could end up with more code reuse and
> greater
> > compat between the new and old impls.
> 
> Yes. The plan is to do that. I was going to call
> AppCacheRequestHandler::MaybeLoadResource here. I see your point though. I
will
> look into this in a subsequent patchset when we start implementing the core
> logic for the AppCache read
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.cc:141: // We don't own
the
> AppCacheHost pointer. This is safe as the host outlives
> On 2017/05/24 00:59:32, michaeln wrote:
> > what guarantees that?
> 
> Changed to take the host in the Start() function. We will eventually pass it
to
> the AppCacheRequestHandler instance
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> File content/browser/appcache/appcache_url_loader_factory.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.h:19: class
> AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
> On 2017/05/24 00:59:32, michaeln wrote:
> > How about we make this class specific to MainResource loads too.
> > 
> > Why does this class that exposes mojo methods? I think the consumer of this
> > factory is navigation code that runs in the same process.
> 
> This class exposes mojo methods because it implements the URLLoaderFactory
> mojom. Once kinuko lands her changes to not use the factory in the browser, we
> will revisit this. This factory will be 
> passed to the renderer for subresources.
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.h:66: int
> renderer_process_id_;
> On 2017/05/24 00:59:32, michaeln wrote:
> > If AppCacheURLLoaderFactory is a per-storagepartition object (i think it
is),
> I
> > don't think this datamember belongs here.
> 
> Needs to be revisited after kinuko's changes come in. If needed we will have a
> new class which handles navigation side code for AppCache with the factory
only
> used for subresources.
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/loader/...
> File content/browser/loader/navigation_url_loader_network_service.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/loader/...
> content/browser/loader/navigation_url_loader_network_service.cc:114:
> AppCacheURLLoaderFactory::SetAppCacheHost(appcache_handle_core->host());
> The plan for now is to have each factory call the next guy in the chain if it
> does not handle it. kinuko's change will basically restart the request if one
> handler decides to not handle the request. I think we are ok for now here.
> This is again a WIP

Yes, I'm working on this part and I expect we'll be re-doing some of the current
plumbing after that. (Fyi, crbug.com/724322)

[ananta, 2017-05-24 03:38:43]

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

[commit-bot: I haz the power, 2017-05-24 03:40:15]

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-24 04:05:15]

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

[commit-bot: I haz the power, 2017-05-24 04:05:15]

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-24 05:36:00]

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

[commit-bot: I haz the power, 2017-05-24 05:36:30]

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-24 06:02:04]

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

[commit-bot: I haz the power, 2017-05-24 06:02:05]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[ananta, 2017-05-24 06:05:13]

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

[commit-bot: I haz the power, 2017-05-24 06:05:43]

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-24 07:34:35]

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

[commit-bot: I haz the power, 2017-05-24 07:34:35]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-24 15:29:03]

I'm still curious whether it's beneficial to keep landing a bunch of small cls
that are not functional? Each one seems to change code done in the previous cl.
Can we wait till we have something working end-to-end, even just for main frame
requests?

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

https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_factory.h:66: int
renderer_process_id_;
On 2017/05/24 02:14:03, ananta wrote:
> On 2017/05/24 00:59:32, michaeln wrote:
> > If AppCacheURLLoaderFactory is a per-storagepartition object (i think it
is),
> I
> > don't think this datamember belongs here.
> 
> Needs to be revisited after kinuko's changes come in. If needed we will have a
> new class which handles navigation side code for AppCache with the factory
only
> used for subresources.

Since you're not passing a factoryptr to the renderer yet, what's the need to
have a renderer process id in this class since it's always -1?

https://codereview.chromium.org/2902653002/diff/140001/content/browser/url_lo...
File content/browser/url_loader_factory_getter.cc (right):

https://codereview.chromium.org/2902653002/diff/140001/content/browser/url_lo...
content/browser/url_loader_factory_getter.cc:58:
mojo::MakeRequest(&appcache_factory_), appcache_service.get(), this, -1);
nit: i think the convention is that 0 is ID of browser process (-1 is invalid)

[ananta, 2017-05-25 00:34:07]

On 2017/05/24 15:29:03, jam wrote:
> I'm still curious whether it's beneficial to keep landing a bunch of small cls
> that are not functional? Each one seems to change code done in the previous
cl.
> Can we wait till we have something working end-to-end, even just for main
frame
> requests?
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> File content/browser/appcache/appcache_url_loader_factory.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/60001/content/browser/appcach...
> content/browser/appcache/appcache_url_loader_factory.h:66: int
> renderer_process_id_;
> On 2017/05/24 02:14:03, ananta wrote:
> > On 2017/05/24 00:59:32, michaeln wrote:
> > > If AppCacheURLLoaderFactory is a per-storagepartition object (i think it
> is),
> > I
> > > don't think this datamember belongs here.
> > 
> > Needs to be revisited after kinuko's changes come in. If needed we will have
a
> > new class which handles navigation side code for AppCache with the factory
> only
> > used for subresources.
> 
> Since you're not passing a factoryptr to the renderer yet, what's the need to
> have a renderer process id in this class since it's always -1?
> 
>
https://codereview.chromium.org/2902653002/diff/140001/content/browser/url_lo...
> File content/browser/url_loader_factory_getter.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/140001/content/browser/url_lo...
> content/browser/url_loader_factory_getter.cc:58:
> mojo::MakeRequest(&appcache_factory_), appcache_service.get(), this, -1);
> nit: i think the convention is that 0 is ID of browser process (-1 is invalid)

I thought sending the boilerplate patches first made it easier to focus better
on the big patches coming later on. Please hold off on this review. I am working
on the main response handling at the moment. Will add those changes in over the
next day or two. Thanks

[ananta, 2017-05-27 02:21:52]

Description was changed from

==========
Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
the URL.

The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
unique_ptr for the ResourceRequest. We now hold
on to a copy of the request. The CreateLoaderAndStart() method on the factory
gives us a copy of the request.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

We still have the global factory for browser side loads at the moment. That will
change
once we integrate with kinukos's patch when it lands.

Changes in this patch include the following:
1. Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
   the URL.
   The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
   with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
   This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
   renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
   would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

2. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

[ananta, 2017-05-27 02:25:34]

We have main and subframe AppCache requests now working. Please note that the
AppCache factory is still used in this patchset from the browser. Plan is to
integrate with kinuko's patch when it lands. So a portion of this patch will
change.

[ananta, 2017-05-27 02:28:01]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

We still have the global factory for browser side loads at the moment. That will
change
once we integrate with kinukos's patch when it lands.

Changes in this patch include the following:
1. Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
   the URL.
   The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
   with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
   This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
   renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
   would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

2. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

We still have the global factory for browser side loads at the moment. That will
change
once we integrate with kinukos's patch when it lands.

Changes in this patch include the following:
1. Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
   the URL.
   The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
   with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
   This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
   renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
   would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

2. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

[ananta, 2017-05-27 02:28:24]

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

[commit-bot: I haz the power, 2017-05-27 02:28:39]

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-27 02:36:21]

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

[commit-bot: I haz the power, 2017-05-27 02:36:22]

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

[kinuko, 2017-05-29 14:58:40]

On 2017/05/27 02:25:34, ananta wrote:
> We have main and subframe AppCache requests now working. Please note that the
> AppCache factory is still used in this patchset from the browser. Plan is to
> integrate with kinuko's patch when it lands. So a portion of this patch will
> change.

Fyi, now the other patch's landed!

[ananta, 2017-05-31 21:04:43]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

We still have the global factory for browser side loads at the moment. That will
change
once we integrate with kinukos's patch when it lands.

Changes in this patch include the following:
1. Pass the precreated AppCacheHost pointer to the AppCacheURLLoaderFactory by
setting it before loading
   the URL.
   The AppCacheRequestHandler class needs the AppCacheHost pointer to function
correctly. The host associates a cache
   with a document and so on. We pass the precreated AppCacheHost to the factory
via an explicit static method SetAppCacheHost()
   This is done in the NavigationURLLoaderService before initiating the load for
the frame URL. The other change is to add the
   renderer_process_id parameter to the creator function for the
AppCacheURLLoaderFactory. The plan is to create a new factory which
   would be passed to the renderer for subresource requests. The process id
parameter enables us to lookup the AppCacheHost for the document.

2. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

Once this lands we can use the host to create the AppCacheRequestHandler
instance and use it to handle AppCache requests.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. Implement a subclass for the URLLoaderRequestHandler  for AppCache. This
class uses the AppCacheURLLoader private
   class to check if the request can be serviced via the AppCache. If yes we
create the AppCache factory and pass it to
   LoaderFactoryCallback. If not we pass null to it.

2. We call the AppCacheURLLoaderFactory::CreateRequestHandler() function from
NavigationURLLoaderNetworkService::URLLoaderRequestController::Start()
   for AppCache.

3. The AppCacheURLLoader private class defined in appcache_url_loader_factory.cc
provides functionality to invoke the LoaderFactoryCallback with the
   factory or null depending on whether the request can be serviced from the
AppCache. We implement the latter functionality for navigation
   requests only.

4. The AppCacheURLLoaderFactory::CreateLoaderAndStart() mojom call looks up the
AppCache host for the renderer pid for non navigation requests and
   pass the host to the AppCacheURLLoader class. Please note that this
functionality is not fully implemented yet.

5. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

6. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

7. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

8. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

[ananta, 2017-05-31 21:05:52]

This patchset is now integrated with kinuko's change. Works for main and
subframes.

[ananta, 2017-06-01 02:16:20]

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

[commit-bot: I haz the power, 2017-06-01 02:16: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-06-01 02:20:08]

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

[commit-bot: I haz the power, 2017-06-01 02:20:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-06-01 02:49:14]

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

[commit-bot: I haz the power, 2017-06-01 02:49:52]

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-06-01 03:01:29]

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

[commit-bot: I haz the power, 2017-06-01 03:01:30]

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

[ananta, 2017-06-01 05:23:17]

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

[commit-bot: I haz the power, 2017-06-01 05:23:30]

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-06-01 07:20:17]

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

[commit-bot: I haz the power, 2017-06-01 07:20:18]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-01 15:00:46]

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34: // service URL loads
from the AppCache. It is also invoked for navigation
I'm finding it hard to read that this class is both used for 2 purposes. Do you
think it would be simpler if there was one class for each?

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:162: // We probably need
to chain to the next factory instead of always
can you explain this some more to me? Isn't this going to be the case for
subresources only, since frame requests took the other path above?

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:249: bool
IsNavigationRequest() const { return navigation_request_; }
nit: no need for this helper method to look up a member variable since it's only
used in the class itself

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:360: explicit
AppCacheNavigationRequestHandler(
nit: explicit not needed

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.h:35: static void
CreateURLLoaderFactory(mojom::URLLoaderFactoryRequest request,
this isn't called externally anymore, so no need to keep it in the header?

[ananta, 2017-06-01 17:28:01]

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34: // service URL loads
from the AppCache. It is also invoked for navigation
On 2017/06/01 15:00:46, jam wrote:
> I'm finding it hard to read that this class is both used for 2 purposes. Do
you
> think it would be simpler if there was one class for each?

This class holds on the AppCacheRequestHandler class which handles requests and
coordinates reads from the AppCache via the AppCacheJob. Additionally it also
caches the AppCache response/data which would be read before we make a call to
return a factory to the callback. We just bind to this instance for navigation
requests when the client connects to us, instead of creating one (Sub
resources).

We don't want to recreate the AppCacheRequestHandler instance when the client
connects for navigation requests as there is AppCache state which could be
cached there.

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:162: // We probably need
to chain to the next factory instead of always
On 2017/06/01 15:00:46, jam wrote:
> can you explain this some more to me? Isn't this going to be the case for
> subresources only, since frame requests took the other path above?

Yes. I was wondering about chain of factories with the n/w service being the
last one. If AppCache has another factory after it, we should probably invoke
that one?

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:249: bool
IsNavigationRequest() const { return navigation_request_; }
On 2017/06/01 15:00:46, jam wrote:
> nit: no need for this helper method to look up a member variable since it's
only
> used in the class itself

Done.

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:360: explicit
AppCacheNavigationRequestHandler(
On 2017/06/01 15:00:46, jam wrote:
> nit: explicit not needed

Done.

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.h:35: static void
CreateURLLoaderFactory(mojom::URLLoaderFactoryRequest request,
On 2017/06/01 15:00:46, jam wrote:
> this isn't called externally anymore, so no need to keep it in the header?

The static function uses the loader_factory_bindings_ member.

[ananta, 2017-06-01 17:32:03]

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

[commit-bot: I haz the power, 2017-06-01 17:32:26]

Dry run: CQ is trying da patch.

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

[ananta, 2017-06-01 17:37:16]

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34: // service URL loads
from the AppCache. It is also invoked for navigation
On 2017/06/01 17:28:01, ananta wrote:
> On 2017/06/01 15:00:46, jam wrote:
> > I'm finding it hard to read that this class is both used for 2 purposes. Do
> you
> > think it would be simpler if there was one class for each?
> 
> This class holds on the AppCacheRequestHandler class which handles requests
and
> coordinates reads from the AppCache via the AppCacheJob. Additionally it also
> caches the AppCache response/data which would be read before we make a call to
> return a factory to the callback. We just bind to this instance for navigation
> requests when the client connects to us, instead of creating one (Sub
> resources).
> 
> We don't want to recreate the AppCacheRequestHandler instance when the client
> connects for navigation requests as there is AppCache state which could be
> cached there.
> 
> 

During navigation the class calls the callback with the AppCache factory when it
receives the AppCache response header. The AppCache data comes in later. It is
possible that the client connects to the factory to request a load before we
have the data. Makes it easier if we reuse the AppCacheURLLoader instance then
as it has the state.

[commit-bot: I haz the power, 2017-06-01 18:58:37]

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

[commit-bot: I haz the power, 2017-06-01 18:58:38]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-06-02 01:18:00]

cc'ing kinuko so we can keep appcache and sw looking similar where we can

https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:85:
request_.url.GetOrigin()) ==
This test is only valid for navigations, cross-origin subresources can be
appcached.

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34: // service URL loads
from the AppCache. It is also invoked for navigation
On 2017/06/01 17:37:16, ananta wrote:
> On 2017/06/01 17:28:01, ananta wrote:
> > On 2017/06/01 15:00:46, jam wrote:
> > > I'm finding it hard to read that this class is both used for 2 purposes.
Do
> > you
> > > think it would be simpler if there was one class for each?
> > 
> > This class holds on the AppCacheRequestHandler class which handles requests
> and
> > coordinates reads from the AppCache via the AppCacheJob. Additionally it
also
> > caches the AppCache response/data which would be read before we make a call
to
> > return a factory to the callback. We just bind to this instance for
navigation
> > requests when the client connects to us, instead of creating one (Sub
> > resources).
> > 
> > We don't want to recreate the AppCacheRequestHandler instance when the
client
> > connects for navigation requests as there is AppCache state which could be
> > cached there.
> > 
> > 
> 
> During navigation the class calls the callback with the AppCache factory when
it
> receives the AppCache response header. The AppCache data comes in later. It is
> possible that the client connects to the factory to request a load before we
> have the data. Makes it easier if we reuse the AppCacheURLLoader instance then
> as it has the state.

I had some trouble reading thru too and have some suggestions that I think might
help.

First, I think greater separation between sub and main resource loading would
help too. The logic is different and the points of integration are different.
Having two simpler things for each case would be good, at least for now. This CL
can be made smaller and simpler by removing everything related explicitly to
subresource loading. If we choose to smush subresource loading into it in a
subsequent CL we can always still do that.

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

https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:134:
AppCacheURLLoaderFactory::CreateRequestHandler(
I'd vote to maintain as much symmetry with S/W as practical. So I'd vote to
rename/refactor things to match:

AppCacheRequestHandler::InitializeForNavigationNetworkService()

https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:167:
handlers_[handler_index_++]->MaybeCreateLoaderFactory(
This method is a whole lot like AppCacheRequestHandler::MaybeLoadResource(). I
like how this was composed for SW and I'd like to see a similar arrangment for
appcache. I think this derivation makes sense...

class CONTENT_EXPORT AppCacheRequestHandler
    : public base::SupportsUserData::Data,
      public AppCacheHost::Observer,
      public AppCacheServiceImpl::Observer,
      public AppCacheStorage::Delegate,
      public URLLoaderRequestHandler {  <<<< here

So MaybeCreateLoaderFactory() method is a peer of MaybeLoadResource() just like
in SW'land

class CONTENT_EXPORT ServiceWorkerRequestHandler
    : public base::SupportsUserData::Data,
      public URLLoaderRequestHandler {

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:84:
appcache_service_->storage()->usage_map()->find(
this test is only valid for main resource loads, crossorigin subresources can be
cached

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:172: DLOG(WARNING) <<
"Sending error response for URL: " << request_.url;
TODO for this?

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:373: AppCacheURLLoader*
loader = AppCacheURLLoader::CreateForNavigationRequest(
The way this reads, we're creating a resulting loader prior to creating the
loaderfactory and prior to knowing whether or not we're going to actually load
anything from the appcache.

If we can refactor to make it more clear when actual appcache loading starts, i
think that might help. I have some ideas about this, see the comment a couple
down below for appcache_url_loader_job.h

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:126:
delegate_->HandleAppCacheData(buffer_.get(), result);
What causes the reader to read more? We need to call ReadData() again to read
the next buffer worth of stuff, and so on.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:31: class CONTENT_EXPORT
AppCacheURLLoaderJob : public AppCacheJob,
To make fewer moving parts, would it make sense to have this class derive from
mojom::URLLoaderFactory and mojom::URLLoader? Here's the decomposition i have in
mind with fewer moving parts involved.

Parts List...

AppCacheRequestHandler : public URLLoaderRequestHandler;
AppCacheURLLoaderJob : public mojom URLLoaderFactory, public URLLoader;

Here's how they could fit together...

The NavigationURLLoaderNetworkService creates an appcache_handler in its handler
chain and calls its MaybeCreateLoaderFactory() method.

AppCacheRequestHandler->MaybeCreateLoaderFactory() would create a job internally
(% origin not found case). The AppCacheURLLoaderJob would store all the network
service specific stuff (mojom bindings, LoaderFactoryCallback). If the response
is not found, or there is an error reading the response info,
DeliverNetworkResponse() would pass nullptr to the factorycallback function.
Otherwise, the the LoaderFactoryCallback would get called at successful
OnResponseInfoLoaded() time. 


AppCacheURLLoaderJob->CreateLoaderAndStart(). That method would set up the mojo
bindings, start an appcache response reader, and plumb the response info and
data to the urlloader client.


That's a lot fewer parts (2 object instances vs 5) and it maintains the same
division of labor that we have in the URLRequest'land, where 'handlers' mostly
decide what to do and 'jobs' move the bytes around.

(AppCacheURLLoaderRequestHandler
AppCacheRequestHandler)
+
(AppCacheURLLoaderFactory
AppCacheURLLoaderJob
AppCacheURLLoader)

wdyt of collapsing things along these lines?

[ananta, 2017-06-02 01:34:47]

On 2017/06/02 01:18:00, michaeln wrote:
> cc'ing kinuko so we can keep appcache and sw looking similar where we can
> 
>
https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_factory.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.cc:85:
> request_.url.GetOrigin()) ==
> This test is only valid for navigations, cross-origin subresources can be
> appcached.
> 
>
https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_factory.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/240001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.cc:34: // service URL
loads
> from the AppCache. It is also invoked for navigation
> On 2017/06/01 17:37:16, ananta wrote:
> > On 2017/06/01 17:28:01, ananta wrote:
> > > On 2017/06/01 15:00:46, jam wrote:
> > > > I'm finding it hard to read that this class is both used for 2 purposes.
> Do
> > > you
> > > > think it would be simpler if there was one class for each?
> > > 
> > > This class holds on the AppCacheRequestHandler class which handles
requests
> > and
> > > coordinates reads from the AppCache via the AppCacheJob. Additionally it
> also
> > > caches the AppCache response/data which would be read before we make a
call
> to
> > > return a factory to the callback. We just bind to this instance for
> navigation
> > > requests when the client connects to us, instead of creating one (Sub
> > > resources).
> > > 
> > > We don't want to recreate the AppCacheRequestHandler instance when the
> client
> > > connects for navigation requests as there is AppCache state which could be
> > > cached there.
> > > 
> > > 
> > 
> > During navigation the class calls the callback with the AppCache factory
when
> it
> > receives the AppCache response header. The AppCache data comes in later. It
is
> > possible that the client connects to the factory to request a load before we
> > have the data. Makes it easier if we reuse the AppCacheURLLoader instance
then
> > as it has the state.
> 
> I had some trouble reading thru too and have some suggestions that I think
might
> help.
> 
> First, I think greater separation between sub and main resource loading would
> help too. The logic is different and the points of integration are different.
> Having two simpler things for each case would be good, at least for now. This
CL
> can be made smaller and simpler by removing everything related explicitly to
> subresource loading. If we choose to smush subresource loading into it in a
> subsequent CL we can always still do that.
> 
>
https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
> File content/browser/loader/navigation_url_loader_network_service.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
> content/browser/loader/navigation_url_loader_network_service.cc:134:
> AppCacheURLLoaderFactory::CreateRequestHandler(
> I'd vote to maintain as much symmetry with S/W as practical. So I'd vote to
> rename/refactor things to match:
> 
> AppCacheRequestHandler::InitializeForNavigationNetworkService()
> 
>
https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
> content/browser/loader/navigation_url_loader_network_service.cc:167:
> handlers_[handler_index_++]->MaybeCreateLoaderFactory(
> This method is a whole lot like AppCacheRequestHandler::MaybeLoadResource(). I
> like how this was composed for SW and I'd like to see a similar arrangment for
> appcache. I think this derivation makes sense...
> 
> class CONTENT_EXPORT AppCacheRequestHandler
>     : public base::SupportsUserData::Data,
>       public AppCacheHost::Observer,
>       public AppCacheServiceImpl::Observer,
>       public AppCacheStorage::Delegate,
>       public URLLoaderRequestHandler {  <<<< here
> 
> So MaybeCreateLoaderFactory() method is a peer of MaybeLoadResource() just
like
> in SW'land
> 
> class CONTENT_EXPORT ServiceWorkerRequestHandler
>     : public base::SupportsUserData::Data,
>       public URLLoaderRequestHandler {
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_factory.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.cc:84:
> appcache_service_->storage()->usage_map()->find(
> this test is only valid for main resource loads, crossorigin subresources can
be
> cached
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.cc:172: DLOG(WARNING) <<
> "Sending error response for URL: " << request_.url;
> TODO for this?
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.cc:373:
AppCacheURLLoader*
> loader = AppCacheURLLoader::CreateForNavigationRequest(
> The way this reads, we're creating a resulting loader prior to creating the
> loaderfactory and prior to knowing whether or not we're going to actually load
> anything from the appcache.
> 
> If we can refactor to make it more clear when actual appcache loading starts,
i
> think that might help. I have some ideas about this, see the comment a couple
> down below for appcache_url_loader_job.h
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_job.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:126:
> delegate_->HandleAppCacheData(buffer_.get(), result);
> What causes the reader to read more? We need to call ReadData() again to read
> the next buffer worth of stuff, and so on.
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_job.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:31: class CONTENT_EXPORT
> AppCacheURLLoaderJob : public AppCacheJob,
> To make fewer moving parts, would it make sense to have this class derive from
> mojom::URLLoaderFactory and mojom::URLLoader? Here's the decomposition i have
in
> mind with fewer moving parts involved.
> 
> Parts List...
> 
> AppCacheRequestHandler : public URLLoaderRequestHandler;
> AppCacheURLLoaderJob : public mojom URLLoaderFactory, public URLLoader;
> 
> Here's how they could fit together...
> 
> The NavigationURLLoaderNetworkService creates an appcache_handler in its
handler
> chain and calls its MaybeCreateLoaderFactory() method.
> 
> AppCacheRequestHandler->MaybeCreateLoaderFactory() would create a job
internally
> (% origin not found case). The AppCacheURLLoaderJob would store all the
network
> service specific stuff (mojom bindings, LoaderFactoryCallback). If the
response
> is not found, or there is an error reading the response info,
> DeliverNetworkResponse() would pass nullptr to the factorycallback function.
> Otherwise, the the LoaderFactoryCallback would get called at successful
> OnResponseInfoLoaded() time. 
> 
> 
> AppCacheURLLoaderJob->CreateLoaderAndStart(). That method would set up the
mojo
> bindings, start an appcache response reader, and plumb the response info and
> data to the urlloader client.
> 
> 
> That's a lot fewer parts (2 object instances vs 5) and it maintains the same
> division of labor that we have in the URLRequest'land, where 'handlers' mostly
> decide what to do and 'jobs' move the bytes around.
> 
> (AppCacheURLLoaderRequestHandler
> AppCacheRequestHandler)
> +
> (AppCacheURLLoaderFactory
> AppCacheURLLoaderJob
> AppCacheURLLoader)
> 
> wdyt of collapsing things along these lines?

Thanks michaeln. I had some questions about the lifetime of the
AppCacheRequestHandler class in that hierarchy. The job in the non network
service  world
is created by the handler who holds onto a weakptr to the job. The handler
lifetime is dependent on the URLRequest and so is the job. In the network
service world it gets a touch tricky to manage the lifetimes of the handler/job,
etc.

[ananta, 2017-06-02 01:51:48]

The URLLoaderFactory interface is designed to hand out URL loader instances as
needed. This
enables the instance implementing the URLLoaderFactory mojom to hold on to
state. For e.g. for AppCache
we can probably have the URL of the frame, etc. If the job implements this
interface, then I am not sure
if we can mimic this lifetime.

[ananta, 2017-06-02 03:19:58]

Hi Michael

I will stop by tomorrow to talk through your suggestions before we move forward
on them.

Thanks
Ananta

https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:85:
request_.url.GetOrigin()) ==
On 2017/06/02 01:17:59, michaeln wrote:
> This test is only valid for navigations, cross-origin subresources can be
> appcached.

Done.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:84:
appcache_service_->storage()->usage_map()->find(
On 2017/06/02 01:18:00, michaeln wrote:
> this test is only valid for main resource loads, crossorigin subresources can
be
> cached

Done.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:172: DLOG(WARNING) <<
"Sending error response for URL: " << request_.url;
On 2017/06/02 01:18:00, michaeln wrote:
> TODO for this?

Done.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:126:
delegate_->HandleAppCacheData(buffer_.get(), result);
On 2017/06/02 01:18:00, michaeln wrote:
> What causes the reader to read more? We need to call ReadData() again to read
> the next buffer worth of stuff, and so on.

Thanks for pointing that out. We now control this via a callback passed in here.
The delegate invokes it when it has read the data out of the buffer so we can
continue reading forward.

[ananta, 2017-06-02 03:20:26]

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

[commit-bot: I haz the power, 2017-06-02 03:20:52]

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-06-02 04:31:25]

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

[commit-bot: I haz the power, 2017-06-02 04:31:26]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-06-02 19:21:05]

On 2017/06/02 01:51:48, ananta wrote:
> The URLLoaderFactory interface is designed to hand out URL loader instances as
> needed. This
> enables the instance implementing the URLLoaderFactory mojom to hold on to
> state. For e.g. for AppCache
> we can probably have the URL of the frame, etc. If the job implements this
> interface, then I am not sure
> if we can mimic this lifetime.

In this use case (plznavigation + networkserice), the factory's
CreateLoaderAndStart()interface is spec'd to be called no more than once. We
definitely have all the state needed to satisfy that call. My suggestion is just
one of refactoring to consolidate objects.

Jam tells me the URLLoaderFactory part of the PlzNavigate use case will is
slated to go away. Instead of the URLLoaderRequestHandler producing factory
objects, they'll directly produce URLoaders, taking the middlemand out of the
picture. (fewer moving parts, hooray). In that world, the
URLLoaderRequestHandler maybe produces a URLLoader, those correspond to the two
appcache specific dervitives, AppCacheURLLoaderRequestHandler and
AppCacheURLLoaderJob. This composition makes a lot of sense in from the PlzNav +
URLLoader point of view. They also make sense in terms of existing appcache base
classes: AppCacheRequestHandler and AppCacheJob.  They fit together nicely.

About the lifetime of the 'handler' and 'job' objects, I think it might be fine,
but if not, I'm pretty sure we can smooth out any rough edges on that.

Ownership of the AppCacheURLLoaderJob is xfered from the 'handler' to the strong
binding when the loader really starts, its lifetime is then dictated by the
renderer. If the handler decides we're not going to start it's loader, any 'job'
it had produced can be deleted.

The AppCacheURLLoaderRequestHandler is owned by the
NavigationURLLoaderNetworkServiceonly. I think that's fine,  see the description
of that object lifetime, it will provide the lifetime scoping we need. (It's not
done yet since that class does not yet support intercepting redirect responses
or the final response, but its a wip).

I don't think the 'handler' needs to outlive the 'job', the 'job' has any
dependency on the handler at all, assuming the job owns the
LoaderFactoryCallback (the completion callback for MaybeCreateLoaderFactory).

I'm intentionally not thinking about the subresource loading use case yet,
NavigationURLLoaderNetworkService will not be involved in that.

> I will stop by tomorrow to talk through your suggestions before we move
forward on them.

sure thing!

[michaeln, 2017-06-02 23:47:28]

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:329: if
(service_->storage()->IsInitialized() &&
Please use the storage() method to access the storage object, in exceptional
cases where corruption is discovered, the storage object can be swapped out.
Running documents/workers will continue to use the old storage object until they
are close. Whe latch onto the storage object for the lifetime of the
document/worker and store the reference in the host object (which is scoped to
the lifetime of the document/worker).

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:84:
appcache_service_->storage()->IsInitialized() &&
ditto about using the host's storage() accessor

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:297:
mojo::WriteDataRaw(data_pipe_.producer_handle.get(), buffer->data(),
fyi: jam's CL for the BlobURLLoader looks very instructive about how incremental
URLLoader/URLLoaderClient loading works, see the HeaderComplete() and ReadMore()
and DidRead().

https://chromiumcodereview.appspot.com/2906543002/diff/160001/content/browser...

[ananta, 2017-06-03 01:50:51]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. Implement a subclass for the URLLoaderRequestHandler  for AppCache. This
class uses the AppCacheURLLoader private
   class to check if the request can be serviced via the AppCache. If yes we
create the AppCache factory and pass it to
   LoaderFactoryCallback. If not we pass null to it.

2. We call the AppCacheURLLoaderFactory::CreateRequestHandler() function from
NavigationURLLoaderNetworkService::URLLoaderRequestController::Start()
   for AppCache.

3. The AppCacheURLLoader private class defined in appcache_url_loader_factory.cc
provides functionality to invoke the LoaderFactoryCallback with the
   factory or null depending on whether the request can be serviced from the
AppCache. We implement the latter functionality for navigation
   requests only.

4. The AppCacheURLLoaderFactory::CreateLoaderAndStart() mojom call looks up the
AppCache host for the renderer pid for non navigation requests and
   pass the host to the AppCacheURLLoader class. Please note that this
functionality is not fully implemented yet.

5. Maintain an instance of the AppCacheRequestHandler class in the
AppCacheURLLoader class. We use the handler to handle URL requests.

6. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   We notify the newly added Delegate interface when we have a response/when we
have an error/AppCache data etc. 
   The AppCacheURLLoaderJob::Delegate interface is implemented by the
AppCacheURLLoader class. The data from the AppCache is
   written to a mojo data pipe by the AppCacheURLLoader class which implements
the URLLoader mojom and passes it off to the
   URLLoaderClient.

7. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

8. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. The AppCacheURLLoaderFactory::CreateLoaderAndStart() mojom call uses the
AppCacheURLLoader job which is passed to
   it during creation (Navigation requests) and binds the endpoints to it.

4. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

5. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

6. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

[ananta, 2017-06-03 01:55:56]

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

https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:134:
AppCacheURLLoaderFactory::CreateRequestHandler(
On 2017/06/02 01:17:59, michaeln wrote:
> I'd vote to maintain as much symmetry with S/W as practical. So I'd vote to
> rename/refactor things to match:
> 
> AppCacheRequestHandler::InitializeForNavigationNetworkService()

Done.

https://codereview.chromium.org/2902653002/diff/240001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:167:
handlers_[handler_index_++]->MaybeCreateLoaderFactory(
On 2017/06/02 01:17:59, michaeln wrote:
> This method is a whole lot like AppCacheRequestHandler::MaybeLoadResource(). I
> like how this was composed for SW and I'd like to see a similar arrangment for
> appcache. I think this derivation makes sense...
> 
> class CONTENT_EXPORT AppCacheRequestHandler
>     : public base::SupportsUserData::Data,
>       public AppCacheHost::Observer,
>       public AppCacheServiceImpl::Observer,
>       public AppCacheStorage::Delegate,
>       public URLLoaderRequestHandler {  <<<< here
> 
> So MaybeCreateLoaderFactory() method is a peer of MaybeLoadResource() just
like
> in SW'land
> 
> class CONTENT_EXPORT ServiceWorkerRequestHandler
>     : public base::SupportsUserData::Data,
>       public URLLoaderRequestHandler {

Thanks done. It made the code a lot simpler.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:84:
appcache_service_->storage()->usage_map()->find(
On 2017/06/02 03:19:57, ananta wrote:
> On 2017/06/02 01:18:00, michaeln wrote:
> > this test is only valid for main resource loads, crossorigin subresources
can
> be
> > cached
> 
> Done.

No longer needed with the handler and the job implementing this functionality

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:373: AppCacheURLLoader*
loader = AppCacheURLLoader::CreateForNavigationRequest(
On 2017/06/02 01:18:00, michaeln wrote:
> The way this reads, we're creating a resulting loader prior to creating the
> loaderfactory and prior to knowing whether or not we're going to actually load
> anything from the appcache.
> 
> If we can refactor to make it more clear when actual appcache loading starts,
i
> think that might help. I have some ideas about this, see the comment a couple
> down below for appcache_url_loader_job.h

This class has been removed now. The job implements most of this logic.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:126:
delegate_->HandleAppCacheData(buffer_.get(), result);
On 2017/06/02 03:19:57, ananta wrote:
> On 2017/06/02 01:18:00, michaeln wrote:
> > What causes the reader to read more? We need to call ReadData() again to
read
> > the next buffer worth of stuff, and so on.
> 
> Thanks for pointing that out. We now control this via a callback passed in
here.
> The delegate invokes it when it has read the data out of the buffer so we can
> continue reading forward.

Please look at the updated patch. We continue reading until we get 0 bytes.

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/250001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:31: class CONTENT_EXPORT
AppCacheURLLoaderJob : public AppCacheJob,
On 2017/06/02 01:18:00, michaeln wrote:
> To make fewer moving parts, would it make sense to have this class derive from
> mojom::URLLoaderFactory and mojom::URLLoader? Here's the decomposition i have
in
> mind with fewer moving parts involved.
> 
> Parts List...
> 
> AppCacheRequestHandler : public URLLoaderRequestHandler;
> AppCacheURLLoaderJob : public mojom URLLoaderFactory, public URLLoader;
> 
> Here's how they could fit together...
> 
> The NavigationURLLoaderNetworkService creates an appcache_handler in its
handler
> chain and calls its MaybeCreateLoaderFactory() method.
> 
> AppCacheRequestHandler->MaybeCreateLoaderFactory() would create a job
internally
> (% origin not found case). The AppCacheURLLoaderJob would store all the
network
> service specific stuff (mojom bindings, LoaderFactoryCallback). If the
response
> is not found, or there is an error reading the response info,
> DeliverNetworkResponse() would pass nullptr to the factorycallback function.
> Otherwise, the the LoaderFactoryCallback would get called at successful
> OnResponseInfoLoaded() time. 
> 
> 
> AppCacheURLLoaderJob->CreateLoaderAndStart(). That method would set up the
mojo
> bindings, start an appcache response reader, and plumb the response info and
> data to the urlloader client.
> 
> 
> That's a lot fewer parts (2 object instances vs 5) and it maintains the same
> division of labor that we have in the URLRequest'land, where 'handlers' mostly
> decide what to do and 'jobs' move the bytes around.
> 
> (AppCacheURLLoaderRequestHandler
> AppCacheRequestHandler)
> +
> (AppCacheURLLoaderFactory
> AppCacheURLLoaderJob
> AppCacheURLLoader)
> 
> wdyt of collapsing things along these lines?

The class implements the URLLoader mojom. The factory is still implemented by
the AppCacheURLLoaderFactory class.

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:329: if
(service_->storage()->IsInitialized() &&
On 2017/06/02 23:47:27, michaeln wrote:
> Please use the storage() method to access the storage object, in exceptional
> cases where corruption is discovered, the storage object can be swapped out.
> Running documents/workers will continue to use the old storage object until
they
> are close. Whe latch onto the storage object for the lifetime of the
> document/worker and store the reference in the host object (which is scoped to
> the lifetime of the document/worker).

Done.

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:84:
appcache_service_->storage()->IsInitialized() &&
On 2017/06/02 23:47:27, michaeln wrote:
> ditto about using the host's storage() accessor

Not needed anymore.

https://codereview.chromium.org/2902653002/diff/270001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:297:
mojo::WriteDataRaw(data_pipe_.producer_handle.get(), buffer->data(),
On 2017/06/02 23:47:27, michaeln wrote:
> fyi: jam's CL for the BlobURLLoader looks very instructive about how
incremental
> URLLoader/URLLoaderClient loading works, see the HeaderComplete() and
ReadMore()
> and DidRead().
> 
>
https://chromiumcodereview.appspot.com/2906543002/diff/160001/content/browser...

Thanks for the tip. I will look into that in the upcoming patchsets.

[ananta, 2017-06-03 02:27:12]

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

[commit-bot: I haz the power, 2017-06-03 02:27: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-06-03 02:29:39]

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

[commit-bot: I haz the power, 2017-06-03 02:29:40]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-06-03 02:48:26]

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

[commit-bot: I haz the power, 2017-06-03 02:48:42]

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-06-03 02:56:08]

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

[commit-bot: I haz the power, 2017-06-03 02:56:09]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-06-05 19:34:21]

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

[commit-bot: I haz the power, 2017-06-05 19:34:41]

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-06-05 19:38:55]

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

[commit-bot: I haz the power, 2017-06-05 19:38:56]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[ananta, 2017-06-05 20:07:53]

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

[commit-bot: I haz the power, 2017-06-05 20:08:25]

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-06-05 20:19:51]

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

[commit-bot: I haz the power, 2017-06-05 20:19:53]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-06-05 20:53:17]

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

[commit-bot: I haz the power, 2017-06-05 20:55:00]

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-06-05 21:06:37]

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

[commit-bot: I haz the power, 2017-06-05 21:06:38]

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

[ananta, 2017-06-05 22:44:30]

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

[commit-bot: I haz the power, 2017-06-05 22:45:09]

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-06-05 22:56:27]

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

[commit-bot: I haz the power, 2017-06-05 22:56:29]

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

[ananta, 2017-06-06 02:31:40]

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

[commit-bot: I haz the power, 2017-06-06 02:32:13]

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-06-06 02:43:35]

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

[commit-bot: I haz the power, 2017-06-06 02:43:36]

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

[ananta, 2017-06-06 02:59:12]

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

[commit-bot: I haz the power, 2017-06-06 02:59:33]

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-06-06 04:12:44]

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

[commit-bot: I haz the power, 2017-06-06 04:12:45]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-06-06 08:07:29]

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

[kinuko, 2017-06-06 08:07:31]

Thanks michealn for driving this review, I just skimmed through some bits but I
think it's getting very close to what I had imagined with the
URLLoaderRequestHandler interface.

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:556: LoaderFactoryCallback
callback) {
(As michael wrote we're likely changing this part, but feel free to land this
first if this one gets ready first)

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:81: URLLoaderFactoryGetter*
url_loader_factory_getter);
Yeah thanks for making this look similar to SW cases, that improves code
readability / consistency.

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:83: 
DCHECK(!binding_); ?

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:85: this,
std::move(url_loader_request)));
nit: I think we can also do something like:

 binding_(this)

in ctor and

 DCHECK(!binding_.is_bound());
 binding_.Bind(std::move(url_loader_request));

here

[ananta, 2017-06-06 19:42:42]

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

[commit-bot: I haz the power, 2017-06-06 19:43:27]

Dry run: CQ is trying da patch.

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

[ananta, 2017-06-06 20:49:16]

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:556: LoaderFactoryCallback
callback) {
On 2017/06/06 08:07:30, kinuko wrote:
> (As michael wrote we're likely changing this part, but feel free to land this
> first if this one gets ready first)

Will keep an eye out :)

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:81: URLLoaderFactoryGetter*
url_loader_factory_getter);
On 2017/06/06 08:07:30, kinuko wrote:
> Yeah thanks for making this look similar to SW cases, that improves code
> readability / consistency.

+1

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:83: 
On 2017/06/06 08:07:30, kinuko wrote:
> DCHECK(!binding_); ?

Not needed now.

https://codereview.chromium.org/2902653002/diff/490001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:85: this,
std::move(url_loader_request)));
On 2017/06/06 08:07:30, kinuko wrote:
> nit: I think we can also do something like:
> 
>  binding_(this)
> 
> in ctor and
> 
>  DCHECK(!binding_.is_bound());
>  binding_.Bind(std::move(url_loader_request));
> 
> here

Done.

[commit-bot: I haz the power, 2017-06-06 20:54:31]

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

[commit-bot: I haz the power, 2017-06-06 20:54:33]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-06-06 23:43:23]

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

[commit-bot: I haz the power, 2017-06-06 23:43:43]

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-06-07 01:23:48]

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

[commit-bot: I haz the power, 2017-06-07 01:23:49]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-07 01:42:17]

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:238:
std::unique_ptr<AppCacheRequestHandler> handler =
I thought an earlier patchset didn't create an appcache handler if that origin
didn't have any appcaches associated, or am I misremembering?

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:239:
appcache_handle_core->host()->CreateRequestHandler(
can't this return null?

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:217: // binds to it
(Serviced via AppCache). If the request cannot be serviced via
nit: just to make things a bit clearer, s/serviced/handled by, since service
means so many things now :)

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:131: new
net::IOBuffer(static_cast<size_t>(info_->response_data_size()));
what if the response is really big, this would end up creating a buffer that
size? also what if the data pipe was smaller than this?

I think using the helper class in content/common would fix these, so should use
these from the start.

[michaeln, 2017-06-07 02:53:46]

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:238:
std::unique_ptr<AppCacheRequestHandler> handler =
On 2017/06/07 01:42:17, jam wrote:
> I thought an earlier patchset didn't create an appcache handler if that origin
> didn't have any appcaches associated, or am I misremembering?

to support redirects to appcached pages, we can't do that check at this point

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:239:
appcache_handle_core->host()->CreateRequestHandler(
On 2017/06/07 01:42:17, jam wrote:
> can't this return null?

yes, it can

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:291:
std::move(loader_factory_callback_).Run(appcache_factory_.get());
Do we still need to retain the appcache_factory_ instance beyond this point? It
looks like kinuko's CL to get rid of the factory is good to go, this should go
away after rebasing.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:366:
service_->storage()->usage_map()->find(request_->GetURL().GetOrigin()) ==
@jam, this is where the origin test goes

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:226:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
Why is this needed? Is this due to the lifetime issue you mentioned? where the
appcacheloader needed to keep it alive due to the associated message pipe stuff?

If so, this seems like something that should be taken care of at a higher level,
otherwise each possible class of loader has to do this. If this doesn't get
resolved by not using associated loaders, I'd vote to hoist responsibility for
this into the NavigationURLLoaderNetworkService class.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:44:
url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(
I think we want to defer to the network by calling the LoaderFactoryCallback
with nullptr so the NavigationURLLoaderNetworkService class tries the next
'handler' in its list.

To do that, we probably should defer calling the LoaderCallback until the
ResponseInfo has been successfully loaded.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:55: // Add support to send
out an error response.
see BlobURLLoader::NotifyCompleted() for an example of how to do that

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:122: if (response_info) {
We shouldn't consume the LoaderRequest until here, I think we want to call
LoaderCallback(this) right at this point.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:139:
DeliverNetworkResponse();
We should call LoaderCallback(nullptr) here to go on to the next in the handler
chain.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:162:
DeliverNetworkResponse();
At this point, it's too late to fallback to the network, we've already sent a
response head and now have failed to load the body, we need to call an internal
NotifyFailure method that signals the client about the failure. The
BlobURLLoaderFactory has a good example in NotifyComplete.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:173:
mojo::WriteDataRaw(data_pipe_.producer_handle.get(), buffer_->data(),
We're assuming a MOJO_RESULT_OK here, if the resource is large, we'll definitely
run into MOJO_RESULT_SHOULD_WAIT. If possible, it'd be nice to get the data
streaming aspects taken care of in this CL, where we read some, write some, read
some more. But if not, We could put a CHECK(MOJO_RESULT_OK) to check the
assumption.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:197:
http_info->headers->EnumerateHeaderLines(&it, &name, &value);) {
HttpResponseHeaders is a refcounted object, would it work to refer to the same
object here instead of making a complete copy?

reasons_head.headers = http_info->headers;

[ananta, 2017-06-07 20:57:17]

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:238:
std::unique_ptr<AppCacheRequestHandler> handler =
On 2017/06/07 01:42:17, jam wrote:
> I thought an earlier patchset didn't create an appcache handler if that origin
> didn't have any appcaches associated, or am I misremembering?

You are correct there. However we decided to go with the handler based approach
for consistency with ServiceWorker.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:239:
appcache_handle_core->host()->CreateRequestHandler(
On 2017/06/07 01:42:17, jam wrote:
> can't this return null?
> 

yes

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:291:
std::move(loader_factory_callback_).Run(appcache_factory_.get());
On 2017/06/07 02:53:45, michaeln wrote:
> Do we still need to retain the appcache_factory_ instance beyond this point?
It
> looks like kinuko's CL to get rid of the factory is good to go, this should go
> away after rebasing.

Will look into that. I am not sure her patch landed which gets rid of the
factory. For now we need this.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:217: // binds to it
(Serviced via AppCache). If the request cannot be serviced via
On 2017/06/07 01:42:17, jam wrote:
> nit: just to make things a bit clearer, s/serviced/handled by, since service
> means so many things now :)

Done.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:226:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
On 2017/06/07 02:53:45, michaeln wrote:
> Why is this needed? Is this due to the lifetime issue you mentioned? where the
> appcacheloader needed to keep it alive due to the associated message pipe
stuff?
> 
> If so, this seems like something that should be taken care of at a higher
level,
> otherwise each possible class of loader has to do this. If this doesn't get
> resolved by not using associated loaders, I'd vote to hoist responsibility for
> this into the NavigationURLLoaderNetworkService class.

This is currently being used to invoke the default service.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:131: new
net::IOBuffer(static_cast<size_t>(info_->response_data_size()));
On 2017/06/07 01:42:17, jam wrote:
> what if the response is really big, this would end up creating a buffer that
> size? also what if the data pipe was smaller than this?
> 
> I think using the helper class in content/common would fix these, so should
use
> these from the start.

Done.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:139:
DeliverNetworkResponse();
On 2017/06/07 02:53:45, michaeln wrote:
> We should call LoaderCallback(nullptr) here to go on to the next in the
handler
> chain.

I don't think we can do that at this point. It is a single use callback.
Added a TODO here to look into a good way to handle this going forward

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:162:
DeliverNetworkResponse();
On 2017/06/07 02:53:46, michaeln wrote:
> At this point, it's too late to fallback to the network, we've already sent a
> response head and now have failed to load the body, we need to call an
internal
> NotifyFailure method that signals the client about the failure. The
> BlobURLLoaderFactory has a good example in NotifyComplete.

Added a call to oncomplete here. Will work on it in the next patchsets.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:173:
mojo::WriteDataRaw(data_pipe_.producer_handle.get(), buffer_->data(),
On 2017/06/07 02:53:46, michaeln wrote:
> We're assuming a MOJO_RESULT_OK here, if the resource is large, we'll
definitely
> run into MOJO_RESULT_SHOULD_WAIT. If possible, it'd be nice to get the data
> streaming aspects taken care of in this CL, where we read some, write some,
read
> some more. But if not, We could put a CHECK(MOJO_RESULT_OK) to check the
> assumption.

Code has changed to async

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:197:
http_info->headers->EnumerateHeaderLines(&it, &name, &value);) {
On 2017/06/07 02:53:46, michaeln wrote:
> HttpResponseHeaders is a refcounted object, would it work to refer to the same
> object here instead of making a complete copy?
> 
> reasons_head.headers = http_info->headers;

Thanks done.

[ananta, 2017-06-07 20:58:07]

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

[commit-bot: I haz the power, 2017-06-07 21:01:28]

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-06-07 21:02:12]

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

[commit-bot: I haz the power, 2017-06-07 21:02:13]

Dry run: CQ cannot get SignCLA result. Please try later.

[ananta, 2017-06-07 21:35:22]

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

[commit-bot: I haz the power, 2017-06-07 21:35:56]

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-06-07 21:56:30]

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

[commit-bot: I haz the power, 2017-06-07 21:56:31]

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

[michaeln, 2017-06-08 02:54:01]

i think i see how we could put more of the UrlLoader specific logic inside of
the "job" and keep the "handler" more agnostic

i didn't look at the async read/write parts yet

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:291:
std::move(loader_factory_callback_).Run(appcache_factory_.get());
On 2017/06/07 20:57:16, ananta wrote:
> On 2017/06/07 02:53:45, michaeln wrote:
> > Do we still need to retain the appcache_factory_ instance beyond this point?
> It
> > looks like kinuko's CL to get rid of the factory is good to go, this should
go
> > away after rebasing.
> 
> Will look into that. I am not sure her patch landed which gets rid of the
> factory. For now we need this.

This logic also could be hoisted into the AppCacheUrlLoaderJob class, so the
callback can be deferred further, until the response info load completes.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:311: if
(navigation_request_job_.get()) {
This bit of logic to call the loadercallback could go inside
AppCacheUrlLoaderJob::DeliverNetworkResponse(), that would keep this class more
agnostic about when loading system was being used.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:563:
loader_factory_callback_ = std::move(callback);
This datamember could go inside the AppCacheUrlLoaderJob, giving it the
responsibility to call it appropriately with nullptr or itself.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:139:
DeliverNetworkResponse();
> I don't think we can do that at this point. It is a single use callback.
> Added a TODO here to look into a good way to handle this going forward

I'm suggesting that we should defer calling the LoaderCallback until we're
inside this method, if we read good response info, LoaderCallback(this), and if
we don't get good info, LoaderCallback(nullptr). Then kinuko's class will invoke
the next in the chain.

The DeliverNetworkResponse method can call LoaderCallback(nullptr) and the
OnResponseInfoLoaded can call LoaderCallback(this) when we get good info. Those
are the two primary callsites for the LoaderCallback. We'd need the third in
AppCacheRequestHandler::MaybeCreateLoaderFactory for when we don't construct a
job at all.

[ananta, 2017-06-08 05:58:19]

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

[commit-bot: I haz the power, 2017-06-08 05:58:32]

Dry run: CQ is trying da patch.

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

[ananta, 2017-06-08 05:58:40]

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:291:
std::move(loader_factory_callback_).Run(appcache_factory_.get());
On 2017/06/08 02:54:00, michaeln wrote:
> On 2017/06/07 20:57:16, ananta wrote:
> > On 2017/06/07 02:53:45, michaeln wrote:
> > > Do we still need to retain the appcache_factory_ instance beyond this
point?
> > It
> > > looks like kinuko's CL to get rid of the factory is good to go, this
should
> go
> > > away after rebasing.
> > 
> > Will look into that. I am not sure her patch landed which gets rid of the
> > factory. For now we need this.
> 
> This logic also could be hoisted into the AppCacheUrlLoaderJob class, so the
> callback can be deferred further, until the response info load completes.

Done.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:311: if
(navigation_request_job_.get()) {
On 2017/06/08 02:54:00, michaeln wrote:
> This bit of logic to call the loadercallback could go inside
> AppCacheUrlLoaderJob::DeliverNetworkResponse(), that would keep this class
more
> agnostic about when loading system was being used.

Done.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:563:
loader_factory_callback_ = std::move(callback);
On 2017/06/08 02:54:00, michaeln wrote:
> This datamember could go inside the AppCacheUrlLoaderJob, giving it the
> responsibility to call it appropriately with nullptr or itself.

Done.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:44:
url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(
On 2017/06/07 02:53:46, michaeln wrote:
> I think we want to defer to the network by calling the LoaderFactoryCallback
> with nullptr so the NavigationURLLoaderNetworkService class tries the next
> 'handler' in its list.
> 
> To do that, we probably should defer calling the LoaderCallback until the
> ResponseInfo has been successfully loaded.

I added call to the loader callback here. We return after invoking the callback
if
we have one. I left the factory stuff here for now. Will revisit this in the
next patch where we start looking at subresources

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:55: // Add support to send
out an error response.
On 2017/06/07 02:53:46, michaeln wrote:
> see BlobURLLoader::NotifyCompleted() for an example of how to do that

Added a small implementation of NotifyCompleted(). Needs more work for
generating an error response etc. Will do that in a later patch. Added TODOs to
that effect.

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:122: if (response_info) {
On 2017/06/07 02:53:46, michaeln wrote:
> We shouldn't consume the LoaderRequest until here, I think we want to call
> LoaderCallback(this) right at this point.

Thanks. makes sense. done

https://codereview.chromium.org/2902653002/diff/550001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:139:
DeliverNetworkResponse();
On 2017/06/08 02:54:01, michaeln wrote:
> > I don't think we can do that at this point. It is a single use callback.
> > Added a TODO here to look into a good way to handle this going forward
> 
> I'm suggesting that we should defer calling the LoaderCallback until we're
> inside this method, if we read good response info, LoaderCallback(this), and
if
> we don't get good info, LoaderCallback(nullptr). Then kinuko's class will
invoke
> the next in the chain.
> 
> The DeliverNetworkResponse method can call LoaderCallback(nullptr) and the
> OnResponseInfoLoaded can call LoaderCallback(this) when we get good info.
Those
> are the two primary callsites for the LoaderCallback. We'd need the third in
> AppCacheRequestHandler::MaybeCreateLoaderFactory for when we don't construct a
> job at all.

Done.

[commit-bot: I haz the power, 2017-06-08 06:06:24]

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

[commit-bot: I haz the power, 2017-06-08 06:06:25]

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...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kinuko, 2017-06-08 10:31:21]

(nits only, thanks for rebasing)

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:548: std::move(callback));
nit: for readability maybe could we have a comment like: "MaybeLoadMainResource
will invoke navigation_request_job_'s methods asyncly via
AppCacheStorage::Delegate." ?

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:221: // In network service
world used to to get the default network service.
nit: used to to -> used to

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34:
mojom::URLLoaderFactoryPtr AppCacheURLLoaderFactory::CreateURLLoaderFactory(
Is this still used somewhere?

[ananta, 2017-06-08 13:48:56]

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.cc (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:548: std::move(callback));
On 2017/06/08 10:31:21, kinuko wrote:
> nit: for readability maybe could we have a comment like:
"MaybeLoadMainResource
> will invoke navigation_request_job_'s methods asyncly via
> AppCacheStorage::Delegate." ?

Done.

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:221: // In network service
world used to to get the default network service.
On 2017/06/08 10:31:21, kinuko wrote:
> nit: used to to -> used to

Done.

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.cc (right):

https://codereview.chromium.org/2902653002/diff/610001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.cc:34:
mojom::URLLoaderFactoryPtr AppCacheURLLoaderFactory::CreateURLLoaderFactory(
On 2017/06/08 10:31:21, kinuko wrote:
> Is this still used somewhere?

Not at the moment. Will be used in some capacity when we create it for
subresources.

[ananta, 2017-06-08 13:49:46]

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

[commit-bot: I haz the power, 2017-06-08 13:50:10]

Dry run: CQ is trying da patch.

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

[ananta, 2017-06-08 14:01:04]

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

[commit-bot: I haz the power, 2017-06-08 14:01:24]

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-06-08 15:22:20]

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

[commit-bot: I haz the power, 2017-06-08 15:22:22]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-06-09 01:48:30]

I think this is very close, please take a close look at BlobURLLoader and try to
sync this up as closely as practical. Ultimately, we should probably share some
of the async Read(iobuf, callback) to MojoPipe, ReadMore(), NotififyComplete(),
logic.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:222:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
why do we need this?

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.h:20: class
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
This isn't used yet, can we remove the files from this CL? I think its premature
for this class since we don't yet know how subresource loading will be put
together.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:52:
url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(
We don't yet know if there will be a chain of handlers for this use case, it
could be composed similary with a LoaderCallback arrangement, i'd suggest
waiting for a later CL to add subresource loading support and eliminating all of
the tentative branching logic. If possible, it'd be nice if the same arrangement
with this class calling the LoaderCallback(nullptr) could work, fewer code
paths.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:136: if
(!loader_callback_.is_null()) {
in our current use case, navigation, this is never expected to be null

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:150:
base::Bind(&AppCacheURLLoaderJob::OnResponseBodyStreamReady,
is it possible/expected for OnResponseBodyStreamReady to be called prior to
return from the Watch() method, or shortly after return, if the pipe is in a
writable state at the time Watch() is called?

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:161: // next handler in the
chain?.
The TODO comment seems stale or misplaced, for navigations the answer is yes,
and this is a question for the DeliverNetworkResponse() method.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:183:
manifest_url_.GetOrigin());
you could early return here and avoid the if (result > 0) down below

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:247: // TODO: How should
this be communicated to our client?
Probably NotifyComplete(ERR)? As coded will progresss stop and the request will
hang?

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:255: buffer_ = new
NetToMojoIOBuffer(pending_write_.get());
Is the buffer_ data member is needed? It's only accessed in this method and the
reader retains its own scoped_ref<>. So its not clear to me it is needed?

I don't know much about mojo datapipe usage, so I hope jam takes a look at this
part. There's a lot of overlap with BlobURLLoader.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:263: // TODO: Handle a bad
|result| value.
ditto probably NotifyComplete(ERR)

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:272: // That requires HTTP
header generation etc.
I think we only need http header generation and a call to OnReceiveResponse when
we're called thru the DeleiverErrorResponse() callsite, the other callsites
happen after headers have already been sent.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:40: int request_id,
the id's aren't needed yet

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:100: AppCacheStorage*
storage_;
this rawptr could become dangling, we need to be careful here

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:136:
mojom::URLLoaderAssociatedPtr url_loader_network_;
since we don't need this yet, i'd vote to remove it?

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:140: int routing_id_;
ditto

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:146:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
ditto

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_request.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_request.cc:62: : request_(request)
{}
is this copy expensive? i see other loaders make a copy too

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

https://codereview.chromium.org/2902653002/diff/650001/content/browser/loader...
content/browser/loader/navigation_url_loader_network_service.cc:170:
std::unique_ptr<URLLoaderRequestHandler> appcache_handler =
nice how this just slots in here

[michaeln, 2017-06-09 01:49:35]

also, please update CL desc to match the code

[michaeln, 2017-06-09 02:06:18]

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:35:
storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);
Unfortunately, the rawptr is a problem

We're going to have to implement the AppCacheServiceImpl::Observer interface,
for OnServiceReinitialized, or access it thru a WeakPtr.

Also we have to call storage->CancelDelegateCallbacks(this) if we're destructed
prior to that method completing.

[ananta, 2017-06-09 04:46:57]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. The AppCacheURLLoaderFactory::CreateLoaderAndStart() mojom call uses the
AppCacheURLLoader job which is passed to
   it during creation (Navigation requests) and binds the endpoints to it.

4. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

5. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

6. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

[ananta, 2017-06-09 04:48:19]

I will take a look at moving the common code for asynchronous reading and
writing to the mojo data pipe to a helper class in a later patchset. That will
affect BlobURLLoader as well.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_request_handler.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.h:222:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
On 2017/06/09 01:48:29, michaeln wrote:
> why do we need this?

Removed now. I was planning to use this for subresources. We can cross that
bridge in the next patchset.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_factory.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_factory.h:20: class
AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
On 2017/06/09 01:48:29, michaeln wrote:
> This isn't used yet, can we remove the files from this CL? I think its
premature
> for this class since we don't yet know how subresource loading will be put
> together.

The URLLoader mojom in the appcache_url_loader_factory.cc file is no longer
needed. If it is ok with you, I would like to land this in the current fashion
and address it in the next patchset.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:35:
storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);
On 2017/06/09 02:06:18, michaeln wrote:
> Unfortunately, the rawptr is a problem
> 
> We're going to have to implement the AppCacheServiceImpl::Observer interface,
> for OnServiceReinitialized, or access it thru a WeakPtr.
> 
> Also we have to call storage->CancelDelegateCallbacks(this) if we're
destructed
> prior to that method completing.

I added a call to CancelDelegateCallbacks in the dtor. The URLRequestJob also
has the same problem  from what I can see. If it is ok we can revisit this in a
later patchset. Added a TODO here.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:52:
url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(
On 2017/06/09 01:48:29, michaeln wrote:
> We don't yet know if there will be a chain of handlers for this use case, it
> could be composed similary with a LoaderCallback arrangement, i'd suggest
> waiting for a later CL to add subresource loading support and eliminating all
of
> the tentative branching logic. If possible, it'd be nice if the same
arrangement
> with this class calling the LoaderCallback(nullptr) could work, fewer code
> paths. 

Removed

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:136: if
(!loader_callback_.is_null()) {
On 2017/06/09 01:48:30, michaeln wrote:
> in our current use case, navigation, this is never expected to be null

Replaced with a DCHECK

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:150:
base::Bind(&AppCacheURLLoaderJob::OnResponseBodyStreamReady,
On 2017/06/09 01:48:29, michaeln wrote:
> is it possible/expected for OnResponseBodyStreamReady to be called prior to
> return from the Watch() method, or shortly after return, if the pipe is in a
> writable state at the time Watch() is called?

It is on the same thread?. Unless I am missing something here.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:161: // next handler in the
chain?.
On 2017/06/09 01:48:29, michaeln wrote:
> The TODO comment seems stale or misplaced, for navigations the answer is yes,
> and this is a question for the DeliverNetworkResponse() method.
Thanks. Removed the comment.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:183:
manifest_url_.GetOrigin());
On 2017/06/09 01:48:30, michaeln wrote:
> you could early return here and avoid the if (result > 0) down below

Done.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:247: // TODO: How should
this be communicated to our client?
On 2017/06/09 01:48:29, michaeln wrote:
> Probably NotifyComplete(ERR)? As coded will progresss stop and the request
will
> hang?

Added a call to NotifyCompleted here.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:255: buffer_ = new
NetToMojoIOBuffer(pending_write_.get());
On 2017/06/09 01:48:29, michaeln wrote:
> Is the buffer_ data member is needed? It's only accessed in this method and
the
> reader retains its own scoped_ref<>. So its not clear to me it is needed?
> 
> I don't know much about mojo datapipe usage, so I hope jam takes a look at
this
> part. There's a lot of overlap with BlobURLLoader.

The buffer_ member should not be needed I think. Removed it. About the overlap,
I plan to add a separate patchset to move the common code out into a helper
class.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:263: // TODO: Handle a bad
|result| value.
On 2017/06/09 01:48:30, michaeln wrote:
> ditto probably NotifyComplete(ERR)

Done.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:272: // That requires HTTP
header generation etc.
On 2017/06/09 01:48:29, michaeln wrote:
> I think we only need http header generation and a call to OnReceiveResponse
when
> we're called thru the DeleiverErrorResponse() callsite, the other callsites
> happen after headers have already been sent.

Thanks. moved this comment to the DeliverErrorResponse function.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:40: int request_id,
On 2017/06/09 01:48:30, michaeln wrote:
> the id's aren't needed yet

Removed

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:100: AppCacheStorage*
storage_;
On 2017/06/09 01:48:30, michaeln wrote:
> this rawptr could become dangling, we need to be careful here

Set it to nullptr in DeliverErrorResponse() like the URLRequestJob

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:136:
mojom::URLLoaderAssociatedPtr url_loader_network_;
On 2017/06/09 01:48:30, michaeln wrote:
> since we don't need this yet, i'd vote to remove it?

Done.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:140: int routing_id_;
On 2017/06/09 01:48:30, michaeln wrote:
> ditto

Done.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:146:
scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
On 2017/06/09 01:48:30, michaeln wrote:
> ditto

Done.

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_request.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_request.cc:62: : request_(request)
{}
On 2017/06/09 01:48:30, michaeln wrote:
> is this copy expensive? i see other loaders make a copy too

Shouldn't be too expensive I think. This is a pattern in the n/w service code.

[ananta, 2017-06-09 04:52:01]

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

[commit-bot: I haz the power, 2017-06-09 04:52:15]

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-06-09 06:05:24]

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

[commit-bot: I haz the power, 2017-06-09 06:05:26]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-06-09 14:31:17]

On 2017/06/09 04:48:19, ananta wrote:
> I will take a look at moving the common code for asynchronous reading and
> writing to the mojo data pipe to a helper class in a later patchset. That will
> affect BlobURLLoader as well.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> File content/browser/appcache/appcache_request_handler.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_request_handler.h:222:
> scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
> On 2017/06/09 01:48:29, michaeln wrote:
> > why do we need this?
> 
> Removed now. I was planning to use this for subresources. We can cross that
> bridge in the next patchset.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_factory.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_factory.h:20: class
> AppCacheURLLoaderFactory : public mojom::URLLoaderFactory {
> On 2017/06/09 01:48:29, michaeln wrote:
> > This isn't used yet, can we remove the files from this CL? I think its
> premature
> > for this class since we don't yet know how subresource loading will be put
> > together.
> 
> The URLLoader mojom in the appcache_url_loader_factory.cc file is no longer
> needed. If it is ok with you, I would like to land this in the current fashion
> and address it in the next patchset.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_job.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:35:
> storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);
> On 2017/06/09 02:06:18, michaeln wrote:
> > Unfortunately, the rawptr is a problem
> > 
> > We're going to have to implement the AppCacheServiceImpl::Observer
interface,
> > for OnServiceReinitialized, or access it thru a WeakPtr.
> > 
> > Also we have to call storage->CancelDelegateCallbacks(this) if we're
> destructed
> > prior to that method completing.
> 
> I added a call to CancelDelegateCallbacks in the dtor. The URLRequestJob also
> has the same problem  from what I can see. If it is ok we can revisit this in
a
> later patchset. Added a TODO here.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:52:
> url_loader_factory_getter_->GetNetworkFactory()->get()->CreateLoaderAndStart(
> On 2017/06/09 01:48:29, michaeln wrote:
> > We don't yet know if there will be a chain of handlers for this use case, it
> > could be composed similary with a LoaderCallback arrangement, i'd suggest
> > waiting for a later CL to add subresource loading support and eliminating
all
> of
> > the tentative branching logic. If possible, it'd be nice if the same
> arrangement
> > with this class calling the LoaderCallback(nullptr) could work, fewer code
> > paths. 
> 
> Removed
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:136: if
> (!loader_callback_.is_null()) {
> On 2017/06/09 01:48:30, michaeln wrote:
> > in our current use case, navigation, this is never expected to be null
> 
> Replaced with a DCHECK
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:150:
> base::Bind(&AppCacheURLLoaderJob::OnResponseBodyStreamReady,
> On 2017/06/09 01:48:29, michaeln wrote:
> > is it possible/expected for OnResponseBodyStreamReady to be called prior to
> > return from the Watch() method, or shortly after return, if the pipe is in a
> > writable state at the time Watch() is called?
> 
> It is on the same thread?. Unless I am missing something here.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:161: // next handler in
the
> chain?.
> On 2017/06/09 01:48:29, michaeln wrote:
> > The TODO comment seems stale or misplaced, for navigations the answer is
yes,
> > and this is a question for the DeliverNetworkResponse() method.
> Thanks. Removed the comment.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:183:
> manifest_url_.GetOrigin());
> On 2017/06/09 01:48:30, michaeln wrote:
> > you could early return here and avoid the if (result > 0) down below
> 
> Done.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:247: // TODO: How should
> this be communicated to our client?
> On 2017/06/09 01:48:29, michaeln wrote:
> > Probably NotifyComplete(ERR)? As coded will progresss stop and the request
> will
> > hang?
> 
> Added a call to NotifyCompleted here.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:255: buffer_ = new
> NetToMojoIOBuffer(pending_write_.get());
> On 2017/06/09 01:48:29, michaeln wrote:
> > Is the buffer_ data member is needed? It's only accessed in this method and
> the
> > reader retains its own scoped_ref<>. So its not clear to me it is needed?
> > 
> > I don't know much about mojo datapipe usage, so I hope jam takes a look at
> this
> > part. There's a lot of overlap with BlobURLLoader.
> 
> The buffer_ member should not be needed I think. Removed it. About the
overlap,
> I plan to add a separate patchset to move the common code out into a helper
> class.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:263: // TODO: Handle a bad
> |result| value.
> On 2017/06/09 01:48:30, michaeln wrote:
> > ditto probably NotifyComplete(ERR)
> 
> Done.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.cc:272: // That requires HTTP
> header generation etc.
> On 2017/06/09 01:48:29, michaeln wrote:
> > I think we only need http header generation and a call to OnReceiveResponse
> when
> > we're called thru the DeleiverErrorResponse() callsite, the other callsites
> > happen after headers have already been sent.
> 
> Thanks. moved this comment to the DeliverErrorResponse function.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_job.h (right):
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:40: int request_id,
> On 2017/06/09 01:48:30, michaeln wrote:
> > the id's aren't needed yet
> 
> Removed
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:100: AppCacheStorage*
> storage_;
> On 2017/06/09 01:48:30, michaeln wrote:
> > this rawptr could become dangling, we need to be careful here
> 
> Set it to nullptr in DeliverErrorResponse() like the URLRequestJob
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:136:
> mojom::URLLoaderAssociatedPtr url_loader_network_;
> On 2017/06/09 01:48:30, michaeln wrote:
> > since we don't need this yet, i'd vote to remove it?
> 
> Done.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:140: int routing_id_;
> On 2017/06/09 01:48:30, michaeln wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_job.h:146:
> scoped_refptr<URLLoaderFactoryGetter> url_loader_factory_getter_;
> On 2017/06/09 01:48:30, michaeln wrote:
> > ditto
> 
> Done.
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> File content/browser/appcache/appcache_url_loader_request.cc (right):
> 
>
https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
> content/browser/appcache/appcache_url_loader_request.cc:62: :
request_(request)
> {}
> On 2017/06/09 01:48:30, michaeln wrote:
> > is this copy expensive? i see other loaders make a copy too
> 
> Shouldn't be too expensive I think. This is a pattern in the n/w service code.

Will look into DeliverErrorResponse().

[jam, 2017-06-09 15:27:33]

lgtm, I defer to Michael for the AppCache specific code. I mostly looked at the
structure and integration with network service.

Agree that for this cl, having duplication of the data pipe usage is fine. It
would make it even bigger to share logic with the other URLLoader
implementations. Ananata and I had talked last week about having helper classes
to make this easier, and I was going to look at that next.

[michaeln, 2017-06-09 19:59:54]

lgtm, but please see comments about the storage ptr handling, there are some
crashes waiting to happen that can be easily addressed in this CL in that one
source file

thanks for being so responsive to comments

+1 sharing byte pushing code with other UrlLoaders in subsequent CLs

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/650001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:35:
storage_->LoadResponseInfo(manifest_url_, entry_.response_id(), this);
On 2017/06/09 04:48:19, ananta wrote:
> On 2017/06/09 02:06:18, michaeln wrote:
> > Unfortunately, the rawptr is a problem
> > 
> > We're going to have to implement the AppCacheServiceImpl::Observer
interface,
> > for OnServiceReinitialized, or access it thru a WeakPtr.
> > 
> > Also we have to call storage->CancelDelegateCallbacks(this) if we're
> destructed
> > prior to that method completing.
> 
> I added a call to CancelDelegateCallbacks in the dtor. The URLRequestJob also
> has the same problem  from what I can see. If it is ok we can revisit this in
a
> later patchset. Added a TODO here.

The other class is actually safe but its not at all obvious why. The 'host'
ensures the storage object does not get deleted and the 'job' is guaranteed to
be killed prior to the host being deleted. Very indirect, but there.

see host::OnServiceReinitialized(), hander::OnDestructionImminent(host), and
job::Kill().

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:13: if (storage_)
this is where we're likely to crash in some cases, the weakptr would protect
against that

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:58: storage_ = nullptr;
i'd suggest moving this into the NotifyCompleted helper and to be safe
conditionally CancelDelegateCallbacks in there if the ptr is non-null

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:116: 
As coded, i'm the raw storage_ptr_ is real likely to cause crashes. I think a
weakptr is probably a good fit eventually, but for this CL, i have some
suggestions about how to handlle it in this class, first...

null the storageptr out before return from this method, we only need the storage
obj to load the info and to create the reader, it's not needed anymore beyond
this point. Also no need to CancelDelegateCallbacks(this) in here either, The
only responseInfo callback we give it has been consumed.

suggestion, you could load it into a local variable and null out the data member
at the top of the method, use the local at the ->CreateResponseReader callsite.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:186: void
AppCacheURLLoaderJob::OnConnectionError() {
we definitely want to nullout the storage_ ptr and call CancelDelegateCallbacks
in here, we don't want to enter OnRepsonseInfoLoaded() past this point

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:260: // Fill other details
in the ResourceRequestCompletionStatus structure.
this method might be a good bottle neck to conditionally null out the storage_
ptr and call CancelDelegateCallbacks(this), we defenitely don't want to enter
OnResponseInfoLoaded past this point.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:71: void
OnCacheLoaded(AppCache* cache, int64_t cache_id) override;
the OnCacheLoaded method isn't needed in this class, its related to the
'executable' handler stuff which is not a supported feature

[ananta, 2017-06-09 20:37:50]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

6. Add WeakPtrFactory to the AppCacheStorage class and provide a getter to
return a weak pointer reference to the instance.

BUG=715632
==========

[ananta, 2017-06-09 20:39:33]

I added weak pointer support to AppCacheStorage and used it in the
AppCacheURLLoaderJob. Please take a peek. Thanks

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:13: if (storage_)
On 2017/06/09 19:59:53, michaeln wrote:
> this is where we're likely to crash in some cases, the weakptr would protect
> against that

Added weakptr support to AppCacheStorage.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:58: storage_ = nullptr;
On 2017/06/09 19:59:54, michaeln wrote:
> i'd suggest moving this into the NotifyCompleted helper and to be safe
> conditionally CancelDelegateCallbacks in there if the ptr is non-null

Done. We don't need to set it to null anymore as we hold a weakptr.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:116: 
On 2017/06/09 19:59:54, michaeln wrote:
> As coded, i'm the raw storage_ptr_ is real likely to cause crashes. I think a
> weakptr is probably a good fit eventually, but for this CL, i have some
> suggestions about how to handlle it in this class, first...
> 
> null the storageptr out before return from this method, we only need the
storage
> obj to load the info and to create the reader, it's not needed anymore beyond
> this point. Also no need to CancelDelegateCallbacks(this) in here either, The
> only responseInfo callback we give it has been consumed.
> 
> suggestion, you could load it into a local variable and null out the data
member
> at the top of the method, use the local at the ->CreateResponseReader
callsite.

We have a weakptr now. Checked it for null at the top and returned with
notifycompleted call.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:186: void
AppCacheURLLoaderJob::OnConnectionError() {
On 2017/06/09 19:59:53, michaeln wrote:
> we definitely want to nullout the storage_ ptr and call
CancelDelegateCallbacks
> in here, we don't want to enter OnRepsonseInfoLoaded() past this point

Done.We don't need to set it to null anymore as we hold a weakptr.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:260: // Fill other details
in the ResourceRequestCompletionStatus structure.
On 2017/06/09 19:59:53, michaeln wrote:
> this method might be a good bottle neck to conditionally null out the storage_
> ptr and call CancelDelegateCallbacks(this), we defenitely don't want to enter
> OnResponseInfoLoaded past this point.

Done. We don't need to set it to null anymore as we hold a weakptr.

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.h (right):

https://codereview.chromium.org/2902653002/diff/670001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:71: void
OnCacheLoaded(AppCache* cache, int64_t cache_id) override;
On 2017/06/09 19:59:54, michaeln wrote:
> the OnCacheLoaded method isn't needed in this class, its related to the
> 'executable' handler stuff which is not a supported feature

Thanks done.

[michaeln, 2017-06-09 21:49:00]

still lgtm

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:31: // connection at this
point though.
probably we should call DeliverErrorResponse() here we havn't sent headers yet,
this ptr can go bad at an time indepent of anythihg else

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:137:
NotifyCompleted(net::ERR_FAILED);
at this point, we haven't sent headers yet, we can call DeliverErrorResponse()

[ananta, 2017-06-09 22:16:53]

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
File content/browser/appcache/appcache_url_loader_job.cc (right):

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:31: // connection at this
point though.
On 2017/06/09 21:49:00, michaeln wrote:
> probably we should call DeliverErrorResponse() here we havn't sent headers
yet,
> this ptr can go bad at an time indepent of anythihg else

Done.

https://codereview.chromium.org/2902653002/diff/690002/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:137:
NotifyCompleted(net::ERR_FAILED);
On 2017/06/09 21:49:00, michaeln wrote:
> at this point, we haven't sent headers yet, we can call DeliverErrorResponse()

Done.

[ananta, 2017-06-09 22:25:11]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-06-09 22:25:11]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2902653002/#ps710001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-06-09 22:25:39]

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-06-09 23:44:08]

CQ is committing da patch.

Bot data: {"patchset_id": 710001, "attempt_start_ts": 1497047111050580,
"parent_rev": "6d30a69cd0eba07bbbff2e8efc9b0ab2e335dae4", "commit_rev":
"a2c8ec6149d02d7cfe9389d9739236ed06571a6a"}

[commit-bot: I haz the power, 2017-06-09 23:44:23]

Description was changed from

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

6. Add WeakPtrFactory to the AppCacheStorage class and provide a getter to
return a weak pointer reference to the instance.

BUG=715632
==========

to

==========
Get main frame and subframe AppCache loads to work. 

The latest iteration of this patch now has AppCache working for the main frame
and 
subframe. Subresource loads will be addressed in a later patchset. With this
change
the w3schools.com AppCache sample here
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_html_manifest
works.

This has been integrated with kinuko's patch, i.e. we implement the
URLLoaderRequestHandler interface to
validate if the request can be serviced via the AppCache.

The URLLoaderFactory for AppCache is now instantiated as needed. 

Changes in this patch include the following:
1. The AppCacheRequestHandler class now implements the URLLoaderRequestHandler
interface. The handler then initializes
   the AppCacheURLLoader job which allows us to determine if the request would
be serviced via the AppCache. If yes
   we return the URLLoaderFactory for the AppCache in the callback or pass null
to the callback.
 
2. We call the AppCacheRequestHandler::InitializeForNavigationNetworkService()
function from 
   NavigationURLLoaderNetworkService::URLLoaderRequestController::Start() for
AppCache.

3. Use the AppCacheResponseReader and AppCacheResponseInfo classes in
AppCacheURLLoaderJob to read data from the AppCache.
   The job invokes methods on the URLLoaderClient when it reads the
response/data, etc.
   The data from the AppCache is written to a mojo data pipe and passed to the
client.

4. I reverted portions of the previous change to the AppCacheURLLoaderRequest
class, which was to hold on to a 
   unique_ptr for the ResourceRequest. We now hold on to a copy of the request.
The CreateLoaderAndStart() method on the factory gives us a
   copy of the request.

5. Moved some of the methods like
IsWaiting/IsDeliveringAppCacheResponse/IsDeliveringNetworkResponse/IsDeliveringErrorResponse/IsCacheEntryNotFound
to the
   AppCacheJob base class as these implementations could be shared between the
URLRequest and URLLoader versions of the subclasses. I also moved the
   member variables used by these methods to the base class.

6. Add WeakPtrFactory to the AppCacheStorage class and provide a getter to
return a weak pointer reference to the instance.

BUG=715632

Review-Url: https://codereview.chromium.org/2902653002
Cr-Commit-Position: refs/heads/master@{#478459}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2c8ec6149d02d7cfe9389d97392...
==========

[commit-bot: I haz the power, 2017-06-09 23:44:25]

Committed patchset #38 (id:710001) as
https://chromium.googlesource.com/chromium/src/+/a2c8ec6149d02d7cfe9389d97392...

[kinuko, 2017-06-13 07:50:35]

lgtm