Add support for subresource request loads in AppCache for the network service.

Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to the job. The job controls
the lifetime of the handler.

The AppCacheSubresourceURLFactory class is passed the precreated AppCache host during init. We save
maintain this as a weak reference in the class to protect against the case where the host dies while we are 
processing the call from the renderer. Added support to get a weak pointer in the AppCacheHost class.
The AppCacheRequestHandler saves away the host as a weak reference during InitializeForNavigationNetworkService()
and passes to the factory in MaybeCreateSubresourceFactory()

The other changes are to pass the information from CreateLoaderAndStart() to the AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we weren't terminating the write correctly
causing the renderer to not consider the load as complete. 

With this change normal subresource loads will work. Fallback support is not added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2956373002
Cr-Commit-Position: refs/heads/master@{#485834}
Committed: https://chromium.googlesource.com/chromium/src/+/2533bff6fceb80f4ac6b582e4ffbf45bfde2a064

[ananta, 2017-06-28 19:39:07]

Description was changed from

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The factory needs the renderer process id to look up the AppCacheHost for the
renderer. To achieve this
we pass the renderer process id from the NavigationURLLoaderNetworkService class
to the URLLoaderRequestController
and from there we pass it to the URLLoaderRequestHandler instances. I added a
method SetRendererProcessId() to
the URLLoaderRequestHandler base class. The AppCacheRequestHandler implements it
and passes the process id to the
subresource factory.

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete.

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

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

to

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The factory needs the renderer process id to look up the AppCacheHost for the
renderer. To achieve this
we pass the renderer process id from the NavigationURLLoaderNetworkService class
to the URLLoaderRequestController
and from there we pass it to the URLLoaderRequestHandler instances. I added a
method SetRendererProcessId() to
the URLLoaderRequestHandler base class. The AppCacheRequestHandler implements it
and passes the process id to the
subresource factory.

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete.

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2017-06-28 19:45:05]

Description was changed from

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The factory needs the renderer process id to look up the AppCacheHost for the
renderer. To achieve this
we pass the renderer process id from the NavigationURLLoaderNetworkService class
to the URLLoaderRequestController
and from there we pass it to the URLLoaderRequestHandler instances. I added a
method SetRendererProcessId() to
the URLLoaderRequestHandler base class. The AppCacheRequestHandler implements it
and passes the process id to the
subresource factory.

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete.

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The factory needs the renderer process id to look up the AppCacheHost for the
renderer. To achieve this
we pass the renderer process id from the NavigationURLLoaderNetworkService class
to the URLLoaderRequestController
and from there we pass it to the URLLoaderRequestHandler instances. I added a
method SetRendererProcessId() to
the URLLoaderRequestHandler base class. The AppCacheRequestHandler implements it
and passes the process id to the
subresource factory.

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete. The
NavigationURLLoaderDelegate interface has been updated with a method
GetRendererProcessId().
This is implemented by the NavigationRequest class to return the renderer pid.

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2017-06-28 19:48:28]

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

[commit-bot: I haz the power, 2017-06-28 19:49:03]

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-28 20:02:53]

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

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

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...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-28 20:19:21]

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

[commit-bot: I haz the power, 2017-06-28 20:19:41]

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-28 20:21:21]

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

[commit-bot: I haz the power, 2017-06-28 20:21:42]

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-28 20:21:46]

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

[ananta, 2017-06-28 20:26:06]

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

[commit-bot: I haz the power, 2017-06-28 20:26: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-28 21:37:47]

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

[commit-bot: I haz the power, 2017-06-28 21:37:49]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-29 15:16:50]

I think you can pass in the frame tree node id in
InitializeForNavigationNetworkService, which is available in
NavigationRequestInfo. then you don't need as much plumbing?

[ananta, 2017-06-29 22:10:05]

Description was changed from

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The factory needs the renderer process id to look up the AppCacheHost for the
renderer. To achieve this
we pass the renderer process id from the NavigationURLLoaderNetworkService class
to the URLLoaderRequestController
and from there we pass it to the URLLoaderRequestHandler instances. I added a
method SetRendererProcessId() to
the URLLoaderRequestHandler base class. The AppCacheRequestHandler implements it
and passes the process id to the
subresource factory.

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete. The
NavigationURLLoaderDelegate interface has been updated with a method
GetRendererProcessId().
This is implemented by the NavigationRequest class to return the renderer pid.

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The AppCacheSubresourceURLFactory class is passed the precreated AppCache host
during init. We save
maintain this as a weak reference in the class to protect against the case where
the host dies while we are 
processing the call from the renderer. Added support to get a weak pointer in
the AppCacheHost class.
The AppCacheRequestHandler saves away the host as a weak reference during
InitializeForNavigationNetworkService()
and passes to the factory in MaybeCreateSubresourceFactory()

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete. 

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2017-06-29 22:11:08]

On 2017/06/29 15:16:50, jam wrote:
> I think you can pass in the frame tree node id in
> InitializeForNavigationNetworkService, which is available in
> NavigationRequestInfo. then you don't need as much plumbing?

As per discussion added support for weak pointers in AppCacheHost. We save the
host as a weak reference
in the Appcache subresource factory

[ananta, 2017-06-29 22:13:35]

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

[commit-bot: I haz the power, 2017-06-29 22:13:57]

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-29 23:46:31]

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

[commit-bot: I haz the power, 2017-06-29 23:46:33]

Dry run: This issue passed the CQ dry run.

[jam, 2017-06-30 01:31:16]

nice, this looks better thanks. I'm not very familiar with the appcache code
tho, so maybe wait for mike who's coming back soon to give the lgtm?

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:24: struct
SubresourceLoadInfo {
nit: why is this in this header if it's not referenced in this file? seems like
it should be in appcache_request_handler.h?

[ananta, 2017-06-30 01:39:07]

It would be great if we could land this if things look ok. I will address
michael's comments in a followup when he gets back on the 5th.

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:24: struct
SubresourceLoadInfo {
On 2017/06/30 01:31:15, jam wrote:
> nit: why is this in this header if it's not referenced in this file? seems
like
> it should be in appcache_request_handler.h?

It is populated in the factory cc file. I thought it is better to have it there.

[ananta, 2017-06-30 01:53:42]

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/100001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:24: struct
SubresourceLoadInfo {
On 2017/06/30 01:39:06, ananta wrote:
> On 2017/06/30 01:31:15, jam wrote:
> > nit: why is this in this header if it's not referenced in this file? seems
> like
> > it should be in appcache_request_handler.h?
> 
> It is populated in the factory cc file. I thought it is better to have it
there.
> 

I moved it to the appcache_url_loader_job.h/.cc files

[ananta, 2017-06-30 01:54:25]

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

[commit-bot: I haz the power, 2017-06-30 01:54: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-30 03:09:43]

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

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

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-07-03 06:34:14]

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

[kinuko, 2017-07-03 06:34:15]

RequestHandler and Job ownership relationship looks a bit unclear, let me leave
some comments...

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_host.h (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_host.h:361: // get destroyed last.
nit: this comment felt a bit confusing to me (two 'last' in different context),
maybe just remove (we have clang warning for this right?) or 'they get destroyed
last' -> 'weak pointers are invalidated before other members'?

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

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:336: !is_main_resource()) {
nit: duplicated !is_main_resource check

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.cc (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:31:
appcache_host_(host->GetWeakPtr()) {
|host|'s already weakptr, isn't it?

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:92:
handler.release();
MaybeLoadResource may not call CreateJob, then wouldn't we just leak handler? 
Also during MaybeLoadResource we seem to temporarily make the handler
double-owned by two unique_ptr, is that right? (making me feel a bit uneasy).
Should we save the return value of MaybeLoadResource, and if it's non-null then
explicitly pass the handler's ownership afterwards?

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:30: // 2. The
|frame_tree_node_id| parameter contains the FrameTreeNodeId for the
Do you mean to mention |host| in the latest patch set?

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

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:77:
std::move(loader_callback_).Run(StartLoaderCallback());
nit: maybe rename loader_callback_ to main_resource_loader_callback_

[ananta, 2017-07-03 07:36:47]

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_host.h (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_host.h:361: // get destroyed last.
On 2017/07/03 06:34:15, kinuko wrote:
> nit: this comment felt a bit confusing to me (two 'last' in different
context),
> maybe just remove (we have clang warning for this right?) or 'they get
destroyed
> last' -> 'weak pointers are invalidated before other members'?

Removed this comment. I checked other places in the code. We have a number of
them which don't have a comment.

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

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:336: !is_main_resource()) {
On 2017/07/03 06:34:15, kinuko wrote:
> nit: duplicated !is_main_resource check

Thanks removed

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.cc (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:31:
appcache_host_(host->GetWeakPtr()) {
On 2017/07/03 06:34:15, kinuko wrote:
> |host|'s already weakptr, isn't it?

Sorry I missed this. Fixed

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:92:
handler.release();
On 2017/07/03 06:34:15, kinuko wrote:
> MaybeLoadResource may not call CreateJob, then wouldn't we just leak handler? 
> Also during MaybeLoadResource we seem to temporarily make the handler
> double-owned by two unique_ptr, is that right? (making me feel a bit uneasy).
> Should we save the return value of MaybeLoadResource, and if it's non-null
then
> explicitly pass the handler's ownership afterwards?

Thanks. done. I added a setter to the AppCacheURLLoaderJob set_request_handler()
which is 
called here if the job is non null.

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:30: // 2. The
|frame_tree_node_id| parameter contains the FrameTreeNodeId for the
On 2017/07/03 06:34:15, kinuko wrote:
> Do you mean to mention |host| in the latest patch set?

Done.

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

https://codereview.chromium.org/2956373002/diff/120001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.cc:77:
std::move(loader_callback_).Run(StartLoaderCallback());
On 2017/07/03 06:34:15, kinuko wrote:
> nit: maybe rename loader_callback_ to main_resource_loader_callback_

Done.

[ananta, 2017-07-03 07:38:11]

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

[commit-bot: I haz the power, 2017-07-03 07:38:21]

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-07-03 08:45:33]

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

[commit-bot: I haz the power, 2017-07-03 08:45:34]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-07-04 05:47:49]

Thanks, this looks reasonable to me for the scope this CL covers.

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

https://codereview.chromium.org/2956373002/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:338: // The job owns us
from this point on.
nit: the comment is stale now

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

https://codereview.chromium.org/2956373002/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:91: void
set_request_handler(AppCacheRequestHandler* handler) {
nit: this can take std::unique_ptr<Handler> to make the ownership semantics
clearer

[ananta, 2017-07-05 13:31:03]

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

https://codereview.chromium.org/2956373002/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:338: // The job owns us
from this point on.
On 2017/07/04 05:47:49, kinuko wrote:
> nit: the comment is stale now

Thanks. removed

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

https://codereview.chromium.org/2956373002/diff/140001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:91: void
set_request_handler(AppCacheRequestHandler* handler) {
On 2017/07/04 05:47:49, kinuko wrote:
> nit: this can take std::unique_ptr<Handler> to make the ownership semantics
> clearer

Thanks. done

[ananta, 2017-07-05 13:32:31]

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

[commit-bot: I haz the power, 2017-07-05 13:32:37]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-07-05 14:35:52]

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

[commit-bot: I haz the power, 2017-07-05 14:35:53]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-07-07 13:10:28]

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

[commit-bot: I haz the power, 2017-07-07 13:10: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-07-07 14:38:00]

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

[commit-bot: I haz the power, 2017-07-07 14:38:01]

Dry run: This issue passed the CQ dry run.

[michaeln, 2017-07-11 00:14:58]

i think this lgtm

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

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_request_handler.cc:333: if (job_.get() &&
!is_main_resource() &&
can job_.get() ever test false here, if not we should remove it

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

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:74: return;
What happens when we return without having created a loader? Does a subresource
load 'hang' from the renderers point of view? We expect to get a null handler in
some cases.

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

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_url_loader_job.h:164:
std::unique_ptr<AppCacheRequestHandler> handler_;
maybe calls this subresource_handler_ to make it more obvious it only applies
there

[kinuko, 2017-07-11 07:28:22]

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

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.cc:74: return;
On 2017/07/11 00:14:58, michaeln wrote:
> What happens when we return without having created a loader? Does a
subresource
> load 'hang' from the renderers point of view? We expect to get a null handler
in
> some cases.

This is a good point, I think we should either fall back to the network or
return an error.

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
File content/browser/appcache/appcache_subresource_url_factory.h (right):

https://codereview.chromium.org/2956373002/diff/200001/content/browser/appcac...
content/browser/appcache/appcache_subresource_url_factory.h:31: //    create the
AppCacheRequestHandler instances for handling subresource
nit: used create -> used to create

[ananta, 2017-07-11 18:48:08]

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

[commit-bot: I haz the power, 2017-07-11 18:48: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-07-11 19:41:36]

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

[commit-bot: I haz the power, 2017-07-11 19:41:37]

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

[ananta, 2017-07-12 00:33:04]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-07-12 00:33:05]

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/2956373002/#ps220001
(title: "Address review comments")

[commit-bot: I haz the power, 2017-07-12 00:33:57]

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-07-12 04:07:31]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499819584564740,
"parent_rev": "727468b41b2fe48bf73983ab0c3aa2632b47d89e", "commit_rev":
"f30ea38178097dccdd7a66fa1e81a882a22dc497"}

[commit-bot: I haz the power, 2017-07-12 04:07:52]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499819584564740,
"parent_rev": "9bbb88ae99a65adb4c35ebaccf4e52c1abc135f1", "commit_rev":
"c8a74838a571e3367e1ef20785293e1660a9af91"}

[commit-bot: I haz the power, 2017-07-12 04:08:11]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1499819584564740,
"parent_rev": "2a2713acf494984de18b70398f739eced6173ca7", "commit_rev":
"2533bff6fceb80f4ac6b582e4ffbf45bfde2a064"}

[commit-bot: I haz the power, 2017-07-12 04:08:25]

Description was changed from

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The AppCacheSubresourceURLFactory class is passed the precreated AppCache host
during init. We save
maintain this as a weak reference in the class to protect against the case where
the host dies while we are 
processing the call from the renderer. Added support to get a weak pointer in
the AppCacheHost class.
The AppCacheRequestHandler saves away the host as a weak reference during
InitializeForNavigationNetworkService()
and passes to the factory in MaybeCreateSubresourceFactory()

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete. 

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Add support for subresource request loads in AppCache for the network service.

This patch builds on the earlier patch where we passed the URLLoaderFactory for
AppCache
to the renderer process. The renderer invokes CreateLoaderAndStart which comes
to the AppCache
factory. We then instantiate the AppCacheRequestHandler instance here and call
into it to handle
the request. The handler creates the AppCacheURLLoaderJob and passes itself to
the job. The job controls
the lifetime of the handler.

The AppCacheSubresourceURLFactory class is passed the precreated AppCache host
during init. We save
maintain this as a weak reference in the class to protect against the case where
the host dies while we are 
processing the call from the renderer. Added support to get a weak pointer in
the AppCacheHost class.
The AppCacheRequestHandler saves away the host as a weak reference during
InitializeForNavigationNetworkService()
and passes to the factory in MaybeCreateSubresourceFactory()

The other changes are to pass the information from CreateLoaderAndStart() to the
AppCacheRequestHandler class
and from there to the job. A bug was fixed in the AppCacheURLLoaderJob where we
weren't terminating the write correctly
causing the renderer to not consider the load as complete. 

With this change normal subresource loads will work. Fallback support is not
added yet and will happen in a followup.

BUG=715632
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2956373002
Cr-Commit-Position: refs/heads/master@{#485834}
Committed:
https://chromium.googlesource.com/chromium/src/+/2533bff6fceb80f4ac6b582e4ffb...
==========

[commit-bot: I haz the power, 2017-07-12 04:08:27]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/2533bff6fceb80f4ac6b582e4ffb...