Create LoadingDataCollector class and have observers rely on it instead of ResourcePrefetchPredictor

predictors: Create LoadingDataCollector class and have observers rely on it instead of ResourcePrefetchPredictor.

Part of a refactoring effort to split ResourcePrefetchPredictor.

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

[trevordixon, 2017-05-22 12:08:42]

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

[alexilin, 2017-05-22 13:20:36]

Thanks Trevor!

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.h:13: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
nit:
Do we need this include?

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:50:
DCHECK(collector_);
nit:
Why do not check both? The implementation detail that collector contains a
pointer to predictor shouldn't affect this code.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h:60:
predictors::ResourcePrefetchPredictor* predictor_;
note:
Pointer to ResourcePrefetchPredictor should gone after we finish StatsCollector
refactoring.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_collector.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.h:17: using
predictors::ResourcePrefetchPredictor;
Please, remove aliases and use fully-qualified names instead.

Explanation:
"Do not use Namespace aliases at namespace scope in header files except in
explicitly marked internal-only namespaces, because anything imported into a
namespace in a header file becomes part of the public API exported by that
file."
https://google.github.io/styleguide/cppguide.html#Namespaces

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.h:21: class GlowplugCollector :
public base::SupportsWeakPtr<GlowplugCollector> {
URLRequestSummary and other *Summary classes also should be moved outside of
resource_prefetch_predictor. And I'd prefer to not nest them inside of any
class.
I'm okay with doing that in forthcoming CL.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.h:21: class GlowplugCollector :
public base::SupportsWeakPtr<GlowplugCollector> {
This class needs the detailed description in a comment.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
chrome/browser/predictors/loading_predictor.h:55:
std::unique_ptr<GlowplugCollector> glowplug_collector_;
We need to decide whether GlowplugCollector should be independent KeyedService
or it's okay for LoadingPredictor to own it. I don't have a strong preference. 
lizeb@ WDYT?

[Benoit L, 2017-05-22 13:37:16]

Thanks!

A few comments.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:973: "predictors/glowplug_collector.cc",
General comment: since most of the classes are now using loading_* as a name, it
probably makes sense to follow this pattern here as well.

That is, what about LoadingDataCollector?

Sorry about this change, happened while you were working on this CL.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:102: if
(!predictors::GlowplugCollector::ShouldRecordRequest(request,
nit: why not
"using predictors::LoadingDataCollector"
at the top of this file?

Then you can keep the code as it was, with the name change.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:136: if
(!predictors::GlowplugCollector::ShouldRecordRedirect(request))
nit: ditto.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_collector.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.cc:14: bool
GlowplugCollector::ShouldRecordRequest(
nit: Can you migrate the logic, instead of just forwarding?

It was probably planned for a future CL, but we might as well do it here as
well.

It's probably not easy to migrate it all, but at least some of it should be
possible to migrate, especially these static methods.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.cc:30:
////////////////////////////////////////////////////////////////////////////////
nit: Remove this.
It was in the code initially, and we didn't remove it, but I would prefer not to
introduce these comments in new code.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
chrome/browser/predictors/loading_predictor.h:55:
std::unique_ptr<GlowplugCollector> glowplug_collector_;
On 2017/05/22 13:20:36, alexilin wrote:
> We need to decide whether GlowplugCollector should be independent KeyedService
> or it's okay for LoadingPredictor to own it. I don't have a strong preference.

> lizeb@ WDYT?

As its lifetime is 100% tied to LoadingPredictor, I would tend to keep it owned
by LoadingPredictor, at least for now.
If at some point we have a reason to have the lifetimes diverge, we can still
make it a service then.

But I'm not 100% set on this either :-)

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:394:
resource_prefetch_predictor_observer_.reset(
nit: as you're touching this code, can you convert it to the new hotness,

a_unique_ptr.reset(new FooBar())
--->
a_unique_ptr = base::MakeUnique<FooBar>();

[trevordixon, 2017-05-22 21:29:01]

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.h:13: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
On 2017/05/22 at 13:20:36, alexilin wrote:
> nit:
> Do we need this include?

Gone.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/page_load_me...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc:50:
DCHECK(collector_);
On 2017/05/22 at 13:20:36, alexilin wrote:
> nit:
> Why do not check both? The implementation detail that collector contains a
pointer to predictor shouldn't affect this code.

You're right. I changed that blindly without real intent.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_collector.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.h:17: using
predictors::ResourcePrefetchPredictor;
On 2017/05/22 at 13:20:36, alexilin wrote:
> Please, remove aliases and use fully-qualified names instead.
> 
> Explanation:
> "Do not use Namespace aliases at namespace scope in header files except in
explicitly marked internal-only namespaces, because anything imported into a
namespace in a header file becomes part of the public API exported by that
file."
> https://google.github.io/styleguide/cppguide.html#Namespaces

Done.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.h:21: class GlowplugCollector :
public base::SupportsWeakPtr<GlowplugCollector> {
On 2017/05/22 at 13:20:36, alexilin wrote:
> This class needs the detailed description in a comment.

Done.

[alexilin, 2017-05-23 08:24:37]

lgtm % lizeb@ comments
Thanks!

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
File chrome/browser/predictors/loading_predictor.h (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/l...
chrome/browser/predictors/loading_predictor.h:55:
std::unique_ptr<GlowplugCollector> glowplug_collector_;
On 2017/05/22 13:37:16, Benoit L wrote:
> On 2017/05/22 13:20:36, alexilin wrote:
> > We need to decide whether GlowplugCollector should be independent
KeyedService
> > or it's okay for LoadingPredictor to own it. I don't have a strong
preference.
> 
> > lizeb@ WDYT?
> 
> As its lifetime is 100% tied to LoadingPredictor, I would tend to keep it
owned
> by LoadingPredictor, at least for now.
> If at some point we have a reason to have the lifetimes diverge, we can still
> make it a service then.
> 
> But I'm not 100% set on this either :-)

SGTM
Just one point in favor of separate lifetimes is that in LEARNING mode we would
be able to use GlowplugCollector without constructing LoadingPredictor.

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/glowplug_collector.h (right):

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:22: explicit
GlowplugCollector(predictors::ResourcePrefetchPredictor* predictor);
nit:
Sorry that I didn't notice it the first time, but you don't have to specify
`predictors::` namespace because you're already inside it.
So you just could use `ResourcePrefetchPredictor` here and
`ResourcePrefetchPredictor::URLRequestSummary` below.

[pasko, 2017-05-23 12:24:58]

pasko@chromium.org changed reviewers:
+ pasko@chromium.org

[pasko, 2017-05-23 12:24:59]

fly by look without understanding details.. pardon for possible inconvenience

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/glowplug_collector.h (right):

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:17: // Records navigation events
as reported by various observers to the database
Does this say that the observers of the database record navigation events? I
thought it's the other way around - navigation observers are used for recording
events and updating the database.

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:20: class GlowplugCollector :
public base::SupportsWeakPtr<GlowplugCollector> {
please try to avoid base::SupportsWeakPtr. See a discussion about it:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/LNK43wiinNo/nsUX2...

if there is absolutely no way to avoid weak refs, WeakPtrFactory is considered
more explicit - less surprising with inheritance, so it makes sense to just
convert to it for consistency reasons, even if no further inheritance for this
class is planned

[trevordixon, 2017-05-23 13:05:15]

Left to do: move logic from old class into this new one.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:973: "predictors/glowplug_collector.cc",
On 2017/05/22 at 13:37:16, Benoit L wrote:
> General comment: since most of the classes are now using loading_* as a name,
it probably makes sense to follow this pattern here as well.
> 
> That is, what about LoadingDataCollector?
> 
> Sorry about this change, happened while you were working on this CL.

OK, renamed.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
File chrome/browser/net/resource_prefetch_predictor_observer.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:102: if
(!predictors::GlowplugCollector::ShouldRecordRequest(request,
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: why not
> "using predictors::LoadingDataCollector"
> at the top of this file?
> 
> Then you can keep the code as it was, with the name change.

Done.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/net/resource...
chrome/browser/net/resource_prefetch_predictor_observer.cc:136: if
(!predictors::GlowplugCollector::ShouldRecordRedirect(request))
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: ditto.

Done.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_collector.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.cc:30:
////////////////////////////////////////////////////////////////////////////////
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: Remove this.
> It was in the code initially, and we didn't remove it, but I would prefer not
to introduce these comments in new code.

Done.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:394:
resource_prefetch_predictor_observer_.reset(
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: as you're touching this code, can you convert it to the new hotness,
> 
> a_unique_ptr.reset(new FooBar())
> --->
> a_unique_ptr = base::MakeUnique<FooBar>();

Done.

[trevordixon, 2017-05-24 20:39:24]

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
File chrome/browser/predictors/glowplug_collector.cc (right):

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.cc:14: bool
GlowplugCollector::ShouldRecordRequest(
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: Can you migrate the logic, instead of just forwarding?
> 
> It was probably planned for a future CL, but we might as well do it here as
well.
> 
> It's probably not easy to migrate it all, but at least some of it should be
possible to migrate, especially these static methods.

Moved the static methods over. I'll look into the others next.

https://codereview.chromium.org/2896713003/diff/1/chrome/browser/predictors/g...
chrome/browser/predictors/glowplug_collector.cc:14: bool
GlowplugCollector::ShouldRecordRequest(
On 2017/05/22 at 13:37:16, Benoit L wrote:
> nit: Can you migrate the logic, instead of just forwarding?
> 
> It was probably planned for a future CL, but we might as well do it here as
well.
> 
> It's probably not easy to migrate it all, but at least some of it should be
possible to migrate, especially these static methods.

Moved the static methods over. I'll look into the others next.

[Benoit L, 2017-05-29 08:55:37]

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:31: class
ResourcePrefetchPredictorObserver {
Can you change this name as well, since it no longer references
ResourcePrefetchPredictor?

LoadingPredictorObserver?

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

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:6: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
nit: unnecessary include.

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

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:451: void
ResourcePrefetchPredictor::SetAllowPortInUrlsForTesting(bool state) {
Do the tests still pass?

I think this function is used in the tests, and since it moved to another class,
and is private, you probably need to update the tests.

[trevordixon, 2017-06-02 12:09:48]

On 2017/05/29 at 08:55:37, lizeb wrote:
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
> File chrome/browser/net/resource_prefetch_predictor_observer.h (right):
> 
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
> chrome/browser/net/resource_prefetch_predictor_observer.h:31: class
ResourcePrefetchPredictorObserver {
> Can you change this name as well, since it no longer references
ResourcePrefetchPredictor?
> 
> LoadingPredictorObserver?
> 
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
> File chrome/browser/predictors/loading_data_collector.cc (right):
> 
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
> chrome/browser/predictors/loading_data_collector.cc:6: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
> nit: unnecessary include.
> 
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
> File chrome/browser/predictors/resource_prefetch_predictor.cc (left):
> 
>
https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
> chrome/browser/predictors/resource_prefetch_predictor.cc:451: void
ResourcePrefetchPredictor::SetAllowPortInUrlsForTesting(bool state) {
> Do the tests still pass?
> 
> I think this function is used in the tests, and since it moved to another
class, and is private, you probably need to update the tests.

Tests fixed.

[trevordixon, 2017-06-02 12:10:36]

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
File chrome/browser/net/resource_prefetch_predictor_observer.h (right):

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/net/reso...
chrome/browser/net/resource_prefetch_predictor_observer.h:31: class
ResourcePrefetchPredictorObserver {
On 2017/05/29 at 08:55:36, Benoit L wrote:
> Can you change this name as well, since it no longer references
ResourcePrefetchPredictor?
> 
> LoadingPredictorObserver?

Looks like you did it!

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

https://codereview.chromium.org/2896713003/diff/60001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:6: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
On 2017/05/29 at 08:55:36, Benoit L wrote:
> nit: unnecessary include.

Gone.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc:44:
collector_ = base::MakeUnique<LoadingDataCollector>(predictor_.get());
Is this OK?

[Benoit L, 2017-06-02 12:44:00]

Thanks, some questions and comments.

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

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:219:
predictor_->resource_prefetch_predictor()->RecordURLRedirect(*summary);
Shouldn't this be removed?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.cc:118:
predictor_->RecordURLRequest(request);
Can you make these functions private in ResourcePrefetchPredictor to make sure
that we haven't missed call sites?

(related to the comment above)

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector.h (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.h:33: //
'LoadingPredictorObserver' calls the below functions to inform the
and LoadingPredictorTabHelper, right?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:17: #include
"chrome/browser/predictors/resource_prefetch_predictor_tables.h"
Is this include needed?

Please run "git cl lint", although I'm not sure it would point out unnecessary
includes.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:51:
std::unique_ptr<net::URLRequest> CreateURLRequest(
See comment below, is it possible to avoid duplicating this function?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:75:
LoadingPredictorConfig config;
There is now PopulateTestConfig() to replace this code.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:88:
predictor_.reset(new ResourcePrefetchPredictor(config, profile_.get()));
nit:

predictor_ = base::MakeUnique<>()...

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:116:
ResetPredictor();
nit: These two functions are called only once, inline?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:121:
histogram_tester_.reset(new base::HistogramTester());
nit:

histogram_tester_ = base::MakeUnique<base::HistogramTester>();

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:129:
TEST_F(LoadingDataCollectorTest, HandledResourceTypes) {
Is it correct that all the tests below only test static functions?

If so, you don't need any of the machinery above, right?

I suspect you will eventually need some of it at least once the function
actually doing the recording are moved to LoadingDataCollector, but it seems
strange to have it without any use.

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

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:282: friend class
LoadingDataCollectorTest;
Is this needed?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_test_util.h:128: void
set_response_info(const net::HttpResponseInfo& response_info);
nit: you can inline trivial setters.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (left):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:246:
std::unique_ptr<net::URLRequest> CreateURLRequest(
This is also duplicated in the tests, is it avoidable?

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:50: class
EmptyURLRequestDelegate : public net::URLRequest::Delegate {
This class is now duplicated, can you move it to a common test header file, even
though it's trivial?

[trevordixon, 2017-06-06 13:08:04]

One thing not addressed yet: need to move duplicated CreateURLRequest to
loading_test_util.

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

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:219:
predictor_->resource_prefetch_predictor()->RecordURLRedirect(*summary);
On 2017/06/02 at 12:43:59, Benoit L wrote:
> Shouldn't this be removed?

Yes. Merge mistake.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.cc:118:
predictor_->RecordURLRequest(request);
On 2017/06/02 at 12:43:59, Benoit L wrote:
> Can you make these functions private in ResourcePrefetchPredictor to make sure
that we haven't missed call sites?
> 
> (related to the comment above)

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector.h (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.h:33: //
'LoadingPredictorObserver' calls the below functions to inform the
On 2017/06/02 at 12:43:59, Benoit L wrote:
> and LoadingPredictorTabHelper, right?

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:17: #include
"chrome/browser/predictors/resource_prefetch_predictor_tables.h"
On 2017/06/02 at 12:43:59, Benoit L wrote:
> Is this include needed?
> 
> Please run "git cl lint", although I'm not sure it would point out unnecessary
includes.

I think I got rid of all the unnecessary ones.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:75:
LoadingPredictorConfig config;
On 2017/06/02 at 12:43:59, Benoit L wrote:
> There is now PopulateTestConfig() to replace this code.

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:88:
predictor_.reset(new ResourcePrefetchPredictor(config, profile_.get()));
On 2017/06/02 at 12:43:59, Benoit L wrote:
> nit:
> 
> predictor_ = base::MakeUnique<>()...

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:116:
ResetPredictor();
On 2017/06/02 at 12:43:59, Benoit L wrote:
> nit: These two functions are called only once, inline?

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:121:
histogram_tester_.reset(new base::HistogramTester());
On 2017/06/02 at 12:43:59, Benoit L wrote:
> nit:
> 
> histogram_tester_ = base::MakeUnique<base::HistogramTester>();

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:129:
TEST_F(LoadingDataCollectorTest, HandledResourceTypes) {
On 2017/06/02 at 12:43:59, Benoit L wrote:
> Is it correct that all the tests below only test static functions?
> 
> If so, you don't need any of the machinery above, right?
> 
> I suspect you will eventually need some of it at least once the function
actually doing the recording are moved to LoadingDataCollector, but it seems
strange to have it without any use.

You're right. At one point I had the RecordFirstContentfulPaint test here, but
removing that meant I should have gotten rid of a bunch of stuff up there.

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

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:282: friend class
LoadingDataCollectorTest;
On 2017/06/02 at 12:43:59, Benoit L wrote:
> Is this needed?

No longer.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_test_util.h (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_test_util.h:128: void
set_response_info(const net::HttpResponseInfo& response_info);
On 2017/06/02 at 12:44:00, Benoit L wrote:
> nit: you can inline trivial setters.

Done.

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/120001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:50: class
EmptyURLRequestDelegate : public net::URLRequest::Delegate {
On 2017/06/02 at 12:44:00, Benoit L wrote:
> This class is now duplicated, can you move it to a common test header file,
even though it's trivial?

Done.

[Benoit L, 2017-06-06 14:10:56]

Thanks!

Only two really minor comments, should be good to go after the next revision.

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

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:24: using
predictors::ResourcePrefetchPredictor;
nit: Is it still necessary?

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:33:
url_request_context_.set_job_factory(&url_request_job_factory_);
Actually, this code is a bit fragile.
Can you also reset url_request_job_factory in each test?

We are using setters in url_request_job_factory and don't reset the object
between tests.

[trevordixon, 2017-06-07 09:14:27]

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

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/net/loa...
chrome/browser/net/loading_predictor_observer.cc:24: using
predictors::ResourcePrefetchPredictor;
On 2017/06/06 at 14:10:55, Benoit L wrote:
> nit: Is it still necessary?

Deleted.

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/180001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:33:
url_request_context_.set_job_factory(&url_request_job_factory_);
On 2017/06/06 at 14:10:55, Benoit L wrote:
> Actually, this code is a bit fragile.
> Can you also reset url_request_job_factory in each test?
> 
> We are using setters in url_request_job_factory and don't reset the object
between tests.

OK. Not certain I did what you had in mind; have a look.

[Benoit L, 2017-06-07 09:49:18]

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

[commit-bot: I haz the power, 2017-06-07 09:49:44]

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-06-07 09:52:08]

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

[commit-bot: I haz the power, 2017-06-07 09:52:09]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Benoit L, 2017-06-07 12:01:06]

Thanks!

lgtm % the small comments (the first one is more a question actually).

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:36:
content::TestBrowserThreadBundle thread_bundle_;
nit: Is this needed?

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
File chrome/browser/predictors/loading_test_util.cc (right):

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.cc:16:
predictors::EmptyURLRequestDelegate empty_url_request_delegate;
nit: g_empty_url_request_delegate.

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
File chrome/browser/predictors/loading_test_util.h (right):

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:87: class EmptyURLRequestDelegate
: public net::URLRequest::Delegate {
nit: Since this is only referred to in the .cc file, can you move it there?

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

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:101: //  
ResourcePrefetchPredictor on the UI thread. This is owned by the
nit: LoadingDataCollector

[Benoit L, 2017-06-07 12:02:09]

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

[commit-bot: I haz the power, 2017-06-07 12:02:35]

Dry run: CQ is trying da patch.

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

[trevordixon, 2017-06-07 12:37:15]

Thank you!

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
File chrome/browser/predictors/loading_data_collector_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:36:
content::TestBrowserThreadBundle thread_bundle_;
On 2017/06/07 at 12:01:05, Benoit L wrote:
> nit: Is this needed?

Yes, the tests crash without this member. But the LoadingDataCollectorTest
constructor doesn't seem to be necessary, so I deleted that.

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
File chrome/browser/predictors/loading_test_util.h (right):

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:87: class EmptyURLRequestDelegate
: public net::URLRequest::Delegate {
On 2017/06/07 at 12:01:05, Benoit L wrote:
> nit: Since this is only referred to in the .cc file, can you move it there?

Done.

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

https://codereview.chromium.org/2896713003/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:101: //  
ResourcePrefetchPredictor on the UI thread. This is owned by the
On 2017/06/07 at 12:01:06, Benoit L wrote:
> nit: LoadingDataCollector

Done.

[alexilin, 2017-06-07 12:37:32]

Still lgtm.
I'm leaving some optional comments.

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:128:
ResourcePrefetchPredictorPageLoadMetricsObserver::CreateIfNeeded(
nit:
We could rename ResourcePrefetchPredictorPageLoadMetricsObserver class to
LoadingPredictorPageLoadMetricsObserver as well.

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

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.h:56: ResourcePrefetchPredictor*
resource_prefetch_predictor() const;
nit:
These methods should be non-const because we return non-const handles to
internal data of object.
More context:
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:117: class
MockLoadingPredictorObserver : public TestObserver {
nit:
It seems that this renaming wasn't really intended. This observer still observes
ResourcePrefetchPredictor. We'll refactor it soon anyway so you can leave it as
is.

[trevordixon, 2017-06-07 13:17:55]

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_initialize.cc (right):

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_initialize.cc:128:
ResourcePrefetchPredictorPageLoadMetricsObserver::CreateIfNeeded(
On 2017/06/07 at 12:37:31, alexilin wrote:
> nit:
> We could rename ResourcePrefetchPredictorPageLoadMetricsObserver class to
LoadingPredictorPageLoadMetricsObserver as well.

OK, done.

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

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
chrome/browser/predictors/loading_predictor.h:56: ResourcePrefetchPredictor*
resource_prefetch_predictor() const;
On 2017/06/07 at 12:37:31, alexilin wrote:
> nit:
> These methods should be non-const because we return non-const handles to
internal data of object.
> More context:
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...

Done.

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2896713003/diff/240001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:117: class
MockLoadingPredictorObserver : public TestObserver {
On 2017/06/07 at 12:37:31, alexilin wrote:
> nit:
> It seems that this renaming wasn't really intended. This observer still
observes ResourcePrefetchPredictor. We'll refactor it soon anyway so you can
leave it as is.

Oops! As easy as it is, I might as well rename it back.

[trevordixon, 2017-06-07 18:19:14]

Description was changed from

==========
Create GlowplugCollector class and have observers rely on it instead of
ResourcePrefetchPredictor.

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

to

==========
Create LoadingDataCollector class and have observers rely on it instead of
ResourcePrefetchPredictor.

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

[trevordixon, 2017-06-07 18:19:15]

trevordixon@chromium.org changed reviewers:
+ asvitkine@chromium.org, jkarlin@chromium.org

[trevordixon, 2017-06-08 11:45:44]

Description was changed from

==========
Create LoadingDataCollector class and have observers rely on it instead of
ResourcePrefetchPredictor.

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

to

==========
predictors: Create LoadingDataCollector class and have observers rely on it
instead of ResourcePrefetchPredictor.

Part of a refactoring effort to split ResourcePrefetchPredictor.

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

[trevordixon, 2017-06-08 11:48:12]

To jkarlin and asvitkine for owners' review.

jkarlin:
  chrome/browser/net/loading_predictor_observer.cc
 
chrome/browser/page_load_metrics/observers/loading_predictor_page_load_metrics_observer.cc
 
chrome/browser/page_load_metrics/observers/loading_predictor_page_load_metrics_observer.h
 
chrome/browser/page_load_metrics/observers/loading_predictor_page_load_metrics_observer_unittest.cc
 
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.cc
 
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer.h
 
chrome/browser/page_load_metrics/observers/resource_prefetch_predictor_page_load_metrics_observer_unittest.cc
  chrome/browser/page_load_metrics/page_load_metrics_initialize.cc

asvitkine:
  tools/metrics/histograms/histograms.xml

Thank you!

[jkarlin, 2017-06-08 13:17:45]

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

[Alexei Svitkine (slow), 2017-06-08 14:25:19]

lgtm

[trevordixon, 2017-06-09 04:46:10]

The CQ bit was checked by trevordixon@chromium.org

[trevordixon, 2017-06-09 04:46:10]

The patchset sent to the CQ was uploaded after l-g-t-m from lizeb@chromium.org,
alexilin@chromium.org
Link to the patchset: https://codereview.chromium.org/2896713003/#ps280001
(title: "Address alexilin comments.")

[commit-bot: I haz the power, 2017-06-09 04:46:26]

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-09 04:48:37]

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

[commit-bot: I haz the power, 2017-06-09 04:48:39]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[trevordixon, 2017-06-09 11:02:07]

The CQ bit was checked by trevordixon@chromium.org

[trevordixon, 2017-06-09 11:02:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexilin@chromium.org, lizeb@chromium.org, jkarlin@chromium.org,
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2896713003/#ps300001
(title: "Rebase")

[commit-bot: I haz the power, 2017-06-09 11:02:16]

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-09 12:21:35]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1497006127747980,
"parent_rev": "79e65182ac5735c9ebe072dbaf2f19a10f1e6475", "commit_rev":
"3eb67a8404b818203d0498faf86438681c4a6c97"}

[commit-bot: I haz the power, 2017-06-09 12:21:48]

Description was changed from

==========
predictors: Create LoadingDataCollector class and have observers rely on it
instead of ResourcePrefetchPredictor.

Part of a refactoring effort to split ResourcePrefetchPredictor.

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

to

==========
predictors: Create LoadingDataCollector class and have observers rely on it
instead of ResourcePrefetchPredictor.

Part of a refactoring effort to split ResourcePrefetchPredictor.

BUG=715525

Review-Url: https://codereview.chromium.org/2896713003
Cr-Commit-Position: refs/heads/master@{#478254}
Committed:
https://chromium.googlesource.com/chromium/src/+/3eb67a8404b818203d0498faf864...
==========

[commit-bot: I haz the power, 2017-06-09 12:21:49]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/3eb67a8404b818203d0498faf864...

[pasko, 2017-06-12 14:06:57]

trevordixon: please respond to unanswered comments in this review. It is the
responsibility of the author of the change to make sure that all comments are
addressed before landing.

I am seeing these unanswered comments (maybe there are more, I have not
checked):
https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...

Sorry for this codereview tool making it super easy to miss comments being
posted. We will have the new tool soon, where missing comments will also be
easy, but in a different way :)

[pasko, 2017-06-12 14:08:13]

On 2017/06/12 14:06:57, pasko wrote:
> trevordixon: please respond to unanswered comments in this review.

oh, and feel free to spawn a followup change if necessary, put the commenters as
reviewers.

[alexilin, 2017-06-12 14:18:35]

Please, notice, that I've already sent a patch that fixes base::SupportsWeakPtr
usage.
https://chromium-review.googlesource.com/c/530747/

[trevordixon, 2017-06-13 11:40:03]

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
File chrome/browser/predictors/glowplug_collector.h (right):

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:17: // Records navigation events
as reported by various observers to the database
On 2017/05/23 at 12:24:59, pasko wrote:
> Does this say that the observers of the database record navigation events? I
thought it's the other way around - navigation observers are used for recording
events and updating the database.

I'll refactor the sentence to make the meaning clear in a followup CL.

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:20: class GlowplugCollector :
public base::SupportsWeakPtr<GlowplugCollector> {
On 2017/05/23 at 12:24:59, pasko wrote:
> please try to avoid base::SupportsWeakPtr. See a discussion about it:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/LNK43wiinNo/nsUX2...
> 
> if there is absolutely no way to avoid weak refs, WeakPtrFactory is considered
more explicit - less surprising with inheritance, so it makes sense to just
convert to it for consistency reasons, even if no further inheritance for this
class is planned

Sorry for missing this comment! Thankfully Alex took care of this and other
similar instances. https://chromium-review.googlesource.com/c/530747/

https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
chrome/browser/predictors/glowplug_collector.h:22: explicit
GlowplugCollector(predictors::ResourcePrefetchPredictor* predictor);
On 2017/05/23 at 08:24:37, alexilin wrote:
> nit:
> Sorry that I didn't notice it the first time, but you don't have to specify
`predictors::` namespace because you're already inside it.
> So you just could use `ResourcePrefetchPredictor` here and
`ResourcePrefetchPredictor::URLRequestSummary` below.

Sorry for missing this comment. I'll fix this in a followup CL.

[trevordixon, 2017-06-13 11:41:24]

On 2017/06/12 at 14:06:57, pasko wrote:
> trevordixon: please respond to unanswered comments in this review. It is the
responsibility of the author of the change to make sure that all comments are
addressed before landing.
> 
> I am seeing these unanswered comments (maybe there are more, I have not
checked):
https://codereview.chromium.org/2896713003/diff/20001/chrome/browser/predicto...
> 
> Sorry for this codereview tool making it super easy to miss comments being
posted. We will have the new tool soon, where missing comments will also be
easy, but in a different way :)

Sorry for missing those comments! I confirmed that all other problems were
addressed (if not responded to; I'll be careful to also respond more thoroughly
in the future).