predictors: move ResourcePrefetcher handling to LoadingPredictor.

predictors: move ResourcePrefetcher handling to LoadingPredictor.

BUG=715525
Review-Url: https://codereview.chromium.org/2928033003
Cr-Commit-Position: refs/heads/master@{#484064}
Committed: https://chromium.googlesource.com/chromium/src/+/ce400eae248cb5725d8ae44672a19b5371b8d49f

[Benoit L, 2017-07-03 13:11:29]

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

[commit-bot: I haz the power, 2017-07-03 13:11:32]

Dry run: CQ is trying da patch.

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

[Benoit L, 2017-07-03 13:28:19]

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

[commit-bot: I haz the power, 2017-07-03 13:28:33]

Dry run: CQ is trying da patch.

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

[Benoit L, 2017-07-03 13:31:21]

lizeb@chromium.org changed reviewers:
+ alexilin@chromium.org

[commit-bot: I haz the power, 2017-07-03 14:44:07]

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

[commit-bot: I haz the power, 2017-07-03 14:44:08]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[alexilin, 2017-07-03 14:55:02]

Thank you! I have only some minor comments.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:66: void
LoadingPredictor::Shutdown() {
Could LoadingPredictor be destructed without a call to Shutdown()? Probably
it'll mean something bad has already happened anyway, so it's okay to destruct
prefetches on UI thread.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:73: // prefetchers must be
destroyed on the IO thread.
nit:
s/prefetchers/|prefetchers|/

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:182:
prediction.subresource_urls);
std::move(prediction.subresource_urls)
|prediction| can be moved in this function as well.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:186:
base::Unretained(prefetcher.get())));
Can you add a comment explaining that passing unretained pointer is okay in this
case because a prefetcher destruction will be scheduled on the IO thread after
that always?

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:202: // Avoid duplicates.
Can you make this comment more detailed? Something like
"Avoid to stop the same prefetcher twice."

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:213: ResourcePrefetcher*
prefetcher,
nit:
Do we really need to pass a pointer to ResourcePrefetcher? It seems unnecessary
and makes me more worried about passing raw ptrs between threads.
PrefetcherStats have a |url| field. On the other hand it could be not right to
wire business logic to stats-reporting logic.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:232:
base::Passed(std::move(it->second.first))));
Just curious, do you know whether we really need to use base::Passed()? I've
landed a CL recently where I used just std::move() without base::Passed() to
pass a unique_ptr to another thread.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.h:121: prefetches_;
I think |active_navigations_| and |prefetches_| could be merged, I don't see a
reason for them to have entries with different lifetimes.
Feel free to leave it as is. I'm going to change it in following CLs anyway.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:257: //
'ResourcePrefetchPredictorObserver' calls the below functions to inform the
s/ResourcePrefetchPredictorObserver/LoadingPredictorObserver

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(left):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:301: void
OnNavigationLearned(size_t url_visit_count,
It's not really good that we throw this check away. Could you add a
TODO(alexilin) to revisit it later?

[alexilin, 2017-07-03 15:01:37]

Could you add a bug 715525 in the CL description as well?

[Benoit L, 2017-07-03 15:41:33]

Description was changed from

==========
predictors: move ResourcePrefetcher handling to LoadingPredictor.
==========

to

==========
predictors: move ResourcePrefetcher handling to LoadingPredictor.

BUG=715525
==========

[Benoit L, 2017-07-03 15:43:15]

Thanks!
All done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:66: void
LoadingPredictor::Shutdown() {
On 2017/07/03 14:55:01, alexilin wrote:
> Could LoadingPredictor be destructed without a call to Shutdown()? Probably
> it'll mean something bad has already happened anyway, so it's okay to destruct
> prefetches on UI thread.

No, it can't.
Unless we crashed, in which case no destructor is called.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:73: // prefetchers must be
destroyed on the IO thread.
On 2017/07/03 14:55:01, alexilin wrote:
> nit:
> s/prefetchers/|prefetchers|/

Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:182:
prediction.subresource_urls);
On 2017/07/03 14:55:01, alexilin wrote:
> std::move(prediction.subresource_urls)
> |prediction| can be moved in this function as well.

Thanks!

Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:186:
base::Unretained(prefetcher.get())));
On 2017/07/03 14:55:01, alexilin wrote:
> Can you add a comment explaining that passing unretained pointer is okay in
this
> case because a prefetcher destruction will be scheduled on the IO thread after
> that always?

Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:202: // Avoid duplicates.
On 2017/07/03 14:55:01, alexilin wrote:
> Can you make this comment more detailed? Something like
> "Avoid to stop the same prefetcher twice."

Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:213: ResourcePrefetcher*
prefetcher,
On 2017/07/03 14:55:01, alexilin wrote:
> nit:
> Do we really need to pass a pointer to ResourcePrefetcher? It seems
unnecessary
> and makes me more worried about passing raw ptrs between threads.
> PrefetcherStats have a |url| field. On the other hand it could be not right to
> wire business logic to stats-reporting logic.

The lifetime of a ResourcePrefetcher is entirely controlled by this object, that
is LoadingPredictor owns the ResourcePrefetcher, so I think that passing
pointers is fine here.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:232:
base::Passed(std::move(it->second.first))));
On 2017/07/03 14:55:01, alexilin wrote:
> Just curious, do you know whether we really need to use base::Passed()? I've
> landed a CL recently where I used just std::move() without base::Passed() to
> pass a unique_ptr to another thread.

Thanks for letting me know!
Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.h:121: prefetches_;
On 2017/07/03 14:55:02, alexilin wrote:
> I think |active_navigations_| and |prefetches_| could be merged, I don't see a
> reason for them to have entries with different lifetimes.
> Feel free to leave it as is. I'm going to change it in following CLs anyway.

It does not seem entirely clear to me that merging them is nicer than leaving
them separate, so I defer to your forthcoming CL.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:257: //
'ResourcePrefetchPredictorObserver' calls the below functions to inform the
On 2017/07/03 14:55:02, alexilin wrote:
> s/ResourcePrefetchPredictorObserver/LoadingPredictorObserver

Oops, bad rebase.
Thanks!

Done.

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc
(left):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_browsertest.cc:301: void
OnNavigationLearned(size_t url_visit_count,
On 2017/07/03 14:55:02, alexilin wrote:
> It's not really good that we throw this check away. Could you add a
> TODO(alexilin) to revisit it later?

Done.

[Benoit L, 2017-07-03 15:43:22]

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

[commit-bot: I haz the power, 2017-07-03 15:43:26]

Dry run: CQ is trying da patch.

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

[alexilin, 2017-07-03 15:57:15]

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2928033003/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:182:
prediction.subresource_urls);
On 2017/07/03 15:43:14, Benoit L wrote:
> On 2017/07/03 14:55:01, alexilin wrote:
> > std::move(prediction.subresource_urls)
> > |prediction| can be moved in this function as well.
> 
> Thanks!
> 
> Done.

I'm very suspicious about moving an object (or its part) that was passed by a
const reference. Could you rewrite the code to avoid that? Probably, it would be
better to pass |prediction| here as
unique_ptr<ResourcePrefetchPredictor::Prediction>

https://codereview.chromium.org/2928033003/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2928033003/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:67:
request_queue_(urls.begin(), urls.end()),
std::move won't really help if we're copying vector content here anyway
oh, |request_queue_| is a list, feel free to drop my std::move() suggestion
above then.

[Benoit L, 2017-07-03 16:19:28]

Done, sorry about the review noise...

https://codereview.chromium.org/2928033003/diff/60001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2928033003/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher.cc:67:
request_queue_(urls.begin(), urls.end()),
On 2017/07/03 15:57:14, alexilin wrote:
> std::move won't really help if we're copying vector content here anyway
> oh, |request_queue_| is a list, feel free to drop my std::move() suggestion
> above then.

Ah, indeed, dropping the move and keeping the const refererence should keep the
same number of allocations in the end.
Done.

[alexilin, 2017-07-03 16:21:29]

lgtm, thank you!

[Benoit L, 2017-07-03 16:21:53]

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

[commit-bot: I haz the power, 2017-07-03 16:22: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-07-03 17:40:44]

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

[commit-bot: I haz the power, 2017-07-03 17:40:45]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-07-04 08:06:29]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2017-07-04 08:06:36]

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499155589948270,
"parent_rev": "08ff97980714bbfb42d9b0d50ac01feabce3e500", "commit_rev":
"ce400eae248cb5725d8ae44672a19b5371b8d49f"}

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

Description was changed from

==========
predictors: move ResourcePrefetcher handling to LoadingPredictor.

BUG=715525
==========

to

==========
predictors: move ResourcePrefetcher handling to LoadingPredictor.

BUG=715525

Review-Url: https://codereview.chromium.org/2928033003
Cr-Commit-Position: refs/heads/master@{#484064}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce400eae248cb5725d8ae44672a1...
==========

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

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/ce400eae248cb5725d8ae44672a1...