Fix precache issue with cancel and resume

Fix precache issue with cancel and resume

If precache cancel happens when all manifests are fetched
(top_hosts_to_fetch_ is empty), CancelPrecaching() saves all the
top hosts in unfinished work. In this case, unfinished work should not
have top host.

Also deletes the non-precached URLs in ClearAllForReferrerHost().

BUG=626463
Committed: https://crrev.com/c8f32980dd74319abb0aafb87cdaac4b5297df56
Cr-Commit-Position: refs/heads/master@{#420445}

[Raj, 2016-09-01 21:22:35]

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

[sclittle, 2016-09-02 18:47:56]

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:412: if
(unfinished_work_->has_config_settings()) {
Could you add a test for this behavior, e.g. when top_hosts_to_fetch_ is
non-empty but unfinished_work_->has_config_settings() is false?

That'll help prevent regressions.

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
components/precache/core/precache_url_table.cc:112: Statement delete_statement(
Should a test be added for this new functionality?

[sclittle, 2016-09-14 21:46:03]

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

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:630:
expected_url_table_map[GURL(resource.url)] = base::Time();
I'm not sure I understand. Everything that was browsed should have a null
base::Time()? Why use a null base::Time()?

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

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:387: void
RunUntilCondition(
This seems maybe a bit too complicated for test code - it almost seems as though
this test code should have its own tests. Is there some way you could design the
test so that it doesn't need this RunUntilCondition?

Even if you do need to wait for some condition to be true, could you just have a
while loop that checks the condition?

while (!should_give_up && !condition) {
  base::RunLoop().RunUntilIdle();
}

Some sort of give up condition is needed so that the test can fail instead of
just waiting infinitely.

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:389:
std::function<bool(PrecacheFetcher*)> condition_checker) {
As cool as std::function is, IIUC it's not allowed in Chromium right now. See
https://chromium-cpp.appspot.com/

If you have to use this design, then pass in a base::Callback or (better) just
use a C function pointer.

[Raj, 2016-09-15 17:02:25]

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:412: if
(unfinished_work_->has_config_settings()) {
On 2016/09/02 18:47:56, sclittle wrote:
> Could you add a test for this behavior, e.g. when top_hosts_to_fetch_ is
> non-empty but unfinished_work_->has_config_settings() is false?
> 
> That'll help prevent regressions.

Done.

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
File components/precache/core/precache_url_table.cc (right):

https://codereview.chromium.org/2300703004/diff/1/components/precache/core/pr...
components/precache/core/precache_url_table.cc:112: Statement delete_statement(
On 2016/09/02 18:47:56, sclittle wrote:
> Should a test be added for this new functionality?

Done.

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

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_database_unittest.cc:630:
expected_url_table_map[GURL(resource.url)] = base::Time();
On 2016/09/14 21:46:02, sclittle wrote:
> I'm not sure I understand. Everything that was browsed should have a null
> base::Time()? Why use a null base::Time()?

Fixed it. Changed to a different time.
The intent is to just check that the url entries are present, and no other url
entries are present.

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

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:387: void
RunUntilCondition(
On 2016/09/14 21:46:02, sclittle wrote:
> This seems maybe a bit too complicated for test code - it almost seems as
though
> this test code should have its own tests. Is there some way you could design
the
> test so that it doesn't need this RunUntilCondition?
> 
> Even if you do need to wait for some condition to be true, could you just have
a
> while loop that checks the condition?
> 
> while (!should_give_up && !condition) {
>   base::RunLoop().RunUntilIdle();
> }
> 
> Some sort of give up condition is needed so that the test can fail instead of
> just waiting infinitely.

It cannot be done as a single while loop.
The first call to RunUntilIdle() will run and finish everything.
I could not find something like RunSingleStep()

I have added a max retry as a give up condition, though the unittest will
timeout in a couple of minutes anyway. It will not be an infinite loop.

https://codereview.chromium.org/2300703004/diff/20001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:389:
std::function<bool(PrecacheFetcher*)> condition_checker) {
On 2016/09/14 21:46:02, sclittle wrote:
> As cool as std::function is, IIUC it's not allowed in Chromium right now. See
> https://chromium-cpp.appspot.com/
> 
> If you have to use this design, then pass in a base::Callback or (better) just
> use a C function pointer.

Changed using callback.
I saw std::function being used in unittests. So thought that is ok for
unittests.
std::function is convenient to define anonymous function at the same unittest
function. Otherwise they both are in different place.

[sclittle, 2016-09-21 20:27:49]

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:387:
PrecacheUnfinishedWork** cancelled_work) {
nit: could this be a pointer to a std::unique_ptr?

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1370: RunUntilCondition(
I think you could maybe replace this with something like:

while (condition) {
  base::RunLoop run_loop;
  PostTask(run_loop.QuitClosure());
  run_loop.Run();
}

That should run one step at a time like you're doing with RunUntilCondition. See
https://cs.chromium.org/chromium/src/base/run_loop.h?q=QuitClosure&sq=package...

[Raj, 2016-09-21 21:22:38]

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
File components/precache/core/precache_fetcher_unittest.cc (right):

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:387:
PrecacheUnfinishedWork** cancelled_work) {
On 2016/09/21 20:27:49, sclittle wrote:
> nit: could this be a pointer to a std::unique_ptr?

Done.

https://codereview.chromium.org/2300703004/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher_unittest.cc:1370: RunUntilCondition(
On 2016/09/21 20:27:49, sclittle wrote:
> I think you could maybe replace this with something like:
> 
> while (condition) {
>   base::RunLoop run_loop;
>   PostTask(run_loop.QuitClosure());
>   run_loop.Run();
> }
> 
> That should run one step at a time like you're doing with RunUntilCondition.
See
>
https://cs.chromium.org/chromium/src/base/run_loop.h?q=QuitClosure&sq=package...

Nice trick. Done.

[sclittle, 2016-09-22 19:49:51]

lgtm

[Raj, 2016-09-22 19:55:50]

The CQ bit was checked by rajendrant@chromium.org

[commit-bot: I haz the power, 2016-09-22 19:56:25]

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, 2016-09-22 20:37:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-22 20:40:25]

Description was changed from

==========
Fix precache issue with cancel and resume

If precache cancel happens when all manifests are fetched
(top_hosts_to_fetch_ is empty), CancelPrecaching() saves all the
top hosts in unfinished work. In this case, unfinished work should not
have top host.

Also deletes the non-precached URLs in ClearAllForReferrerHost().

BUG=626463
==========

to

==========
Fix precache issue with cancel and resume

If precache cancel happens when all manifests are fetched
(top_hosts_to_fetch_ is empty), CancelPrecaching() saves all the
top hosts in unfinished work. In this case, unfinished work should not
have top host.

Also deletes the non-precached URLs in ClearAllForReferrerHost().

BUG=626463

Committed: https://crrev.com/c8f32980dd74319abb0aafb87cdaac4b5297df56
Cr-Commit-Position: refs/heads/master@{#420445}
==========

[commit-bot: I haz the power, 2016-09-22 20:40:26]

Patchset 4 (id:??) landed as
https://crrev.com/c8f32980dd74319abb0aafb87cdaac4b5297df56
Cr-Commit-Position: refs/heads/master@{#420445}