predictors: Refactor resource_prefetch_predictor triggering.

predictors: Refactor resource_prefetch_predictor triggering.

Triggering and policy move to LoadingPredictor, instead of being in
ResourcePrefetchPredictor. As a consequence, rename
ResourcePrefetchPredictorObserver.

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

[Benoit L, 2017-05-17 16:20:13]

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

[commit-bot: I haz the power, 2017-05-17 16:21:22]

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-17 17:13:30]

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

[commit-bot: I haz the power, 2017-05-17 17:13:31]

Dry run: 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_...)

[Benoit L, 2017-05-22 12:23:29]

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

[commit-bot: I haz the power, 2017-05-22 12:23:48]

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-22 12:54:01]

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

[commit-bot: I haz the power, 2017-05-22 12:54:01]

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

[Benoit L, 2017-05-22 18:05:30]

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

[commit-bot: I haz the power, 2017-05-22 18:06:08]

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-22 19:26:08]

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

[commit-bot: I haz the power, 2017-05-22 19:26:09]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-05-23 09:14:05]

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

[Benoit L, 2017-05-23 09:14:20]

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

[commit-bot: I haz the power, 2017-05-23 09:14:20]

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-23 10:42:45]

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

[commit-bot: I haz the power, 2017-05-23 10:42:46]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-05-23 11:01:39]

Thanks!
I have a couple of comments.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/net/load...
File chrome/browser/net/loading_predictor_observer.h (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/net/load...
chrome/browser/net/loading_predictor_observer.h:34: explicit
LoadingPredictorObserver(predictors::LoadingPredictor* predictor);
It obviously conflicts with the change that Trevor are making here
http://crrev.com/2896713003.
I see that the problem is that Glowplug uses the same observer for both page
loading data collection and prediction triggering.
Since GlowplugCollector is owned by LoadingPredictor, it's fine to have
LoadingPredictorObserver holding reference to LoadingPredictor. If ownership is
changed, we can add the second parameter to the constructor.

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

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:15:
base::MakeUnique<ResourcePrefetchPredictor>(config, profile)) {}
Do you think it's better to construct the pointer through the copy constructor
instead of using the constructor taking a raw pointer?

My suggestion is to use operator new:
resource_prefetch_predictor_(new ResourcePrefetchPredictor(config, profile))

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:20: if
(resource_prefetch_predictor_->CanPrefetchUrlForOrigin(url, origin))
Shouldn't it be like LoadingPredictor tries to get a prediction for Prefetch and
calls ResourcePrefetcher directly if success?
According to our plans, ResourcePrefetchPredictor should eventually disappear
and LoadingPredictor should consult right with GlowplugDatabase.
Do you think we need to keep different types of predictors as abstractions
between LoadingPredictor and Database?

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (left):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:671: void
ResourcePrefetchPredictor::StartPrefetching(const GURL& url,
Why moving code?

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:732:
StopPrefetching(nav_it->second->initial_url);
Shouldn't we move StopPrefetching outside of ResourcePrefetchPredictor as well?

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1800:
config_.IsPrefetchingEnabledForOrigin(profile_, origin) &&
I think the second condition is always true if the first one is fulfilled.
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1801:
IsUrlPrefetchable(url);
TODO:
Look at the cost of IsUrlPrefetchable() method call. We call it many times for a
single navigation (with producing the same results) and it's not trivial.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (left):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2098: //
Prefetching duration for an url without resources in the database
Why changing this test?
We wanted to record prefetching duration even when prefetching is disabled.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:260: void
ResetPredictor(int mode = kDefaultMode) {
It changes the current behavior of tests. Why not leave the default mode and
enable PREFETCHING if it's needed?

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2130: for
(const auto& kv : test_host_data_)
nit:
predictor_->host_table_cache_ = test_host_data_;

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2197: for
(const auto& kv : test_host_data_)
ditto

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:2220: for
(const auto& kv : test_host_data_)
ditto

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetcher_manager.h (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetcher_manager.h:50: virtual void
MaybeAddPrefetch(const GURL& main_frame_url,
"Virtual for testing" comment?

[Benoit L, 2017-05-29 14:34:50]

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

[commit-bot: I haz the power, 2017-05-29 14:35:10]

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-05-29 15:29:08]

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

[commit-bot: I haz the power, 2017-05-29 15:29:56]

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

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

[commit-bot: I haz the power, 2017-05-29 16:36:13]

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-05-29 16:45:14]

Thanks!

I moved more of the triggering logic out of ResourcePrefetchPredictor. More will
follow, to remove the PrefetcherManager for instance.

PTAL, thanks.

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

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:15:
base::MakeUnique<ResourcePrefetchPredictor>(config, profile)) {}
On 2017/05/23 11:01:38, alexilin wrote:
> Do you think it's better to construct the pointer through the copy constructor
> instead of using the constructor taking a raw pointer?
> 
> My suggestion is to use operator new:
> resource_prefetch_predictor_(new ResourcePrefetchPredictor(config, profile))

We are trying to remove "new" as possible from Chrome, see
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

In this case the generated code is highly likely to be the same.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_predictor.cc:20: if
(resource_prefetch_predictor_->CanPrefetchUrlForOrigin(url, origin))
On 2017/05/23 11:01:38, alexilin wrote:
> Shouldn't it be like LoadingPredictor tries to get a prediction for Prefetch
and
> calls ResourcePrefetcher directly if success?
> According to our plans, ResourcePrefetchPredictor should eventually disappear
> and LoadingPredictor should consult right with GlowplugDatabase.
> Do you think we need to keep different types of predictors as abstractions
> between LoadingPredictor and Database?

That's the eventual goal yes, but it doesn't seem that all is ready yet to get
there, hence it stays for now.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:732:
StopPrefetching(nav_it->second->initial_url);
On 2017/05/23 11:01:39, alexilin wrote:
> Shouldn't we move StopPrefetching outside of ResourcePrefetchPredictor as
well?

The unfortunate part is that without navigation tracking in LoadingPredictor, we
cannot know which prefetch to stop.
That's why the navigation prefetch is "fire and forget" for now.

Any suggestion to make it simpler in one step?

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1800:
config_.IsPrefetchingEnabledForOrigin(profile_, origin) &&
On 2017/05/23 11:01:38, alexilin wrote:
> I think the second condition is always true if the first one is fulfilled.
>
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...

Nope, the first one is true if prefetching is enabled for some origin, not for
the current one.

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:260: void
ResetPredictor(int mode = kDefaultMode) {
On 2017/05/23 11:01:39, alexilin wrote:
> It changes the current behavior of tests. Why not leave the default mode and
> enable PREFETCHING if it's needed? 

Done.

[commit-bot: I haz the power, 2017-05-29 17:45:21]

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

[commit-bot: I haz the power, 2017-05-29 17:45:22]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-05-30 11:55:27]

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:732:
StopPrefetching(nav_it->second->initial_url);
On 2017/05/29 16:45:14, Benoit L wrote:
> On 2017/05/23 11:01:39, alexilin wrote:
> > Shouldn't we move StopPrefetching outside of ResourcePrefetchPredictor as
> well?
> 
> The unfortunate part is that without navigation tracking in LoadingPredictor,
we
> cannot know which prefetch to stop.
> That's why the navigation prefetch is "fire and forget" for now.
> 
> Any suggestion to make it simpler in one step?

Answered in other comment.
TL;DR
Expose navigation tracking from ResourcePrefetchPredictor now, later this logic
will move to LoadingDataCollector.

void LoadingPredictor::CancelPageLoadHint(const GURL& url) {
  const auto& summary = resource_prefetch_predictor_->GetPageLoadSummary(url);
  auto it = active_hints_.find(summary.initial_url);
  CancelActiveHint(it);
}

https://codereview.chromium.org/2887133003/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1800:
config_.IsPrefetchingEnabledForOrigin(profile_, origin) &&
On 2017/05/29 16:45:14, Benoit L wrote:
> On 2017/05/23 11:01:38, alexilin wrote:
> > I think the second condition is always true if the first one is fulfilled.
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...
> 
> Nope, the first one is true if prefetching is enabled for some origin, not for
> the current one.

Yup, you're right.

The following paragraph is just observation that could be useful when we'll be
deleting PrefetchManager:
The opposite direction almost works as well: the first condition is true if the
second one is fulfilled except the case when ResourcePrefetchPredictor is
non-initialized or was shut down. 
So we indirectly check the initialization state of ResourcePrefetchPredictor by
checking prefetch_manager_. The class shouldn't be used after shutdown so we can
replace the first check by initialization_state_ == INITIALIZED and it should
work.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:59: active_navigations_.emplace(
Shouldn't it be a part of LoadingDataCollector? I'd prefer to avoid to have the
same functionality in different components.
It's fine to store active_hints here but active_navigations (page_loads?) should
live inside LoadingDataCollector.
Then, observers could just call CancelPageLoadHint with a url they have and
LoadingPredictor will consult LoadingDataCollector to obtain initial_url. WDYT?
For now page loads tracking is located inside ResourcePrefetchPredictor, you
could bind your code with it.

[alexilin, 2017-05-30 15:06:00]

Discussed offline:
LoadingPredictor has to know about current navigations for various reasons. For
example, it needs to cancel a hint if another main frame was requested on the
same tab before the current main frame was committed.

My proposal:
Make LoadingPredictor unaware about current navigations and redirects. Delegate
all job on cancelling hints for abandoned or committed navigations to
LoadingDataCollector. For instance, LoadingDataCollector will have the method
ClearAbandonnedNavigations from where it will cancel no longer relevant hints.
So the scheme for navigation hints will be "LoadingObserver ->
LoadingDataCollector -> LoadingPredictor"
The LoadingPredictor remains available to call for external triggering.
Please, point me if I'm missing some use-case again. I haven't found any reasons
in code why active hints should be tied with active navigations. I see only
one-way relation navigation->hint when we want to cancel an abandoned
navigation. It could be implemented inside of LoadingDataCollector.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:111: // Will to the last bucket
meaning that the duration was unlimited.
nit:
Delete the comment because it isn't relevant.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:314:
TestCanPrefetchUrlForOrigin);
Does this test still exist?

[alexilin, 2017-05-30 16:19:15]

Discussed offline the second time:
We want to give LoadingPredictor a power to decide when to cancel different
types of hints. Making it independent from LoadingPageCollector seems to be the
better design.

Please find some major and minor comments below.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
File chrome/browser/net/loading_predictor_observer.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:203:
predictor_->OnMainFrameRequest(*summary);
Requests are recorded only for main frames but probably it's worth to add the
same check for future us.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:217:
predictor_->OnMainFrameRedirect(*summary);
ditto

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:231:
predictor_->OnMainFrameResponse(*summary);
We need to check that a response actually come from a main frame. In the current
state we call it for all subresources as well.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:28: active_hints_.emplace(url,
base::TimeTicks::Now());
We used to report a hint duration only for prefetchable urls. Is this change
intended?

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:31:
!resource_prefetch_predictor_->IsUrlPrefetchable(url)) {
nit:
There is no real need to check for IsUrlPrefetchable here because if it's not
prefetchable, ResourcePrefetchPredictor just doesn't get a prediction.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:35:
resource_prefetch_predictor_->StartPrefetching(url);
Should we do something if the url is already in active_hints? We have
deduplication on ResourcePrefetcher level but we need to move it here. It's ok
for me to do it later.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:40: CancelActiveHint(it);
tiny tiny nit:
Why not in one line?
CancelActiveHint(active_hints_.find(url));

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:61:
std::make_pair(navigation_id.main_frame_url, base::TimeTicks::Now()));
The second member of the pair is unused.
You're using navigation_id.creation_time instead.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:69: navigation_id.main_frame_url
= summary.redirect_url;
What if redirect_url == main_frame_url?
I know redirecting to itself is silly but shouldn't we check that the insertion
really happened?

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:79: const auto& initial_url =
it->second.first;
nit:
s/auto/GURL/ since it's the same number of chars but GURL is easier to read

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:124:
CancelActiveHint(active_hints_.find(initial_url));
nit:
CancelPageLoadHint(initial_url) since we don't care about the return value and
to be consistent with OnMainFrameResponse()?

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:28:
content::TestBrowserThreadBundle thread_bundle_;
Nit: thread_bundle_ must be the first member.
https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:45: base::RunLoop loop;
tiny nit:
Why not base::RunLoop.RunUntilIdle() as above?

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:79:
TEST_F(LoadingPredictorTest, TestMainFrameResponseCancelHint) {
nit:
s/Cancel/Cancels/

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:90:
TEST_F(LoadingPredictorTest, TestMainFrameRequestCancelStaleNavigations) {
ditto

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:147: }  // namespace
predictors
Another useful usecase:
OnMainFrameRequest doesn't cancel previously received external hint for the same
url.

[Benoit L, 2017-05-31 14:31:51]

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

[commit-bot: I haz the power, 2017-05-31 14:32:05]

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-05-31 14:34:42]

Thanks!

All done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
File chrome/browser/net/loading_predictor_observer.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:203:
predictor_->OnMainFrameRequest(*summary);
On 2017/05/30 16:19:14, alexilin wrote:
> Requests are recorded only for main frames but probably it's worth to add the
> same check for future us.

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:217:
predictor_->OnMainFrameRedirect(*summary);
On 2017/05/30 16:19:14, alexilin wrote:
> ditto

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:231:
predictor_->OnMainFrameResponse(*summary);
On 2017/05/30 16:19:14, alexilin wrote:
> We need to check that a response actually come from a main frame. In the
current
> state we call it for all subresources as well.

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:28: active_hints_.emplace(url,
base::TimeTicks::Now());
On 2017/05/30 16:19:14, alexilin wrote:
> We used to report a hint duration only for prefetchable urls. Is this change
> intended?

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:31:
!resource_prefetch_predictor_->IsUrlPrefetchable(url)) {
On 2017/05/30 16:19:14, alexilin wrote:
> nit:
> There is no real need to check for IsUrlPrefetchable here because if it's not
> prefetchable, ResourcePrefetchPredictor just doesn't get a prediction.

Kept the check anyway, both for tracking and also to centralize triggering here.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:35:
resource_prefetch_predictor_->StartPrefetching(url);
On 2017/05/30 16:19:14, alexilin wrote:
> Should we do something if the url is already in active_hints? We have
> deduplication on ResourcePrefetcher level but we need to move it here. It's ok
> for me to do it later.

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:40: CancelActiveHint(it);
On 2017/05/30 16:19:14, alexilin wrote:
> tiny tiny nit:
> Why not in one line?
> CancelActiveHint(active_hints_.find(url));

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:59: active_navigations_.emplace(
On 2017/05/30 11:55:27, alexilin wrote:
> Shouldn't it be a part of LoadingDataCollector? I'd prefer to avoid to have
the
> same functionality in different components.
> It's fine to store active_hints here but active_navigations (page_loads?)
should
> live inside LoadingDataCollector.
> Then, observers could just call CancelPageLoadHint with a url they have and
> LoadingPredictor will consult LoadingDataCollector to obtain initial_url.
WDYT?
> For now page loads tracking is located inside ResourcePrefetchPredictor, you
> could bind your code with it.

After offline discussion, decided to keep it this way.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:61:
std::make_pair(navigation_id.main_frame_url, base::TimeTicks::Now()));
On 2017/05/30 16:19:14, alexilin wrote:
> The second member of the pair is unused.
> You're using navigation_id.creation_time instead.

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:69: navigation_id.main_frame_url
= summary.redirect_url;
On 2017/05/30 16:19:14, alexilin wrote:
> What if redirect_url == main_frame_url?
> I know redirecting to itself is silly but shouldn't we check that the
insertion
> really happened?

That's likely filtered lower in the stack (as unless the 302 is no-store, this
is an infinite loop), but more paranoia doesn't hurt indeed.
Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:79: const auto& initial_url =
it->second.first;
On 2017/05/30 16:19:14, alexilin wrote:
> nit:
> s/auto/GURL/ since it's the same number of chars but GURL is easier to read

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:111: // Will to the last bucket
meaning that the duration was unlimited.
On 2017/05/30 15:06:00, alexilin wrote:
> nit:
> Delete the comment because it isn't relevant.

I actually wanted to point that out, clarified the comment.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:124:
CancelActiveHint(active_hints_.find(initial_url));
On 2017/05/30 16:19:14, alexilin wrote:
> nit:
> CancelPageLoadHint(initial_url) since we don't care about the return value and
> to be consistent with OnMainFrameResponse()?

I would prefer to keep it as is to be consistent with the call above in the same
function.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:28:
content::TestBrowserThreadBundle thread_bundle_;
On 2017/05/30 16:19:14, alexilin wrote:
> Nit: thread_bundle_ must be the first member.
>
https://cs.chromium.org/chromium/src/content/public/test/test_browser_thread_...

Thanks!
Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:45: base::RunLoop loop;
On 2017/05/30 16:19:14, alexilin wrote:
> tiny nit:
> Why not base::RunLoop.RunUntilIdle() as above?

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:79:
TEST_F(LoadingPredictorTest, TestMainFrameResponseCancelHint) {
On 2017/05/30 16:19:14, alexilin wrote:
> nit:
> s/Cancel/Cancels/

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:90:
TEST_F(LoadingPredictorTest, TestMainFrameRequestCancelStaleNavigations) {
On 2017/05/30 16:19:14, alexilin wrote:
> ditto

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:147: }  // namespace
predictors
On 2017/05/30 16:19:14, alexilin wrote:
> Another useful usecase:
> OnMainFrameRequest doesn't cancel previously received external hint for the
same
> url.

Done.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:314:
TestCanPrefetchUrlForOrigin);
On 2017/05/30 15:06:00, alexilin wrote:
> Does this test still exist?

Done.

[alexilin, 2017-05-31 16:02:04]

lgtm, thank you!
Just a few comments and nits.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:69: navigation_id.main_frame_url
= summary.redirect_url;
On 2017/05/31 14:34:42, Benoit L wrote:
> On 2017/05/30 16:19:14, alexilin wrote:
> > What if redirect_url == main_frame_url?
> > I know redirecting to itself is silly but shouldn't we check that the
> insertion
> > really happened?
> 
> That's likely filtered lower in the stack (as unless the 302 is no-store, this
> is an infinite loop), but more paranoia doesn't hurt indeed.
> Done.

Well, a redirect response could set a new cookie so it could have some sense.

https://codereview.chromium.org/2887133003/diff/140001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.cc:111: // Will to the last bucket
meaning that the duration was unlimited.
On 2017/05/31 14:34:41, Benoit L wrote:
> On 2017/05/30 15:06:00, alexilin wrote:
> > nit:
> > Delete the comment because it isn't relevant.
> 
> I actually wanted to point that out, clarified the comment.

s/Will to/Will go to/
Initially I was confused by this comment because I didn't understand it :)

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.h:87: friend class
LoadingPredictorTest;
nit:
Usually we put friends declarations closer to the beginning of the private
section. It's up to you.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:41:
MOCK_METHOD0(StartInitialization, void());
I don't know whether there is the sense to use gmock if you don't set call
expectations. (What you have is called stub, I guess?) Wouldn't it be easier
just to override these methods with the empty body? 
I don't have a strong preference though.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:179:
EXPECT_FALSE(active_navigations.empty());
nit:
Do you have any criteria of choosing between
EXPECT_FALSE(active_navigations.empty()); and
EXPECT_NE(active_navigations.find(navigation_id), active_navigations.end());? 
It looks a little bit inconsistent for me.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:1536: return;
nit:
Delete this line.

[commit-bot: I haz the power, 2017-05-31 16:21:23]

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

[commit-bot: I haz the power, 2017-05-31 16:21:24]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-06-01 08:28:45]

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

[commit-bot: I haz the power, 2017-06-01 08:28:56]

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-06-01 08:32:48]

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

[Benoit L, 2017-06-01 08:32:50]

jkarlin@: Please review changes in chrome/browser/net and
chrome/browser/page_load_metrics, thanks!

It's mostly a rename in these directories.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
File chrome/browser/predictors/loading_predictor_unittest.cc (right):

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:41:
MOCK_METHOD0(StartInitialization, void());
On 2017/05/31 16:02:03, alexilin wrote:
> I don't know whether there is the sense to use gmock if you don't set call
> expectations. (What you have is called stub, I guess?) Wouldn't it be easier
> just to override these methods with the empty body? 
> I don't have a strong preference though.

It feels more idiomatic, and we can add expectations later without code churn.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor_unittest.cc:179:
EXPECT_FALSE(active_navigations.empty());
On 2017/05/31 16:02:03, alexilin wrote:
> nit:
> Do you have any criteria of choosing between
> EXPECT_FALSE(active_navigations.empty()); and
> EXPECT_NE(active_navigations.find(navigation_id), active_navigations.end());? 
> It looks a little bit inconsistent for me.

It is used in cases where we don't want to exactly match it. Especially since
the navigation_id is different here.

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor.cc (right):

https://codereview.chromium.org/2887133003/diff/200001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:1536: return;
On 2017/05/31 16:02:03, alexilin wrote:
> nit:
> Delete this line.

Done.

[commit-bot: I haz the power, 2017-06-01 09:53:08]

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

[commit-bot: I haz the power, 2017-06-01 09:53:09]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-06-01 12:16:39]

chrome/browser/net and chrome/browser/page_load_metrics lgtm

[Benoit L, 2017-06-01 12:29:26]

lizeb@chromium.org changed reviewers:
+ csharrison@chromium.org, msarda@chromium.org

[Benoit L, 2017-06-01 12:29:27]

+msarda: PTAL chrome/browser/profiles
+csharrison: PTAL chrome/browser/loader

Thanks!

[Charlie Harrison, 2017-06-01 12:38:41]

c/b/l lgtm

[msarda, 2017-06-01 15:55:54]

chrome/browser/profiles/* LGTM.

[Benoit L, 2017-06-01 15:58:46]

The CQ bit was checked by lizeb@chromium.org

[Benoit L, 2017-06-01 15:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexilin@chromium.org
Link to the patchset: https://codereview.chromium.org/2887133003/#ps220001
(title: ".")

[commit-bot: I haz the power, 2017-06-01 15:59:07]

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-06-01 16:02:34]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496332726846130,
"parent_rev": "0fbaeef38e204c0a97c1792689a2d0fd4fb784ea", "commit_rev":
"46c85c0e2c60c88ed671ea707bebd3d35bc4f28b"}

[commit-bot: I haz the power, 2017-06-01 16:02:44]

Description was changed from

==========
predictors: Refactor resource_prefetch_predictor triggering.

Triggering and policy move to LoadingPredictor, instead of being in
ResourcePrefetchPredictor. As a consequence, rename
ResourcePrefetchPredictorObserver.

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

to

==========
predictors: Refactor resource_prefetch_predictor triggering.

Triggering and policy move to LoadingPredictor, instead of being in
ResourcePrefetchPredictor. As a consequence, rename
ResourcePrefetchPredictorObserver.

BUG=715525

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

[commit-bot: I haz the power, 2017-06-01 16:02:49]

Committed patchset #12 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/46c85c0e2c60c88ed671ea707beb...