Add pause/resume functionality to precache

Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
Committed: https://crrev.com/0bb5f835faf844ba31f602f6502b7c3f8db30624
Cr-Commit-Position: refs/heads/master@{#396358}

[bengr, 2016-05-09 20:58:16]

Patchset #1 (id:1) has been deleted

[bengr, 2016-05-09 20:59:16]

bengr@chromium.org changed reviewers:
+ sclittle@chromium.org

[bengr, 2016-05-09 20:59:17]

sclittle@: PTAL

[bengr, 2016-05-09 21:00:51]

Description was changed from

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
==========

to

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
==========

[sclittle, 2016-05-10 00:01:27]

https://codereview.chromium.org/1961153003/diff/20001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/con...
components/precache/content/precache_manager.cc:302: void
PrecacheManager::OnInitDone() {
Why is this method necessary? Couldn't PrecacheFetcher::Init be combined with
PrecacheFetcher::Start?

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_database.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:40: class UnfinishedWork : public
base::RefCountedThreadSafe<UnfinishedWork> {
See my comment in precache_fetcher.cc about passing this around as a
std::unique_ptr instead of refcounting.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:63: std::list<GURL> manifests;
If you just have publicly visible members, then could you change UnfinishedWork
to be a struct instead of a class? Otherwise, you should add getters and setters
for each.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:78: base::TimeTicks start_time;
Should this be base::Time instead? I don't think it's safe to compare
base::TimeTicks values between different sessions, especially if the device was
rebooted or something.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:329:
database_task_runner_->PostTaskAndReply(
I'm not sure that UnfinishedWork needs to be refcounted or passed as a
scoped_refptr.

Could you just use PostTaskAndReplyWithResult, where the result is a
std::unique_ptr<UnfinishedWork>?
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u...

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:342: //TODO(rajendrant): report
expired unfinished job.
nit: add space between "//" and "TODO", and capitalize "report".

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:346: manifest_urls_to_fetch_ =
unfinished_work->manifests;
Use std::move on the lists here to avoid copying them all.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:469: if
(manifest_urls_to_fetch_.empty()) {
What about if the UnfinishedWork already finished fetching the manifests, and
only has resource URLs left? It would try to fetch all the manifests again here,
wouldn't it?

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:119: base::WeakPtr<PrecacheDatabase>
precache_database,
It seems odd for the PrecacheFetcher to have knowledge of the database. AIUI,
ideally PrecacheFetcher would just fetch URLs that it's told to fetch. What if
the classes were reorganized like this:

- Make UnfinishedWork contain almost all the state of PrecacheFetcher, including
the starting_hosts, the config_url, and the ConfigurationSettings itself if it's
already been fetched. That way, the UnfinishedWork can represent the state right
before precaching is started.
- Make PrecacheFetcher always take in an UnfinishedWork and develop its state
around that, where UnfinishedWork could represent the state before precaching
was even started.

Then, PrecacheManager could use PrecacheDatabase to check if any partially done
UnfinishedWork was saved and use that if it exists, otherwise it could just
create a new UnfinishedWork representing the state before any precaching is
done, e.g. only the starting_hosts and stuff are set.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:138: int num_manifest_urls_to_fetch,
nit: Why an int? Why not a size_t?

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:200: size_t total_response_bytes_;
nit, maybe for a followup CL: This should probably actually be an int64_t, not a
size_t. Same with |network_response_bytes_| below.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:211: std::list<GURL>
manifest_urls_to_fetch_;
nit, maybe for a followup CL: This was originally my fault when I first wrote
this class years ago, but an std::deque (or even just use an std::queue) would
almost certainly be more efficient than an std::list for this, since we only
need to add or remove elements from the beginning and end. Do you mind fixing
this?

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_session_tables.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:44: while (statement.Step())
{
How large can the list of URLs get? e.g. 50? 500?

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:91: for (auto manifest :
manifests)
Use "const auto&" or "const GURL&" here to avoid copying the GURL every time.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:93: for (auto resource :
resources)
Use "const auto&" or "const GURL&" here to avoid copying the GURL every time.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_session_tables.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.h:31: enum StatType {
Instead of using a Type enum, could you either use a protobuf (if you're worried
about easily adding new stats in the future) or just create a column for each of
these?

You could probably even just make the entire UnfinishedWork a protobuf if you
really wanted, and just serialize that to a string when saving it. Then this
whole PrecacheSessionTables could be replaced with just a single table with a
single row of the serialized proto.

[bengr, 2016-05-19 01:01:04]

Patchset #2 (id:40001) has been deleted

[bengr, 2016-05-19 01:25:30]

Patchset #2 (id:60001) has been deleted

[bengr, 2016-05-19 01:25:44]

https://codereview.chromium.org/1961153003/diff/20001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/con...
components/precache/content/precache_manager.cc:302: void
PrecacheManager::OnInitDone() {
On 2016/05/10 00:01:26, sclittle wrote:
> Why is this method necessary? Couldn't PrecacheFetcher::Init be combined with
> PrecacheFetcher::Start?

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_database.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:40: class UnfinishedWork : public
base::RefCountedThreadSafe<UnfinishedWork> {
On 2016/05/10 00:01:26, sclittle wrote:
> See my comment in precache_fetcher.cc about passing this around as a
> std::unique_ptr instead of refcounting.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:63: std::list<GURL> manifests;
On 2016/05/10 00:01:26, sclittle wrote:
> If you just have publicly visible members, then could you change
UnfinishedWork
> to be a struct instead of a class? Otherwise, you should add getters and
setters
> for each.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_database.h:78: base::TimeTicks start_time;
On 2016/05/10 00:01:26, sclittle wrote:
> Should this be base::Time instead? I don't think it's safe to compare
> base::TimeTicks values between different sessions, especially if the device
was
> rebooted or something.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:329:
database_task_runner_->PostTaskAndReply(
On 2016/05/10 00:01:26, sclittle wrote:
> I'm not sure that UnfinishedWork needs to be refcounted or passed as a
> scoped_refptr.
> 
> Could you just use PostTaskAndReplyWithResult, where the result is a
> std::unique_ptr<UnfinishedWork>?
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/task_runner_u...

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:342: //TODO(rajendrant): report
expired unfinished job.
On 2016/05/10 00:01:26, sclittle wrote:
> nit: add space between "//" and "TODO", and capitalize "report".

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:346: manifest_urls_to_fetch_ =
unfinished_work->manifests;
On 2016/05/10 00:01:26, sclittle wrote:
> Use std::move on the lists here to avoid copying them all.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.cc:469: if
(manifest_urls_to_fetch_.empty()) {
On 2016/05/10 00:01:26, sclittle wrote:
> What about if the UnfinishedWork already finished fetching the manifests, and
> only has resource URLs left? It would try to fetch all the manifests again
here,
> wouldn't it?

Good catch. Thanks.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:119: base::WeakPtr<PrecacheDatabase>
precache_database,
On 2016/05/10 00:01:27, sclittle wrote:
> It seems odd for the PrecacheFetcher to have knowledge of the database. AIUI,
> ideally PrecacheFetcher would just fetch URLs that it's told to fetch. What if
> the classes were reorganized like this:
> 
> - Make UnfinishedWork contain almost all the state of PrecacheFetcher,
including
> the starting_hosts, the config_url, and the ConfigurationSettings itself if
it's
> already been fetched. That way, the UnfinishedWork can represent the state
right
> before precaching is started.
> - Make PrecacheFetcher always take in an UnfinishedWork and develop its state
> around that, where UnfinishedWork could represent the state before precaching
> was even started.
> 
> Then, PrecacheManager could use PrecacheDatabase to check if any partially
done
> UnfinishedWork was saved and use that if it exists, otherwise it could just
> create a new UnfinishedWork representing the state before any precaching is
> done, e.g. only the starting_hosts and stuff are set.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:138: int num_manifest_urls_to_fetch,
On 2016/05/10 00:01:26, sclittle wrote:
> nit: Why an int? Why not a size_t?

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:200: size_t total_response_bytes_;
On 2016/05/10 00:01:26, sclittle wrote:
> nit, maybe for a followup CL: This should probably actually be an int64_t, not
a
> size_t. Same with |network_response_bytes_| below.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher.h:211: std::list<GURL>
manifest_urls_to_fetch_;
On 2016/05/10 00:01:26, sclittle wrote:
> nit, maybe for a followup CL: This was originally my fault when I first wrote
> this class years ago, but an std::deque (or even just use an std::queue) would
> almost certainly be more efficient than an std::list for this, since we only
> need to add or remove elements from the beginning and end. Do you mind fixing
> this?

Acknowledged.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_session_tables.cc (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:44: while (statement.Step())
{
On 2016/05/10 00:01:27, sclittle wrote:
> How large can the list of URLs get? e.g. 50? 500?

Covered in tests.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:91: for (auto manifest :
manifests)
On 2016/05/10 00:01:27, sclittle wrote:
> Use "const auto&" or "const GURL&" here to avoid copying the GURL every time.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.cc:93: for (auto resource :
resources)
On 2016/05/10 00:01:27, sclittle wrote:
> Use "const auto&" or "const GURL&" here to avoid copying the GURL every time.

Done.

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
File components/precache/core/precache_session_tables.h (right):

https://codereview.chromium.org/1961153003/diff/20001/components/precache/cor...
components/precache/core/precache_session_tables.h:31: enum StatType {
On 2016/05/10 00:01:27, sclittle wrote:
> Instead of using a Type enum, could you either use a protobuf (if you're
worried
> about easily adding new stats in the future) or just create a column for each
of
> these?
> 
> You could probably even just make the entire UnfinishedWork a protobuf if you
> really wanted, and just serialize that to a string when saving it. Then this
> whole PrecacheSessionTables could be replaced with just a single table with a
> single row of the serialized proto.

Done.

[sclittle, 2016-05-19 01:59:46]

preliminary comments, I'll take a closer look soon

https://codereview.chromium.org/1961153003/diff/80001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/con...
components/precache/content/precache_manager.cc:160: for (int i = 0; i <
unfinished_work_->top_host_size(); ++i) {
Could the hosts just be stored in the unfinished_work proto, then passed to the
fetcher from there?

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:385:
TEST_F(PrecacheDatabaseTest, SaveAndGetUnfinishedWork) {
You could probably just remove this test, since there are already session table
tests for this stuff.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:289: if (has_config_)
This seems unnecessary, just check has_config_settings().

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:291: for (int i = 0; i <
unfinished_work->manifest_size(); ++i)
nit: braces

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:291: for (int i = 0; i <
unfinished_work->manifest_size(); ++i)
nit: use range-based for, e.g. for (const auto& blah: ...)

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:103: PrecacheFetcher(
Could this be separated into two constructors, with one taking just an
unfinished_work and the other taking the stuff to build one?

Alternatively, you could just put all the |config_url| and stuff into the
unfinished work itself, and just have one constructor with the
|unfinished_work|.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:108: const PrecacheUnfinishedWork*
unfinished_work,
Could you just pass in an unfinished_work with the starting_hosts already set?

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:167: bool has_config_;
Could you just check if |config_| is non-null?

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_session_table.h (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_session_table.h:34: class PrecacheSessionTable
{
Maybe consider cutting out this class and just putting the implementations of
Save/GetUnfinishedWork into PrecacheDatabase itself?

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/proto/unfinished_work.proto (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/proto/unfinished_work.proto:19: optional int32 rank =
2;
Could you just store these in rank order instead of storing rank here?

[bengr, 2016-05-19 23:19:32]

https://codereview.chromium.org/1961153003/diff/80001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/con...
components/precache/content/precache_manager.cc:160: for (int i = 0; i <
unfinished_work_->top_host_size(); ++i) {
On 2016/05/19 01:59:45, sclittle wrote:
> Could the hosts just be stored in the unfinished_work proto, then passed to
the
> fetcher from there?

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_database_unittest.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:385:
TEST_F(PrecacheDatabaseTest, SaveAndGetUnfinishedWork) {
On 2016/05/19 01:59:45, sclittle wrote:
> You could probably just remove this test, since there are already session
table
> tests for this stuff.

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:289: if (has_config_)
On 2016/05/19 01:59:45, sclittle wrote:
> This seems unnecessary, just check has_config_settings().

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:291: for (int i = 0; i <
unfinished_work->manifest_size(); ++i)
On 2016/05/19 01:59:45, sclittle wrote:
> nit: braces

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.cc:291: for (int i = 0; i <
unfinished_work->manifest_size(); ++i)
On 2016/05/19 01:59:45, sclittle wrote:
> nit: use range-based for, e.g. for (const auto& blah: ...)

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:103: PrecacheFetcher(
On 2016/05/19 01:59:46, sclittle wrote:
> Could this be separated into two constructors, with one taking just an
> unfinished_work and the other taking the stuff to build one?
> 
> Alternatively, you could just put all the |config_url| and stuff into the
> unfinished work itself, and just have one constructor with the
> |unfinished_work|.

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:108: const PrecacheUnfinishedWork*
unfinished_work,
On 2016/05/19 01:59:45, sclittle wrote:
> Could you just pass in an unfinished_work with the starting_hosts already set?

Yes.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_fetcher.h:167: bool has_config_;
On 2016/05/19 01:59:45, sclittle wrote:
> Could you just check if |config_| is non-null?

Done.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/precache_session_table.h (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/precache_session_table.h:34: class PrecacheSessionTable
{
On 2016/05/19 01:59:46, sclittle wrote:
> Maybe consider cutting out this class and just putting the implementations of
> Save/GetUnfinishedWork into PrecacheDatabase itself?

Nah. I like the separation. Makes it easier to read, maintain, and extend.

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
File components/precache/core/proto/unfinished_work.proto (right):

https://codereview.chromium.org/1961153003/diff/80001/components/precache/cor...
components/precache/core/proto/unfinished_work.proto:19: optional int32 rank =
2;
On 2016/05/19 01:59:46, sclittle wrote:
> Could you just store these in rank order instead of storing rank here?

Done.

[bengr, 2016-05-20 21:34:46]

Patchset #5 (id:140001) has been deleted

[bengr, 2016-05-20 21:34:56]

Patchset #4 (id:120001) has been deleted

[sclittle, 2016-05-20 21:57:07]

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/content/precache_manager.cc:164: history_service_->TopHosts(
nit: indentation

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.cc:328:
UMA_HISTOGRAM_PERCENTAGE("Precache.Fetch.PercentCompleted",
Can you clean up this UMA like we discussed offline?

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.cc:387:
resource_urls_to_fetch_.pop_front();
If precaching is cancelled, what happens to any resource fetches currently in
progress? From this, it looks like they'd be cancelled then never continued when
precaching is resumed.

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.h:117:
std::unique_ptr<PrecacheUnfinishedWork> CancelPrecaching();
nit: add a comment saying that nothing should be done with the PrecacheFetcher
after calling cancel

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.h:120: void AssignWorkForTest(const
std::list<GURL>& manifests,
Is this method really necessary? Couldn't you just use the normal constructor
with all of this in the UnfinishedWork?

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:138: if (!start_time.is_null()
&&
nit: could you explicitly check has_start_time instead of assuming that null is
the default value? It just seems clearer and more readable.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:140:
unfinished_work->start_time()) > base::TimeDelta::FromHours(6)) {
Instead of measuring start_time from when the config is fetched, should it
instead track when the proto is created and the hosts are added? e.g. it's
theoretically possible for the hosts to be much older than the config.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:143:
base::Bind(&PrecacheDatabase::DeleteUnfinishedWork,
Should the UnfinishedWork be deleted from the database right after you read it
in, instead of removing it here?

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/content/precache_manager_unittest.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager_unittest.cc:490:
//precache_manager_->CancelPrecaching();
nit: remove commented line?

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:263: void
PrecacheFetcher::RecordCompletionStatistics(
nit: add a // static

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:417: RecordCompletionStatistics(
nit: would it make sense to pull the RecordCompletionStatistics() and
precache_delegate_->OnDone() into another method?

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:418: *(unfinished_work_.get()),
manifest_urls_to_fetch_.size(),
nit: you don't need the .get() here

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.h:158: const std::vector<std::string>
starting_hosts_;
Isn't this in the UnfinishedWork now?

[bengr, 2016-05-21 00:16:24]

Patchset #5 (id:180001) has been deleted

[bengr, 2016-05-21 00:16:33]

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/content/precache_manager.cc:164: history_service_->TopHosts(
On 2016/05/20 21:57:06, sclittle wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.cc:328:
UMA_HISTOGRAM_PERCENTAGE("Precache.Fetch.PercentCompleted",
On 2016/05/20 21:57:07, sclittle wrote:
> Can you clean up this UMA like we discussed offline?

Done.

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.cc:387:
resource_urls_to_fetch_.pop_front();
On 2016/05/20 21:57:07, sclittle wrote:
> If precaching is cancelled, what happens to any resource fetches currently in
> progress? From this, it looks like they'd be cancelled then never continued
when
> precaching is resumed.

Done.

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.h:117:
std::unique_ptr<PrecacheUnfinishedWork> CancelPrecaching();
On 2016/05/20 21:57:07, sclittle wrote:
> nit: add a comment saying that nothing should be done with the PrecacheFetcher
> after calling cancel

Done.

https://codereview.chromium.org/1961153003/diff/100001/components/precache/co...
components/precache/core/precache_fetcher.h:120: void AssignWorkForTest(const
std::list<GURL>& manifests,
On 2016/05/20 21:57:07, sclittle wrote:
> Is this method really necessary? Couldn't you just use the normal constructor
> with all of this in the UnfinishedWork?

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:138: if (!start_time.is_null()
&&
On 2016/05/20 21:57:07, sclittle wrote:
> nit: could you explicitly check has_start_time instead of assuming that null
is
> the default value? It just seems clearer and more readable.

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:140:
unfinished_work->start_time()) > base::TimeDelta::FromHours(6)) {
On 2016/05/20 21:57:07, sclittle wrote:
> Instead of measuring start_time from when the config is fetched, should it
> instead track when the proto is created and the hosts are added? e.g. it's
> theoretically possible for the hosts to be much older than the config.

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager.cc:143:
base::Bind(&PrecacheDatabase::DeleteUnfinishedWork,
On 2016/05/20 21:57:07, sclittle wrote:
> Should the UnfinishedWork be deleted from the database right after you read it
> in, instead of removing it here?

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/content/precache_manager_unittest.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/content/precache_manager_unittest.cc:490:
//precache_manager_->CancelPrecaching();
On 2016/05/20 21:57:07, sclittle wrote:
> nit: remove commented line?

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:263: void
PrecacheFetcher::RecordCompletionStatistics(
On 2016/05/20 21:57:07, sclittle wrote:
> nit: add a // static

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:417: RecordCompletionStatistics(
On 2016/05/20 21:57:07, sclittle wrote:
> nit: would it make sense to pull the RecordCompletionStatistics() and
> precache_delegate_->OnDone() into another method?

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.cc:418: *(unfinished_work_.get()),
manifest_urls_to_fetch_.size(),
On 2016/05/20 21:57:07, sclittle wrote:
> nit: you don't need the .get() here

Done.

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
File components/precache/core/precache_fetcher.h (right):

https://codereview.chromium.org/1961153003/diff/160001/components/precache/co...
components/precache/core/precache_fetcher.h:158: const std::vector<std::string>
starting_hosts_;
On 2016/05/20 21:57:07, sclittle wrote:
> Isn't this in the UnfinishedWork now?

Done.

[sclittle, 2016-05-23 20:42:04]

LGTM % minor nit

https://codereview.chromium.org/1961153003/diff/200001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/200001/components/precache/co...
components/precache/core/precache_fetcher.cc:436: for (const auto& it :
pool_.elements()) {
nit: |it| typically stands for "iterator", could you rename this variable to
something more fitting, like |fetcher| or something?

[bengr, 2016-05-25 21:44:31]

Patchset #6 (id:220001) has been deleted

[bengr, 2016-05-25 21:44:41]

https://codereview.chromium.org/1961153003/diff/200001/components/precache/co...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/1961153003/diff/200001/components/precache/co...
components/precache/core/precache_fetcher.cc:436: for (const auto& it :
pool_.elements()) {
On 2016/05/23 20:42:04, sclittle wrote:
> nit: |it| typically stands for "iterator", could you rename this variable to
> something more fitting, like |fetcher| or something?

Done.

[bengr, 2016-05-25 21:45:06]

The CQ bit was checked by bengr@chromium.org

[bengr, 2016-05-25 21:45:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1961153003/#ps240001
(title: "Fixed nit and gn")

[commit-bot: I haz the power, 2016-05-25 21:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961153003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961153003/240001

[commit-bot: I haz the power, 2016-05-25 22:04:21]

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

[commit-bot: I haz the power, 2016-05-25 22:04:22]

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

[bengr, 2016-05-27 00:18:58]

The CQ bit was checked by bengr@chromium.org

[bengr, 2016-05-27 00:18:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1961153003/#ps260001
(title: "test fix")

[commit-bot: I haz the power, 2016-05-27 00:19:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1961153003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1961153003/260001

[commit-bot: I haz the power, 2016-05-27 02:25:10]

Description was changed from

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
==========

to

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
==========

[commit-bot: I haz the power, 2016-05-27 02:25:11]

Committed patchset #7 (id:260001)

[commit-bot: I haz the power, 2016-05-27 02:27:17]

Description was changed from

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399
==========

to

==========
Add pause/resume functionality to precache

This CL serializes an interrupted precache session
to a database and resumes on the next opportunity that
the task scheduler provides.

BUG=610399

Committed: https://crrev.com/0bb5f835faf844ba31f602f6502b7c3f8db30624
Cr-Commit-Position: refs/heads/master@{#396358}
==========

[commit-bot: I haz the power, 2016-05-27 02:27:18]

Patchset 7 (id:??) landed as
https://crrev.com/0bb5f835faf844ba31f602f6502b7c3f8db30624
Cr-Commit-Position: refs/heads/master@{#396358}