predictors: Move more methods from ResourcePrefetchPredictor into LoadingDataCollector.

predictors: Move more methods from ResourcePrefetchPredictor into LoadingDataCollector.

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

[trevordixon, 2017-06-14 08:04:33]

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

[trevordixon, 2017-06-14 08:04:34]

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:23:
predictors::ResourcePrefetchPredictor::PageRequestSummary;
Might make sense to move PageRequestSummary into loading_predictor.h or
loading_data_collector.h?

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:156:
ResourcePrefetchPredictor::INITIALIZED)
I copied all of the initialization_state checks over, so the logic hasn't
changed at all, but I think it might not be necessary. I think the only place
where the predictor's initialization_state matters is in
LoadingDataCollector::OnNavigationComplete, before calling
ResourcePrefetchPredictor::HandlePageRequestSummary. Can this check be moved
into either LoadingDataCollector::OnNavigationComplete or
ResourcePrefetchPredictor::HandlePageRequestSummary?

[alexilin, 2017-06-14 11:41:54]

Looks good except initialization logic leaked into LoadingCollectorClass.

Thanks!

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:23:
predictors::ResourcePrefetchPredictor::PageRequestSummary;
On 2017/06/14 08:04:34, trevordixon wrote:
> Might make sense to move PageRequestSummary into loading_predictor.h or
> loading_data_collector.h?

I'd suggest you to move all of OriginRequestSummary, URLRequestSummary and
PageRequestSummary into loading_data_collector.h. And I think it'd be even
better to make them non-nested.

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:156:
ResourcePrefetchPredictor::INITIALIZED)
On 2017/06/14 08:04:34, trevordixon wrote:
> I copied all of the initialization_state checks over, so the logic hasn't
> changed at all, but I think it might not be necessary. I think the only place
> where the predictor's initialization_state matters is in
> LoadingDataCollector::OnNavigationComplete, before calling
> ResourcePrefetchPredictor::HandlePageRequestSummary. Can this check be moved
> into either LoadingDataCollector::OnNavigationComplete or
> ResourcePrefetchPredictor::HandlePageRequestSummary?

Yeah, initialization is only needed for ResourcePrefetchPredictor.
LoadingDataCollector has nothing to initialize. So I think it'll be clearer to
delete all initialization-related logic from this class and keep it inside
ResourcePrefetchPredictor. LoadingDataCollector shouldn't care about what's
initialization state of ResourcePrefetchPredictor, that's my opinion.

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:52: virtual void
RecordFirstContentfulPaint(
Why virtual?

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:87: // this point are the
only ones considered for prefetching.
s/for prefetching//

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:93: typedef std::map<
This formatting looks strange. Did you run `git cl format`?

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:320: void
HandlePageRequestSummary(std::unique_ptr<PageRequestSummary> summary);
s/HandlePageRequestSummary/RecordPageRequestSummary
It should be a public method.

[trevordixon, 2017-06-16 05:11:28]

Have another look. Still disentangling tests.

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:23:
predictors::ResourcePrefetchPredictor::PageRequestSummary;
On 2017/06/14 at 11:41:54, alexilin wrote:
> On 2017/06/14 08:04:34, trevordixon wrote:
> > Might make sense to move PageRequestSummary into loading_predictor.h or
> > loading_data_collector.h?
> 
> I'd suggest you to move all of OriginRequestSummary, URLRequestSummary and
PageRequestSummary into loading_data_collector.h. And I think it'd be even
better to make them non-nested.

Done.

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:156:
ResourcePrefetchPredictor::INITIALIZED)
On 2017/06/14 at 11:41:54, alexilin wrote:
> On 2017/06/14 08:04:34, trevordixon wrote:
> > I copied all of the initialization_state checks over, so the logic hasn't
> > changed at all, but I think it might not be necessary. I think the only
place
> > where the predictor's initialization_state matters is in
> > LoadingDataCollector::OnNavigationComplete, before calling
> > ResourcePrefetchPredictor::HandlePageRequestSummary. Can this check be moved
> > into either LoadingDataCollector::OnNavigationComplete or
> > ResourcePrefetchPredictor::HandlePageRequestSummary?
> 
> Yeah, initialization is only needed for ResourcePrefetchPredictor.
LoadingDataCollector has nothing to initialize. So I think it'll be clearer to
delete all initialization-related logic from this class and keep it inside
ResourcePrefetchPredictor. LoadingDataCollector shouldn't care about what's
initialization state of ResourcePrefetchPredictor, that's my opinion.

Done.

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:52: virtual void
RecordFirstContentfulPaint(
On 2017/06/14 at 11:41:54, alexilin wrote:
> Why virtual?

So a MockLoadingDataCollector can override it. Does it need to be virtual in
that case?

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:87: // this point are the
only ones considered for prefetching.
On 2017/06/14 at 11:41:54, alexilin wrote:
> s/for prefetching//

Done.

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:93: typedef std::map<
On 2017/06/14 at 11:41:54, alexilin wrote:
> This formatting looks strange. Did you run `git cl format`?

Yes, this is what it did!

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:320: void
HandlePageRequestSummary(std::unique_ptr<PageRequestSummary> summary);
On 2017/06/14 at 11:41:54, alexilin wrote:
> s/HandlePageRequestSummary/RecordPageRequestSummary
> It should be a public method.

Done.

[trevordixon, 2017-06-16 05:35:55]

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:288:
std::unique_ptr<PageRequestSummary> summary) {
Especially review what I did here. I copied the initialization check logic from 
ResourcePrefetchPredictor::RecordMainFrameLoadComplete
(https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...).

So before the flow was ResourcePrefetchPredictor::RecordMainFrameComplete ->
ResourcePrefetchPredictor::OnNavigationComplete, and this initialization check
was is RecordMainFrameComplete.

Now it goes LoadingDataCollector::RecordMainFrameComplete ->
ResourcePrefetchPredictor::HandlePageRequestSummary, and the initialization
check is in HandlePageRequestSummary.

[alexilin, 2017-06-16 09:31:57]

Thanks! It remains only to fix the tests.
Please add a link to the bug 715525 into the CL description.

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:52: virtual void
RecordFirstContentfulPaint(
On 2017/06/16 05:11:28, trevordixon wrote:
> On 2017/06/14 at 11:41:54, alexilin wrote:
> > Why virtual?
> 
> So a MockLoadingDataCollector can override it. Does it need to be virtual in
> that case?

Yes, it has to be virtual to be overriden by mock class. You just haven't added
unittests to this CL yet so I decided to clarify.

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:93: typedef std::map<
On 2017/06/16 05:11:28, trevordixon wrote:
> On 2017/06/14 at 11:41:54, alexilin wrote:
> > This formatting looks strange. Did you run `git cl format`?
> 
> Yes, this is what it did!

Ok, it's much better after stripping namespaces!

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:320: void
HandlePageRequestSummary(std::unique_ptr<PageRequestSummary> summary);
On 2017/06/16 05:11:28, trevordixon wrote:
> On 2017/06/14 at 11:41:54, alexilin wrote:
> > s/HandlePageRequestSummary/RecordPageRequestSummary
> > It should be a public method.
> 
> Done.

You didn't rename the method :)

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/net/load...
chrome/browser/net/loading_predictor_observer.cc:25: using URLRequestSummary =
predictors::URLRequestSummary;
nit:
Why not `using predictors::URLRequestSummary;`?

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:294:
OnMainFrameRequest(request);
nit:
Could we just inline this function here?

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:302:
OnSubresourceResponse(response);
nit:
ditto

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:167: typedef
std::map<NavigationID, std::unique_ptr<PageRequestSummary>>
nit:
using NavigationMap = std::map<NavigationID,
std::unique_ptr<PageRequestSummary>> - I'd prefer that we use C++11 style type
aliasing;
and put it in the beginning of the private session according to the style guide:
https://google.github.io/styleguide/cppguide.html#Declaration_Order

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_stats_collector.h:15: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
nit:
We can use just a forward declaration of ResourcePrefetchPredictor now.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:288:
std::unique_ptr<PageRequestSummary> summary) {
On 2017/06/16 05:35:55, trevordixon wrote:
> Especially review what I did here. I copied the initialization check logic
from 
> ResourcePrefetchPredictor::RecordMainFrameLoadComplete
>
(https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...).
> 
> So before the flow was ResourcePrefetchPredictor::RecordMainFrameComplete ->
> ResourcePrefetchPredictor::OnNavigationComplete, and this initialization check
> was is RecordMainFrameComplete.
> 
> Now it goes LoadingDataCollector::RecordMainFrameComplete ->
> ResourcePrefetchPredictor::HandlePageRequestSummary, and the initialization
> check is in HandlePageRequestSummary.

Thanks! I think it's acceptable solution for now. 
The only difference from the previous code is that LoadingDataCollector collects
page load data even if the database hasn't been initialized yet that turns out
to be a wasted effort. In the future we probably would like to start the
database initialization earlier.

[trevordixon, 2017-06-16 09:34:07]

Description was changed from

==========
predictors: Move more methods from ResourcePrefetchPredictor into
LoadingDataCollector.

BUG=
==========

to

==========
predictors: Move more methods from ResourcePrefetchPredictor into
LoadingDataCollector.

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

[trevordixon, 2017-06-16 10:53:20]

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

https://codereview.chromium.org/2937623007/diff/40001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:320: void
HandlePageRequestSummary(std::unique_ptr<PageRequestSummary> summary);
On 2017/06/16 at 09:31:57, alexilin wrote:
> On 2017/06/16 05:11:28, trevordixon wrote:
> > On 2017/06/14 at 11:41:54, alexilin wrote:
> > > s/HandlePageRequestSummary/RecordPageRequestSummary
> > > It should be a public method.
> > 
> > Done.
> 
> You didn't rename the method :)

Oops, missed that line! Done.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/net/load...
chrome/browser/net/loading_predictor_observer.cc:25: using URLRequestSummary =
predictors::URLRequestSummary;
On 2017/06/16 at 09:31:57, alexilin wrote:
> nit:
> Why not `using predictors::URLRequestSummary;`?

Done.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:294:
OnMainFrameRequest(request);
On 2017/06/16 at 09:31:57, alexilin wrote:
> nit:
> Could we just inline this function here?

Yes.

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.cc:302:
OnSubresourceResponse(response);
On 2017/06/16 at 09:31:57, alexilin wrote:
> nit:
> ditto

Done.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_data_collector.h:167: typedef
std::map<NavigationID, std::unique_ptr<PageRequestSummary>>
On 2017/06/16 at 09:31:57, alexilin wrote:
> nit:
> using NavigationMap = std::map<NavigationID,
std::unique_ptr<PageRequestSummary>> - I'd prefer that we use C++11 style type
aliasing;
> and put it in the beginning of the private session according to the style
guide: https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/loading_stats_collector.h:15: #include
"chrome/browser/predictors/resource_prefetch_predictor.h"
On 2017/06/16 at 09:31:57, alexilin wrote:
> nit:
> We can use just a forward declaration of ResourcePrefetchPredictor now.

Done.

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

https://codereview.chromium.org/2937623007/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:288:
std::unique_ptr<PageRequestSummary> summary) {
On 2017/06/16 at 09:31:57, alexilin wrote:
> On 2017/06/16 05:35:55, trevordixon wrote:
> > Especially review what I did here. I copied the initialization check logic
from 
> > ResourcePrefetchPredictor::RecordMainFrameLoadComplete
> >
(https://cs.chromium.org/chromium/src/chrome/browser/predictors/resource_prefe...).
> > 
> > So before the flow was ResourcePrefetchPredictor::RecordMainFrameComplete ->
> > ResourcePrefetchPredictor::OnNavigationComplete, and this initialization
check
> > was is RecordMainFrameComplete.
> > 
> > Now it goes LoadingDataCollector::RecordMainFrameComplete ->
> > ResourcePrefetchPredictor::HandlePageRequestSummary, and the initialization
> > check is in HandlePageRequestSummary.
> 
> Thanks! I think it's acceptable solution for now. 
> The only difference from the previous code is that LoadingDataCollector
collects page load data even if the database hasn't been initialized yet that
turns out to be a wasted effort. In the future we probably would like to start
the database initialization earlier.

👍

[trevordixon, 2017-06-19 20:41:08]

(Still in the middle, so not in a very reviewable state yet, but I have one
question.)

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

https://codereview.chromium.org/2937623007/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:280: // Single
navigation but history count is low, so should not record url data.
Copied these tests without changing names, updating comments, or ensuring
they're all still necessary. Now that nothing db-related is tested, I think a
few of these first 6 may be redundant, and all of them need their names changed:

NavigationLowHistoryCount
NavigationUrlNotInDB
NavigationUrlInDB
NavigationUrlNotInDBAndDBFull
RedirectUrlNotInDB
RedirectUrlInDB

These 6 are all testing the code in pretty much the same way. Maybe I can just
keep NavigationUrlNotInDB and RedirectUrlNotInDB and change their names?

[alexilin, 2017-06-20 09:00:19]

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

https://codereview.chromium.org/2937623007/diff/120001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:280: // Single
navigation but history count is low, so should not record url data.
On 2017/06/19 20:41:08, trevordixon wrote:
> Copied these tests without changing names, updating comments, or ensuring
> they're all still necessary. Now that nothing db-related is tested, I think a
> few of these first 6 may be redundant, and all of them need their names
changed:
> 
> NavigationLowHistoryCount
> NavigationUrlNotInDB
> NavigationUrlInDB
> NavigationUrlNotInDBAndDBFull
> RedirectUrlNotInDB
> RedirectUrlInDB
> 
> These 6 are all testing the code in pretty much the same way. Maybe I can just
> keep NavigationUrlNotInDB and RedirectUrlNotInDB and change their names?

Yes, I agree with your plan. At first, I've thought that the test methods 

OnMainFrameRequest
OnMainFrameRedirect
OnSubresourceResponse
TestRecordFirstContentfulPaint

would be enough but then I've realized that we don't record a single navigation
there. So tests that record navigation with/without a redirect are needed.
We don't need to keep all these 6 tests because they don't depend on the state
of the database now, you're right.

[trevordixon, 2017-06-20 11:07:27]

On 2017/06/20 at 09:00:19, alexilin wrote:
>
https://codereview.chromium.org/2937623007/diff/120001/chrome/browser/predict...
> File chrome/browser/predictors/loading_data_collector_unittest.cc (right):
> 
>
https://codereview.chromium.org/2937623007/diff/120001/chrome/browser/predict...
> chrome/browser/predictors/loading_data_collector_unittest.cc:280: // Single
navigation but history count is low, so should not record url data.
> On 2017/06/19 20:41:08, trevordixon wrote:
> > Copied these tests without changing names, updating comments, or ensuring
> > they're all still necessary. Now that nothing db-related is tested, I think
a
> > few of these first 6 may be redundant, and all of them need their names
changed:
> > 
> > NavigationLowHistoryCount
> > NavigationUrlNotInDB
> > NavigationUrlInDB
> > NavigationUrlNotInDBAndDBFull
> > RedirectUrlNotInDB
> > RedirectUrlInDB
> > 
> > These 6 are all testing the code in pretty much the same way. Maybe I can
just
> > keep NavigationUrlNotInDB and RedirectUrlNotInDB and change their names?
> 
> Yes, I agree with your plan. At first, I've thought that the test methods 
> 
> OnMainFrameRequest
> OnMainFrameRedirect
> OnSubresourceResponse
> TestRecordFirstContentfulPaint
> 
> would be enough but then I've realized that we don't record a single
navigation there. So tests that record navigation with/without a redirect are
needed.
> We don't need to keep all these 6 tests because they don't depend on the state
of the database now, you're right.

Done.

[trevordixon, 2017-06-20 11:08:51]

Almost done. Some ResourcePrefetchPredictorTest failures still.

[trevordixon, 2017-06-22 12:33:43]

Tests are ready for review now. Have a look.

[alexilin, 2017-06-22 19:27:20]

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

[commit-bot: I haz the power, 2017-06-22 19:27:39]

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-22 19:30:38]

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

[commit-bot: I haz the power, 2017-06-22 19:30:39]

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

[alexilin, 2017-06-22 19:39:06]

Haven't finished to look at the code yet, but all bots are failing because you
need to rebase.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.cc:366: LOG(WARNING) <<
"predictor_:  " << predictor_;
Please, remove all logging statements.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:34: LOG(WARNING) <<
"RecordPageRequestSummary";
ditto

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:71: class
MockResourcePrefetchPredictorTables
Why did you move it here? As I can see ResourcePrefetchPredictorTest is the only
client for this mock.

[alexilin, 2017-06-23 16:55:05]

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.h:136: virtual void
RecordFirstContentfulPaint(
nit:
You aren't mocking this class so there is no need to make this method virtual.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.cc:489: lhs.subresource_requests ==
rhs.subresource_requests;
It's seems like we need to compare lhs.origins and rhs.origins here.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1633:
TEST_F(ResourcePrefetchPredictorTest, TestRecordFirstContentfulPaint) {
nit:
Could you add some simple test to check that FCP is actually written to the
database?

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:710:
URLRequestSummary fb1 = CreateURLRequestSummary(1, "http://fb.com/google");
nit:
fb1, fb2, fb3, fb_end are not used. Feel free to delete.

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:756:
URLRequestSummary fb1 = CreateURLRequestSummary(1, "http://fb.com/google");
nit:
ditto

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1018:
TEST_F(ResourcePrefetchPredictorTest, SummarizeResponse) {
nit:
LoadingDataCollectorTest is the better place for all Summarize* tests.

[trevordixon, 2017-06-27 21:28:55]

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.cc:366: LOG(WARNING) <<
"predictor_:  " << predictor_;
On 2017/06/22 at 19:39:05, alexilin wrote:
> Please, remove all logging statements.

Done.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector.h:136: virtual void
RecordFirstContentfulPaint(
On 2017/06/23 at 16:55:05, alexilin wrote:
> nit:
> You aren't mocking this class so there is no need to make this method virtual.

Done.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.cc:489: lhs.subresource_requests ==
rhs.subresource_requests;
On 2017/06/23 at 16:55:05, alexilin wrote:
> It's seems like we need to compare lhs.origins and rhs.origins here.

Done.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:34: LOG(WARNING) <<
"RecordPageRequestSummary";
On 2017/06/22 at 19:39:05, alexilin wrote:
> ditto

Done.

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/loading_test_util.h:71: class
MockResourcePrefetchPredictorTables
On 2017/06/22 at 19:39:05, alexilin wrote:
> Why did you move it here? As I can see ResourcePrefetchPredictorTest is the
only client for this mock.

You're right, moved back.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1633:
TEST_F(ResourcePrefetchPredictorTest, TestRecordFirstContentfulPaint) {
On 2017/06/23 at 16:55:05, alexilin wrote:
> nit:
> Could you add some simple test to check that FCP is actually written to the
database?

Still working on this.

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

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:710:
URLRequestSummary fb1 = CreateURLRequestSummary(1, "http://fb.com/google");
On 2017/06/23 at 16:55:05, alexilin wrote:
> nit:
> fb1, fb2, fb3, fb_end are not used. Feel free to delete.

Done.

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:756:
URLRequestSummary fb1 = CreateURLRequestSummary(1, "http://fb.com/google");
On 2017/06/23 at 16:55:05, alexilin wrote:
> nit:
> ditto

Done.

https://codereview.chromium.org/2937623007/diff/220001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1018:
TEST_F(ResourcePrefetchPredictorTest, SummarizeResponse) {
On 2017/06/23 at 16:55:05, alexilin wrote:
> nit:
> LoadingDataCollectorTest is the better place for all Summarize* tests.

Done.

[trevordixon, 2017-06-27 21:29:46]

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1690:
TEST_F(ResourcePrefetchPredictorTest, TestRecordFirstContentfulPaint) {
Still need to test that fcp is recorded in db.

[alexilin, 2017-06-30 13:37:11]

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

[commit-bot: I haz the power, 2017-06-30 13:37:15]

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

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

[commit-bot: I haz the power, 2017-06-30 13:40:06]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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_...)

[alexilin, 2017-06-30 14:58:49]

I'm okay with adding a test for ResourcePrefetchPredictor recording fcp later,
the CL is already big.

lgtm % comments below, thanks!

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:41: base::RunLoop
loop;
nit:
These two statements could be combined into one.
base::RunLoop().RunUntilIdle();

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:268: static bool
IsNoStore(const net::URLRequest& request);
Implementation of this function was moved into anonymous namespace in
loading_data_collector.cc
Please, remove the declaration from here.

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1086: //
TEST_F(ResourcePrefetchPredictorTest, SummarizeResponse) {
Don't forget to remove these comments.

[trevordixon, 2017-07-11 11:08:08]

Thanks!

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/loading_data_collector_unittest.cc:41: base::RunLoop
loop;
On 2017/06/30 at 14:58:49, alexilin wrote:
> nit:
> These two statements could be combined into one.
> base::RunLoop().RunUntilIdle();

Done.

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:268: static bool
IsNoStore(const net::URLRequest& request);
On 2017/06/30 at 14:58:49, alexilin wrote:
> Implementation of this function was moved into anonymous namespace in
loading_data_collector.cc
> Please, remove the declaration from here.

Done.

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

https://codereview.chromium.org/2937623007/diff/260001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:1086: //
TEST_F(ResourcePrefetchPredictorTest, SummarizeResponse) {
On 2017/06/30 at 14:58:49, alexilin wrote:
> Don't forget to remove these comments.

Done.

[trevordixon, 2017-07-11 11:10:11]

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

[trevordixon, 2017-07-11 11:11:15]

To jkarlin for owner review.

  chrome/browser/net/loading_predictor_observer.cc
  chrome/browser/net/loading_predictor_observer.h
 
chrome/browser/page_load_metrics/observers/loading_predictor_page_load_metrics_observer_unittest.cc

[jkarlin, 2017-07-11 11:21:45]

net and page_load_metrics lgtm

[trevordixon, 2017-07-11 11:32:14]

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

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

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-11 11:45:22]

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

[commit-bot: I haz the power, 2017-07-11 11:45:23]

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

[trevordixon, 2017-07-11 11:57:55]

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

[commit-bot: I haz the power, 2017-07-11 11:58:07]

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-11 12:31:37]

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

[commit-bot: I haz the power, 2017-07-11 12:31:38]

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

[trevordixon, 2017-07-11 13:21:05]

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

[commit-bot: I haz the power, 2017-07-11 13:21:09]

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-11 14:25:27]

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

[commit-bot: I haz the power, 2017-07-11 14:25:28]

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_...)

[trevordixon, 2017-07-13 07:48:58]

The CQ bit was checked by trevordixon@chromium.org

[trevordixon, 2017-07-13 07:48:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexilin@chromium.org, jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2937623007/#ps320001
(title: "Fix browser test")

[commit-bot: I haz the power, 2017-07-13 07:49:19]

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-13 09:26:51]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1499932138556280,
"parent_rev": "1d53084e955fd1af88f2c5aebdb9e8ab167b01e1", "commit_rev":
"d87657cd9251e9c60a1c8df464df6d032e91530c"}

[commit-bot: I haz the power, 2017-07-13 09:27:51]

Description was changed from

==========
predictors: Move more methods from ResourcePrefetchPredictor into
LoadingDataCollector.

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

to

==========
predictors: Move more methods from ResourcePrefetchPredictor into
LoadingDataCollector.

BUG=715525

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

[commit-bot: I haz the power, 2017-07-13 09:27:53]

Committed patchset #17 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/d87657cd9251e9c60a1c8df464df...