predictors: Fix duplicate content type detection.

predictors: Fix duplicate content type detection.

Content type mapping is done in two places: in
ResourcePrefetchPredictor, and in ResourcePrefetchPredictorObserver. The
first one is undesirable, since it overrides the resource type set at
URL request time.
Remove the duplication, and move the summary creation to
ResourcePrefetchPredictor. This will make it easier to add tests as
well.

BUG=631966
Committed: https://crrev.com/135b7336c5c7f1e38a88c0b89b762d335208cd6f
Cr-Commit-Position: refs/heads/master@{#412496}

[Benoit L, 2016-08-09 14:47:48]

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

[commit-bot: I haz the power, 2016-08-09 14:48:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Benoit L, 2016-08-09 16:08:28]

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

[commit-bot: I haz the power, 2016-08-09 16:19:59]

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

[commit-bot: I haz the power, 2016-08-09 16:20:00]

Dry run: This issue passed the CQ dry run.

[pasko, 2016-08-09 17:08:54]

generally looks good, just a few stylistic comments

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:96: static bool
SummarizeResponse(const net::URLRequest* request,
const net::URLRequest&

(yes, the const pointer is a majority, not sure why, it is against style guide,
here it is certainly not necessary to be a pointer)

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:97: URLRequestSummary*
summary);
Just returning a unique_ptr (instead of, ugh, output parameter) would make the
code more readable without comments and will make it easier to later remove the
evil default constructor for NavigationID.

Up to you for this small change, either to update the comment to say what this
function returns or to change the declaration.

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:99:
NavigationID navigation_id(process_id, render_frame_id,
for consistency it would be nice to do the same in
resource_prefetcher_unittest.cc

[Benoit L, 2016-08-09 17:22:56]

Thanks!

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:96: static bool
SummarizeResponse(const net::URLRequest* request,
On 2016/08/09 17:08:54, pasko wrote:
> const net::URLRequest&
> 
> (yes, the const pointer is a majority, not sure why, it is against style
guide,
> here it is certainly not necessary to be a pointer)

Unfortunately, it is:
CONTENT_EXPORT static const ResourceRequestInfo* ForRequest(const
net::URLRequest* request);

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:97: URLRequestSummary*
summary);
On 2016/08/09 17:08:54, pasko wrote:
> Just returning a unique_ptr (instead of, ugh, output parameter) would make the
> code more readable without comments and will make it easier to later remove
the
> evil default constructor for NavigationID.
> 
> Up to you for this small change, either to update the comment to say what this
> function returns or to change the declaration.

The rest of the code uses URLRequestSummary by value, and output parameters are
common in Chrome.
Added a comment.

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor_unittest.cc (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:99:
NavigationID navigation_id(process_id, render_frame_id,
On 2016/08/09 17:08:54, pasko wrote:
> for consistency it would be nice to do the same in
> resource_prefetcher_unittest.cc

Thanks for the suggestion.
Done.

[pasko, 2016-08-10 09:08:46]

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:96: static bool
SummarizeResponse(const net::URLRequest* request,
On 2016/08/09 17:22:56, Benoit L wrote:
> On 2016/08/09 17:08:54, pasko wrote:
> > const net::URLRequest&
> > 
> > (yes, the const pointer is a majority, not sure why, it is against style
> guide,
> > here it is certainly not necessary to be a pointer)
> 
> Unfortunately, it is:
> CONTENT_EXPORT static const ResourceRequestInfo* ForRequest(const
> net::URLRequest* request); 

Why would it be inconvenient to do the following?

content::ResourceRequestInfo::ForRequest(&request)

The content export is not beautiful, but let's try to make it easier to change
for better.

[Benoit L, 2016-08-10 14:08:47]

Done, thanks!

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetch_predictor.h (right):

https://codereview.chromium.org/2228003002/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetch_predictor.h:96: static bool
SummarizeResponse(const net::URLRequest* request,
On 2016/08/10 09:08:46, pasko wrote:
> On 2016/08/09 17:22:56, Benoit L wrote:
> > On 2016/08/09 17:08:54, pasko wrote:
> > > const net::URLRequest&
> > > 
> > > (yes, the const pointer is a majority, not sure why, it is against style
> > guide,
> > > here it is certainly not necessary to be a pointer)
> > 
> > Unfortunately, it is:
> > CONTENT_EXPORT static const ResourceRequestInfo* ForRequest(const
> > net::URLRequest* request); 
> 
> Why would it be inconvenient to do the following?
> 
> content::ResourceRequestInfo::ForRequest(&request)
> 
> The content export is not beautiful, but let's try to make it easier to change
> for better.

Done.

[pasko, 2016-08-10 15:10:29]

lgtm

[Benoit L, 2016-08-10 15:20:37]

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

[Benoit L, 2016-08-10 15:20:37]

Hi battre@,

The CL moves code to browser/net/predictors (and fixes a bug at the same time).
Beyond fixing a bug, this will make testing simpler (and a first round of tests
is in review).

Can you take a look at
chrome/browser/net/resource_prefetch_predictor_observer.cc?

Thanks!

[battre, 2016-08-11 14:56:14]

On 2016/08/10 15:20:37, Benoit L wrote:
> Hi battre@,
> 
> The CL moves code to browser/net/predictors (and fixes a bug at the same
time).
> Beyond fixing a bug, this will make testing simpler (and a first round of
tests
> is in review).
> 
> Can you take a look at
> chrome/browser/net/resource_prefetch_predictor_observer.cc?
> 
> Thanks!

moving code out of chrome/browser/net/resource_prefetch_predictor_observer.cc
LGTM. I did not review the rest.

[Benoit L, 2016-08-17 09:41:33]

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

[commit-bot: I haz the power, 2016-08-17 09:41:53]

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, 2016-08-17 10:22:28]

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

[commit-bot: I haz the power, 2016-08-17 10:22:29]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2016-08-17 11:14:41]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2016-08-17 11:14:56]

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, 2016-08-17 11:18:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-17 11:20:00]

Description was changed from

==========
predictors: Fix duplicate content type detection.

Content type mapping is done in two places: in
ResourcePrefetchPredictor, and in ResourcePrefetchPredictorObserver. The
first one is undesirable, since it overrides the resource type set at
URL request time.
Remove the duplication, and move the summary creation to
ResourcePrefetchPredictor. This will make it easier to add tests as
well.

BUG=631966
==========

to

==========
predictors: Fix duplicate content type detection.

Content type mapping is done in two places: in
ResourcePrefetchPredictor, and in ResourcePrefetchPredictorObserver. The
first one is undesirable, since it overrides the resource type set at
URL request time.
Remove the duplication, and move the summary creation to
ResourcePrefetchPredictor. This will make it easier to add tests as
well.

BUG=631966

Committed: https://crrev.com/135b7336c5c7f1e38a88c0b89b762d335208cd6f
Cr-Commit-Position: refs/heads/master@{#412496}
==========

[commit-bot: I haz the power, 2016-08-17 11:20:01]

Patchset 3 (id:??) landed as
https://crrev.com/135b7336c5c7f1e38a88c0b89b762d335208cd6f
Cr-Commit-Position: refs/heads/master@{#412496}