Fix PrecacheFetcher behavior when reaching max_bytes.

Fix PrecacheFetcher behavior when reaching max_bytes.

1. Report PercentCompleted properly, instead of as 100%.
2. Load manifests in the order that they were passed in starting_hosts
   (that is, by number of visits descending).

BUG=309216
Committed: https://crrev.com/c595787f571c3345e44ebc80181e6280be2f2a7f
Cr-Commit-Position: refs/heads/master@{#386209}

[twifkak, 2016-04-05 05:10:37]

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

[jamartin, 2016-04-05 13:53:36]

You may need to sync to HEAD and check the interaction with the parallel
fetches.

[twifkak, 2016-04-05 23:02:39]

On 2016/04/05 13:53:36, jamartin1 wrote:
> You may need to sync to HEAD and check the interaction with the parallel
> fetches.

Done.

[bengr, 2016-04-08 18:24:35]

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

https://codereview.chromium.org/1859023002/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher.cc:359: precache_delegate_->OnDone();
You might want to also clear the lists, else the eventual pause/resume CL might
get confused.

https://codereview.chromium.org/1859023002/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher.cc:401: const std::string manifest_url
= ConstructManifestURL(prefix, host);
I wonder if it makes sense to have a helper function in an anonymous namespace,
e.g.:

void AddUniqueURLsToList(const std::vector<std::string>& urls_or_hosts,
                         std::list<GURL>* url_list);

[twifkak, 2016-04-08 19:09:05]

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

https://codereview.chromium.org/1859023002/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher.cc:359: precache_delegate_->OnDone();
On 2016/04/08 18:24:35, bengr wrote:
> You might want to also clear the lists, else the eventual pause/resume CL
might
> get confused.

I can't clear the lists. That's what this CL is about. The lists are left in
tact so that PercentCompleted is computed correctly in the destructor.

As the code is currently architected, client code should construct a new
PrecacheFetcher each time it wants to fetch, and destroy immediately afterwards,
as the destructor logs all the relevant UMAs. I actually think that's a good fit
for the eventual pause/resume CL, but I could be wrong.

Otherwise, we can't clear the lists until after the UMAs are logged, or we'll
have to change the way PercentCompleted is computed.

https://codereview.chromium.org/1859023002/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher.cc:401: const std::string manifest_url
= ConstructManifestURL(prefix, host);
On 2016/04/08 18:24:35, bengr wrote:
> I wonder if it makes sense to have a helper function in an anonymous
namespace,
> e.g.:
> 
> void AddUniqueURLsToList(const std::vector<std::string>& urls_or_hosts,
>                          std::list<GURL>* url_list);

Yeah, I was on the fence about that, but your comment pushes me over the fence.
Done.

[bengr, 2016-04-08 21:25:48]

lgtm

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

https://codereview.chromium.org/1859023002/diff/40001/components/precache/cor...
components/precache/core/precache_fetcher.cc:359: precache_delegate_->OnDone();
On 2016/04/08 19:09:05, twifkak wrote:
> On 2016/04/08 18:24:35, bengr wrote:
> > You might want to also clear the lists, else the eventual pause/resume CL
> might
> > get confused.
> 
> I can't clear the lists. That's what this CL is about. The lists are left in
> tact so that PercentCompleted is computed correctly in the destructor.
> 
> As the code is currently architected, client code should construct a new
> PrecacheFetcher each time it wants to fetch, and destroy immediately
afterwards,
> as the destructor logs all the relevant UMAs. I actually think that's a good
fit
> for the eventual pause/resume CL, but I could be wrong.
> 
> Otherwise, we can't clear the lists until after the UMAs are logged, or we'll
> have to change the way PercentCompleted is computed.

I know that's what the CL is about. What you have is fine, but it might not
interact well with the pause/resume CL, but that's on my plate. :)

[twifkak, 2016-04-08 21:29:29]

The CQ bit was checked by twifkak@chromium.org

[commit-bot: I haz the power, 2016-04-08 21:29:53]

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

[commit-bot: I haz the power, 2016-04-08 21:35:26]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-08 21:36:39]

Description was changed from

==========
Fix PrecacheFetcher behavior when reaching max_bytes.

1. Report PercentCompleted properly, instead of as 100%.
2. Load manifests in the order that they were passed in starting_hosts
   (that is, by number of visits descending).

BUG=309216
==========

to

==========
Fix PrecacheFetcher behavior when reaching max_bytes.

1. Report PercentCompleted properly, instead of as 100%.
2. Load manifests in the order that they were passed in starting_hosts
   (that is, by number of visits descending).

BUG=309216

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

[commit-bot: I haz the power, 2016-04-08 21:36:40]

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