predictors: Add origin learning.

predictors: Add origin learning.

This CL makes the resource predictor aware of origin requests made while
loading a page. It only adds the data collection, no fetching is enabled
yet, and the collection is not enabled outside of tests.

Changes:
- Adds a new database table to track origin requests
- Adds a new protocol buffer, loading and storing logic
- All redirects are now tracked as they cause origin requests
- No-store resources are tracked as well, only to collect origin data.

BUG=699080
Review-Url: https://codereview.chromium.org/2796783004
Cr-Commit-Position: refs/heads/master@{#464035}
Committed: https://chromium.googlesource.com/chromium/src/+/a72556982dcf08c91239167f2a5cd0e1b7f50ea8

[Benoit L, 2017-04-05 15:32:32]

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

[commit-bot: I haz the power, 2017-04-05 15:33:23]

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-04-05 16:23:14]

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

[commit-bot: I haz the power, 2017-04-05 16:23:15]

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

[Benoit L, 2017-04-10 09:36:32]

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

[Benoit L, 2017-04-10 09:36:33]

Hi alexilin@,

First CL of the origin learning in the predictors/.
It's only enabled in tests for now, and we don't do anything with the database
yet.

The rest will follow, but the CL was already large enough...

[alexilin, 2017-04-10 14:58:28]

Thanks! 

(1) Top-level comment. Don't we want to prioritize content origins over ads
ones? I suppose the most of the ads come from iframes and then...

(2) Isn't it a problem that we ignore iframes? We can cause regression for them
by replacing net predictor with Glowplug.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_common.cc:142:
is_origin_prediction_enabled(false) {}
nit:
Could you add the check of default value for |is_origin_prediction_enabled| to
resource_prefetch_common_unittest.cc?
To be precise, the function TestIsDefaultExtraConfig().

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:72: void
UpdateOrAddToSummaries(
nit:
We have a lot of different summaries. I'd propose
s/UpdateOrAddToSummaries/UpdateOrAddToOriginSummaries
or
s/UpdateOrAddToSummaries/UpdateOrAddToOrigins

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:75: GURL origin =
request_summary.resource_url.GetOrigin();
Do we need a check for incorrect url here?
From the description of GetOrigin(): "If this URL is not a standard URL, then
the result will be an empty, invalid GURL".

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:292: bool
ResourcePrefetchPredictor::ShouldRecordRedirect(
ShouldRecordRedirect and ShouldRecordResponse are the same now. Could we merge
them? At least, call the one from other.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:449: : origin(),
always_access_network(false), accessed_network(false) {}
Initializer for |first_occurrence|?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:452: const
ResourcePrefetchPredictor::OriginRequestSummary& other) = default;
nit:
No need to fully specialize the parameter name, I guess?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:462: is_no_store(false)
{}
Initializer for |network_accessed|?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:465: const
URLRequestSummary& other) = default;
Nice catch!

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:804: if
(!config_.is_origin_prediction_enabled)
nit:
Please, add DCHECK for the UI thread and the correct iniaitlization state.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:812:
UpdateOrAddToSummaries(&page_request_summary.origins, response);
Just to make it clear: do you need original url or redirected url here?
Is it acceptable to use the same logic for redirect responses? I suppose yes,
because Chrome follows cache-control directives for redirects as well.

By the way, this triggered the idea that we could consider different redirect
response codes differently for the redirect table. For example, we could be
really confident that 301 redirect is permanent.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1052: if
(url_table_cache_->find(url_spec) != url_table_cache_->end()) {
TODO for future CLs:
Use erase with iterator to avoid a second lookup in a map.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1488: for (auto& kv :
summaries) {
const auto& ?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1524:
old_origin->set_accessed_network(true);
Why not `new_origin.accessed_network`?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1575: void
ResourcePrefetchPredictor::RemoveOldestEntryInOriginDataMap(
nit:
Could you put this function before LearnNavigation() definition to correspond
declaration order in .h file? I know, it's broken for this file but at least we
could preserve the order for new functions.
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:107: struct
OriginRequestSummary {
Do we really need this struct?
Can't we derive the summary from a vector of URLRequestSummary?

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:87: message
OriginData {
Some comments to the messages?

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:88: message
OriginStat {
It could be more convenient to not nest messages. Nesting only causes
unnecessary namespaces pain.
(I'd move RedirectStat outside of RedirectData if I could make it painless)

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:221: const
std::string& host,
nit: 
You could get rid of the host parameter because the OriginData contains a host.
This is not true for a PrecacheManifest.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:383:
origin_map->insert({key, data});
Add `DCHECK_EQ(data.host(), key)`

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:43: typedef
std::unordered_map<std::string, PrefetchData> PrefetchDataMap;
Wow, this is a big change! Maybe for another CL?
At least, it's worth to mention this in a CL description.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:838:
EXPECT_TRUE(compute_score(20, 2, 12., false, false) >
nit:
EXPECT_GT(x,y) is generally better than EXPECT_TRUE(x > y)
Applicable to all expressions below as well.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:844:
EXPECT_TRUE(compute_score(20, 2, 12., false, false) ==
nit:
EXPECT_NEAR(x, y, eps)

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_test_util.cc:340:
AlmostEqual(lhs.weight_ratio(), rhs.weight_ratio());
Thanks for fixing this!

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:501:
.Times(testing::AtLeast(1));
nit:
I suppose it should be called exactly once. Get rid of Times() then?
I'll remove ambiguity for a former call in
https://codereview.chromium.org/2804283002/

[Benoit L, 2017-04-11 08:50:19]

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

[commit-bot: I haz the power, 2017-04-11 08:50:50]

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

[Benoit L, 2017-04-11 09:04:05]

Thanks for the detailed review!

All done.

About the lack of subframe tracking: this is intentional, as past experiments
suggested that we tended to create too many connections. Adding sub-frame
tracking would be easy with the code, so this leaves the door open to adding it
later.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_common.cc:142:
is_origin_prediction_enabled(false) {}
On 2017/04/10 14:58:27, alexilin wrote:
> nit:
> Could you add the check of default value for |is_origin_prediction_enabled| to
> resource_prefetch_common_unittest.cc?
> To be precise, the function TestIsDefaultExtraConfig().

Done.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:72: void
UpdateOrAddToSummaries(
On 2017/04/10 14:58:27, alexilin wrote:
> nit:
> We have a lot of different summaries. I'd propose
> s/UpdateOrAddToSummaries/UpdateOrAddToOriginSummaries
> or
> s/UpdateOrAddToSummaries/UpdateOrAddToOrigins
> 

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:75: GURL origin =
request_summary.resource_url.GetOrigin();
On 2017/04/10 14:58:28, alexilin wrote:
> Do we need a check for incorrect url here?
> From the description of GetOrigin(): "If this URL is not a standard URL, then
> the result will be an empty, invalid GURL".

Given the way this code is called, this should not happen.
Added a DCHECK() and an early return nonetheless, thanks for noticing!
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:292: bool
ResourcePrefetchPredictor::ShouldRecordRedirect(
On 2017/04/10 14:58:28, alexilin wrote:
> ShouldRecordRedirect and ShouldRecordResponse are the same now. Could we merge
> them? At least, call the one from other.

Thanks!
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:449: : origin(),
always_access_network(false), accessed_network(false) {}
On 2017/04/10 14:58:28, alexilin wrote:
> Initializer for |first_occurrence|?

Ooops... Thanks. Too bad that neither I nor the compiler catched this.
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:452: const
ResourcePrefetchPredictor::OriginRequestSummary& other) = default;
On 2017/04/10 14:58:28, alexilin wrote:
> nit:
> No need to fully specialize the parameter name, I guess?

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:462: is_no_store(false)
{}
On 2017/04/10 14:58:27, alexilin wrote:
> Initializer for |network_accessed|?

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:804: if
(!config_.is_origin_prediction_enabled)
On 2017/04/10 14:58:28, alexilin wrote:
> nit:
> Please, add DCHECK for the UI thread and the correct iniaitlization state.

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:812:
UpdateOrAddToSummaries(&page_request_summary.origins, response);
On 2017/04/10 14:58:28, alexilin wrote:
> Just to make it clear: do you need original url or redirected url here?
> Is it acceptable to use the same logic for redirect responses? I suppose yes,
> because Chrome follows cache-control directives for redirects as well.
> 
> By the way, this triggered the idea that we could consider different redirect
> response codes differently for the redirect table. For example, we could be
> really confident that 301 redirect is permanent. 

As discussed offline, we want to keep the redirected origins with the page load
data.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1488: for (auto& kv :
summaries) {
On 2017/04/10 14:58:28, alexilin wrote:
> const auto& ?

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1524:
old_origin->set_accessed_network(true);
On 2017/04/10 14:58:27, alexilin wrote:
> Why not `new_origin.accessed_network`?

Oops.
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.cc:1575: void
ResourcePrefetchPredictor::RemoveOldestEntryInOriginDataMap(
On 2017/04/10 14:58:27, alexilin wrote:
> nit:
> Could you put this function before LearnNavigation() definition to correspond
> declaration order in .h file? I know, it's broken for this file but at least
we
> could preserve the order for new functions.
>
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Done.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:107: struct
OriginRequestSummary {
On 2017/04/10 14:58:28, alexilin wrote:
> Do we really need this struct?
> Can't we derive the summary from a vector of URLRequestSummary?

You're right, this is not strictly necessary.
This is mostly here for readability, and also because we do not store all the
matching URLRequestSummary. For instance, redirects and no-store resources.

I would prefer to keep it, if you don't mind.
wdyt?

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:87: message
OriginData {
On 2017/04/10 14:58:28, alexilin wrote:
> Some comments to the messages?

Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.proto:88: message
OriginStat {
On 2017/04/10 14:58:28, alexilin wrote:
> It could be more convenient to not nest messages. Nesting only causes
> unnecessary namespaces pain.
> (I'd move RedirectStat outside of RedirectData if I could make it painless)

Done.
The changes were surprisingly minimal though (just removing two typedefs).

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:221: const
std::string& host,
On 2017/04/10 14:58:28, alexilin wrote:
> nit: 
> You could get rid of the host parameter because the OriginData contains a
host.
> This is not true for a PrecacheManifest.

I retained it for symmetry with the rest of the code, but that indeed a bit
heavy-handed.
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.cc:383:
origin_map->insert({key, data});
On 2017/04/10 14:58:28, alexilin wrote:
> Add `DCHECK_EQ(data.host(), key)`

Done.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables.h:43: typedef
std::unordered_map<std::string, PrefetchData> PrefetchDataMap;
On 2017/04/10 14:58:28, alexilin wrote:
> Wow, this is a big change! Maybe for another CL?
> At least, it's worth to mention this in a CL description.

I really dislike map<>, but unordered_map<> is not really better here,
unfortunately. Wanted to compare the two. Removed the change.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:838:
EXPECT_TRUE(compute_score(20, 2, 12., false, false) >
On 2017/04/10 14:58:28, alexilin wrote:
> nit:
> EXPECT_GT(x,y) is generally better than EXPECT_TRUE(x > y)
> Applicable to all expressions below as well.

I see from gtest that the operands no longer have a predefined meaning, so
EXPECT_TRUE() is now strictly worse.
Thanks for the suggestion.
Done.

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_tables_unittest.cc:844:
EXPECT_TRUE(compute_score(20, 2, 12., false, false) ==
On 2017/04/10 14:58:28, alexilin wrote:
> nit:
> EXPECT_NEAR(x, y, eps)

Done.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:501:
.Times(testing::AtLeast(1));
On 2017/04/10 14:58:28, alexilin wrote:
> nit:
> I suppose it should be called exactly once. Get rid of Times() then?
> I'll remove ambiguity for a former call in
> https://codereview.chromium.org/2804283002/

Done.

[alexilin, 2017-04-11 09:42:19]

LGTM % (test subresource redirect use case), thanks!

I'm wondering if we could also take in consideration "importance" of origin for
early loading stages to prioritize such origins. Probably we could use
before_fcp flag after Trevor lands his CL.

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

https://codereview.chromium.org/2796783004/diff/80001/chrome/browser/predicto...
chrome/browser/predictors/resource_prefetch_predictor.h:107: struct
OriginRequestSummary {
On 2017/04/11 09:04:05, Benoit L wrote:
> On 2017/04/10 14:58:28, alexilin wrote:
> > Do we really need this struct?
> > Can't we derive the summary from a vector of URLRequestSummary?
> 
> You're right, this is not strictly necessary.
> This is mostly here for readability, and also because we do not store all the
> matching URLRequestSummary. For instance, redirects and no-store resources.
> 
> I would prefer to keep it, if you don't mind.
> wdyt?

Sgtm. Anyway, we would have to reconstruct this map in LearnOrigins().

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

https://codereview.chromium.org/2796783004/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:681: auto
no_store = CreateURLRequestSummary(
I'd test subresource redirect case as well to be sure that we record correct
origin.

[commit-bot: I haz the power, 2017-04-11 09:42:57]

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

[commit-bot: I haz the power, 2017-04-11 09:42:58]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-04-11 14:20:13]

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

https://codereview.chromium.org/2796783004/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor_unittest.cc:681: auto
no_store = CreateURLRequestSummary(
On 2017/04/11 09:42:19, alexilin wrote:
> I'd test subresource redirect case as well to be sure that we record correct
> origin.

Thank you, turns out that it was incorrect!

[Benoit L, 2017-04-11 14:55:18]

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

[commit-bot: I haz the power, 2017-04-11 14:55:48]

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

[commit-bot: I haz the power, 2017-04-11 15:22:38]

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

[commit-bot: I haz the power, 2017-04-11 15:22:39]

Dry run: Try jobs failed on following builders:
  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-04-11 15:47:13]

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

[commit-bot: I haz the power, 2017-04-11 15:47:42]

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-04-11 16:38:49]

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

[commit-bot: I haz the power, 2017-04-11 16:38:50]

Dry run: This issue passed the CQ dry run.

[alexilin, 2017-04-12 08:21:45]

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

https://codereview.chromium.org/2796783004/diff/140001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:504: if
(request.url_chain().size() > 1)
I'd prefer just to add a new `request_url` field to URLRequestSummary. If you're
worried about the memory overhead, we could save some memory using
std::unique_ptr<GURL> for optional GURLs or even convert some GURLs to
std::string. For example, we don't need parsed GURL for `resource_url` at all.

Empty GURL object takes 104 bytes versus 8 bytes for empty std::string... 

I'd just add the new field in this CL and make memory savings in forthcoming
CLs.
======================================================================
But if you want to move this way...

I'm worried that this value will be always overwritten on a one code path and
won't be on another.
SummarizeResponse is called from
ResourcePrefetchPredictorObserver::OnRequestRedirected (1) and from
ResourcePrefetchPredictorObserver::OnResponseStarted (2). In the case (1) we'll
overwrite the value set here. In the case (2) we'll not.

Could we move this assignment to (2) after SummarizeResponse call? And add a
comment in URLRequestSummary that redirect_url is the latest url in the redirect
chain or empty if there were no redirects.

[Benoit L, 2017-04-12 12:32:40]

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

https://codereview.chromium.org/2796783004/diff/140001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:504: if
(request.url_chain().size() > 1)
On 2017/04/12 08:21:45, alexilin wrote:
> I'd prefer just to add a new `request_url` field to URLRequestSummary. If
you're
> worried about the memory overhead, we could save some memory using
> std::unique_ptr<GURL> for optional GURLs or even convert some GURLs to
> std::string. For example, we don't need parsed GURL for `resource_url` at all.
> 
> Empty GURL object takes 104 bytes versus 8 bytes for empty std::string... 
> 
> I'd just add the new field in this CL and make memory savings in forthcoming
> CLs.
> ======================================================================
> But if you want to move this way...
> 
> I'm worried that this value will be always overwritten on a one code path and
> won't be on another.
> SummarizeResponse is called from
> ResourcePrefetchPredictorObserver::OnRequestRedirected (1) and from
> ResourcePrefetchPredictorObserver::OnResponseStarted (2). In the case (1)
we'll
> overwrite the value set here. In the case (2) we'll not.
> 
> Could we move this assignment to (2) after SummarizeResponse call? And add a
> comment in URLRequestSummary that redirect_url is the latest url in the
redirect
> chain or empty if there were no redirects.

Done.

[Benoit L, 2017-04-12 13:10:54]

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

[commit-bot: I haz the power, 2017-04-12 13:11:11]

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

[alexilin, 2017-04-12 13:44:05]

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

https://codereview.chromium.org/2796783004/diff/160001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:75: const GURL&
request_url = request_summary.request_url;
No need in picking between resource_url and request_url. You can always use
request_url now and it'll be correct both for redirect and response.

request_url is an URL we've requested and redirect_url we're going to request
next if present.

https://codereview.chromium.org/2796783004/diff/160001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.cc:82: // If the response
is a redirect, then add the original origin, otherwise
request_url is exact URL that was requested so we won't update the starting
point of the redirect chain multiple times in the case of long redirect chain.

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

https://codereview.chromium.org/2796783004/diff/160001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:126: GURL request_url; 
// URL after all redirects.
I'd say it's last issued URL comparing that resource_url is the first issued
URL.

[alexilin, 2017-04-12 13:53:08]

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

https://codereview.chromium.org/2796783004/diff/160001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_predictor.h:107: struct
OriginRequestSummary {
Oh, and a comment to this class would be nice.

[commit-bot: I haz the power, 2017-04-12 13:54:11]

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

[commit-bot: I haz the power, 2017-04-12 13:54:12]

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-04-12 14:36:20]

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

[commit-bot: I haz the power, 2017-04-12 14:36:45]

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

[alexilin, 2017-04-12 14:41:22]

LGTM refreshed.

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

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

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

Dry run: This issue passed the CQ dry run.

[Benoit L, 2017-04-12 15:30:38]

The CQ bit was checked by lizeb@chromium.org

[commit-bot: I haz the power, 2017-04-12 15:31:03]

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-04-12 15:36:39]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1492011038906540,
"parent_rev": "55633a0e8db901aa6ce0990c99352b5d77087d06", "commit_rev":
"a72556982dcf08c91239167f2a5cd0e1b7f50ea8"}

[commit-bot: I haz the power, 2017-04-12 15:37:30]

Description was changed from

==========
predictors: Add origin learning.

This CL makes the resource predictor aware of origin requests made while
loading a page. It only adds the data collection, no fetching is enabled
yet, and the collection is not enabled outside of tests.

Changes:
- Adds a new database table to track origin requests
- Adds a new protocol buffer, loading and storing logic
- All redirects are now tracked as they cause origin requests
- No-store resources are tracked as well, only to collect origin data.

BUG=699080
==========

to

==========
predictors: Add origin learning.

This CL makes the resource predictor aware of origin requests made while
loading a page. It only adds the data collection, no fetching is enabled
yet, and the collection is not enabled outside of tests.

Changes:
- Adds a new database table to track origin requests
- Adds a new protocol buffer, loading and storing logic
- All redirects are now tracked as they cause origin requests
- No-store resources are tracked as well, only to collect origin data.

BUG=699080

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

[commit-bot: I haz the power, 2017-04-12 15:37:31]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/a72556982dcf08c91239167f2a5c...