Add an abstraction for a job in the AppCacheRequestHandler class.

Add an abstraction for a job in the AppCacheRequestHandler class.

The abstraction is in the form of an AppCacheJob base class which has two subclasses.
1. AppCacheURLRequestJob : This is the current URLRequestJob implementation which relies
   on URLRequest interception to serve requests from the cache.

2. AppCacheURLLoaderJob : This will eventually be the job mechanism for the URLLoader based
   AppCache functionality (network service).

The AppCacheJob provides a number of methods like DeliverAppCachedResponse(),
DeliverNetworkResponse(), DeliverErrorResponse(), etc which allow the job to control the content
being served out.

The expectation is that with this abstraction, we should have minimal changes in core AppCache
functionality for the URLLoader work. We will mostly be an AppCache consumer.

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

[ananta, 2017-05-05 04:18:48]

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

[ananta, 2017-05-05 04:18:59]

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

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

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-05 04:28:38]

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

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

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-05 05:59:59]

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

[commit-bot: I haz the power, 2017-05-05 06:00: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-05-05 07:16:29]

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

[commit-bot: I haz the power, 2017-05-05 07:16:30]

Dry run: This issue passed the CQ dry run.

[jam, 2017-05-05 23:15:25]

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_job.h:30: // Interface for an AppCache job.
Subclasses implement this interface to
nit: can you describe what the purpose of this interface is? i.e. it's used to
send data stored in appcache to the networking code

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_job.h:35: public
base::SupportsWeakPtr<AppCacheJob> {
nit: don't need to derive from SupportsWeakPtr since it already has a
GetWeakPtr()

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:301:
std::unique_ptr<AppCacheJob> job = AppCacheJob::Create(
why not just create the two concrete classes here directly, instead of having
the enum? This code can check for the command line flag.

then you can remove the friend statement in appcache_request.h

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:19: // AppCacheJob wrapper
for the AppCacheURLLoaderJob class.
nit: the comment is a bit confusing. maybe it should be like the net:: one and
say 

// A mojom::URLLoader derivative that knows how to return a response stored
// in the appcache.

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:41: // AppCacheJob::Create()
creates this instance.
ditto

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_request_job.h:76: // AppCacheJob::Create()
creates this instance.
do we gain much by making the constructor private?

[ananta, 2017-05-06 03:01:35]

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_job.h:30: // Interface for an AppCache job.
Subclasses implement this interface to
On 2017/05/05 23:15:25, jam wrote:
> nit: can you describe what the purpose of this interface is? i.e. it's used to
> send data stored in appcache to the networking code

Done.

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_job.h:35: public
base::SupportsWeakPtr<AppCacheJob> {
On 2017/05/05 23:15:25, jam wrote:
> nit: don't need to derive from SupportsWeakPtr since it already has a
> GetWeakPtr()

Unfortunately we need this as the AppCacheURLRequestJob class uses the weakptr
to post tasks within
itself. This ensures that we get a casted weakptr.

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_request_handler.cc:301:
std::unique_ptr<AppCacheJob> job = AppCacheJob::Create(
On 2017/05/05 23:15:25, jam wrote:
> why not just create the two concrete classes here directly, instead of having
> the enum? This code can check for the command line flag.
> 
> then you can remove the friend statement in appcache_request.h

Removed the enum. I thought it is better to leave the ctors private for the jobs
as it seems
better to not create the classes directly except for the case here and in tests.
Leaving the friend as is.

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:19: // AppCacheJob wrapper
for the AppCacheURLLoaderJob class.
On 2017/05/05 23:15:25, jam wrote:
> nit: the comment is a bit confusing. maybe it should be like the net:: one and
> say 
> 
> // A mojom::URLLoader derivative that knows how to return a response stored
> // in the appcache.

Done.

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_loader_job.h:41: // AppCacheJob::Create()
creates this instance.
On 2017/05/05 23:15:25, jam wrote:
> ditto

Leaving this as is based on the response in the other comment.

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_url_request_job.h:76: // AppCacheJob::Create()
creates this instance.
On 2017/05/05 23:15:25, jam wrote:
> do we gain much by making the constructor private?

It seemed better to have create done by the base AppCache class and not have the
AppCacheRequestHandler have to know about it.

[ananta, 2017-05-06 04:18:42]

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

[commit-bot: I haz the power, 2017-05-06 04:19: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-05-06 04:28:19]

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

[commit-bot: I haz the power, 2017-05-06 04:28:20]

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

[ananta, 2017-05-07 18:20:47]

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

[commit-bot: I haz the power, 2017-05-07 18:21:01]

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-07 19:40:32]

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

[commit-bot: I haz the power, 2017-05-07 19:40:33]

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

[ananta, 2017-05-08 02:42:57]

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

[commit-bot: I haz the power, 2017-05-08 02:43:19]

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-08 04:09:25]

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

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

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

[jam, 2017-05-08 14:03:12]

lgtm with one comment

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

https://codereview.chromium.org/2865613002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_job.h:35: public
base::SupportsWeakPtr<AppCacheJob> {
On 2017/05/06 03:01:34, ananta wrote:
> On 2017/05/05 23:15:25, jam wrote:
> > nit: don't need to derive from SupportsWeakPtr since it already has a
> > GetWeakPtr()
> 
> Unfortunately we need this as the AppCacheURLRequestJob class uses the weakptr
> to post tasks within
> itself. This ensures that we get a casted weakptr.

AppCacheURLRequestJob used to call weak_factory_.GetWeakPtr(). You changed it to
StaticAsWeakPtr(this).

but it can also be written as GetWeakPtr(), and then you can remove the base
class from the interface

[jam, 2017-05-08 21:51:41]

thanks for the in-person explanation (GetWeakPtr won't work since type needs to
be for the child classes)

lgtm

[ananta, 2017-05-08 21:54:21]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-05-08 21:55:22]

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-08 22:47:11]

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

[commit-bot: I haz the power, 2017-05-08 22:47:12]

Try jobs failed on following builders:
  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...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-05-09 02:01:18]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-05-09 02:03:20]

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-09 04:56:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494295278916550,
"parent_rev": "6788ad5e2f48d4228195884715f1809e2922e3ac", "commit_rev":
"46f3e94b444a8fa09c22cdd0b0d3313414feb0c1"}

[commit-bot: I haz the power, 2017-05-09 04:56:48]

Description was changed from

==========
Add an abstraction for a job in the AppCacheRequestHandler class.

The abstraction is in the form of an AppCacheJob base class which has two
subclasses.
1. AppCacheURLRequestJob : This is the current URLRequestJob implementation
which relies
   on URLRequest interception to serve requests from the cache.

2. AppCacheURLLoaderJob : This will eventually be the job mechanism for the
URLLoader based
   AppCache functionality (network service).

The AppCacheJob provides a number of methods like DeliverAppCachedResponse(),
DeliverNetworkResponse(), DeliverErrorResponse(), etc which allow the job to
control the content
being served out.

The expectation is that with this abstraction, we should have minimal changes in
core AppCache
functionality for the URLLoader work. We will mostly be an AppCache consumer.

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

to

==========
Add an abstraction for a job in the AppCacheRequestHandler class.

The abstraction is in the form of an AppCacheJob base class which has two
subclasses.
1. AppCacheURLRequestJob : This is the current URLRequestJob implementation
which relies
   on URLRequest interception to serve requests from the cache.

2. AppCacheURLLoaderJob : This will eventually be the job mechanism for the
URLLoader based
   AppCache functionality (network service).

The AppCacheJob provides a number of methods like DeliverAppCachedResponse(),
DeliverNetworkResponse(), DeliverErrorResponse(), etc which allow the job to
control the content
being served out.

The expectation is that with this abstraction, we should have minimal changes in
core AppCache
functionality for the URLLoader work. We will mostly be an AppCache consumer.

BUG=715632

Review-Url: https://codereview.chromium.org/2865613002
Cr-Commit-Position: refs/heads/master@{#470227}
Committed:
https://chromium.googlesource.com/chromium/src/+/46f3e94b444a8fa09c22cdd0b0d3...
==========

[commit-bot: I haz the power, 2017-05-09 04:56:49]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/46f3e94b444a8fa09c22cdd0b0d3...