[Durable] Updated Durable heuristic to use 'important sites'

[Durable] Updated Durable heuristic to use 'important sites'

This also involves moving the important sites util into a location
visible to the durable storage permission context.

BUG=595433
Committed: https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121
Cr-Commit-Position: refs/heads/master@{#423695}

[dmurph, 2016-10-05 04:12:20]

dmurph@chromium.org changed reviewers:
+ dominickn@chromium.org, markusheintz@google.com, michaeln@chromium.org,
tedchoc@chromium.org

[dmurph, 2016-10-05 04:12:21]

Hello!

This updates the durable storage permission to use Important Sites as the source
of granting. PTAL, I'd like to get this in before branch time hopefully :)

Thanks!
Daniel

Reviewer coverage:

dominickn:
  chrome/browser/engagement/important_sites_util.cc
  chrome/browser/engagement/important_sites_util.h
  chrome/browser/engagement/important_sites_util_unittest.cc
  chrome/browser/engagement/site_engagement_score.h
tedchoc:
  chrome/browser/android/preferences/important_sites_util.cc
  chrome/browser/android/preferences/important_sites_util.h
  chrome/browser/android/preferences/important_sites_util_unittest.cc
  chrome/browser/android/preferences/pref_service_bridge.cc
  chrome/browser/android/preferences/website_preference_bridge.cc
markusheintz:
  components/content_settings/core/browser/website_settings_registry.cc
michaeln:
  chrome/browser/storage/durable_storage_permission_context.cc
  chrome/browser/storage/durable_storage_permission_context.h
  chrome/browser/storage/durable_storage_permission_context_unittest.cc

[Ted C, 2016-10-05 16:49:53]

On 2016/10/05 04:12:21, dmurph wrote:
> Hello!
> 
> This updates the durable storage permission to use Important Sites as the
source
> of granting. PTAL, I'd like to get this in before branch time hopefully :)
> 
> Thanks!
> Daniel
> 
> Reviewer coverage:
> 
> dominickn:
>   chrome/browser/engagement/important_sites_util.cc
>   chrome/browser/engagement/important_sites_util.h
>   chrome/browser/engagement/important_sites_util_unittest.cc
>   chrome/browser/engagement/site_engagement_score.h
> tedchoc:
>   chrome/browser/android/preferences/important_sites_util.cc
>   chrome/browser/android/preferences/important_sites_util.h
>   chrome/browser/android/preferences/important_sites_util_unittest.cc
>   chrome/browser/android/preferences/pref_service_bridge.cc
>   chrome/browser/android/preferences/website_preference_bridge.cc
> markusheintz:
>   components/content_settings/core/browser/website_settings_registry.cc
> michaeln:
>   chrome/browser/storage/durable_storage_permission_context.cc
>   chrome/browser/storage/durable_storage_permission_context.h
>   chrome/browser/storage/durable_storage_permission_context_unittest.cc

android - rs lgtm since it is a move.  I lucked out on this one :-P

[raymes, 2016-10-05 23:04:27]

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

[raymes, 2016-10-05 23:04:28]

components/content_settings/core/browser/website_settings_registry.cc lgtm

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:30: namespace {} 
// namespace
nit: is this still needed?

[michaeln, 2016-10-05 23:27:46]

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

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:340: "engagement/site_engagement_eviction_policy.h",
you have a duplicate line here, delete me plz

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
File chrome/browser/engagement/important_sites_util.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
chrome/browser/engagement/important_sites_util.cc:5: #include
<chrome/browser/engagement/important_sites_util.h>
is there a reason to use <>'s, if no please use ""'s
also typically seperate the 'main' include from the others with a blank line

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
File chrome/browser/engagement/important_sites_util_unittest.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
chrome/browser/engagement/important_sites_util_unittest.cc:5: #include
<chrome/browser/engagement/important_sites_util.h>
ditto <>'s and blank line

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:69: static const
size_t kMaxImportantResults = 10;
why static? i dont see the need for static here
and why limit it to 10?

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:82: if
(important_site.registerable_domain == registerable_domain) {
maybe not for this cl, but nice utlits could be
ImportantSitesUtil::IsOriginImportant(ori);
ImportantSitesUtil::IsRegistarableDomainImportant(string);

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:85:
SiteEngagementScore::SetParamValuesForTesting();
would it work/makesense to put this call inside of MakeOriginImportant() since
this is needed for that? the dependency between this and that, and the need for
this call, is not obvious

[dominickn, 2016-10-06 00:35:18]

engagement lgtm % michaeln's comments

[dmurph, 2016-10-06 00:58:54]

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

[commit-bot: I haz the power, 2016-10-06 00:59:45]

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

[dmurph, 2016-10-06 01:01:42]

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

[dmurph, 2016-10-06 01:02:00]

Thanks for the comments!

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

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/BUILD.gn#new...
chrome/browser/BUILD.gn:340: "engagement/site_engagement_eviction_policy.h",
On 2016/10/05 23:27:45, michaeln wrote:
> you have a duplicate line here, delete me plz

Done.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
File chrome/browser/engagement/important_sites_util.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
chrome/browser/engagement/important_sites_util.cc:5: #include
<chrome/browser/engagement/important_sites_util.h>
On 2016/10/05 23:27:45, michaeln wrote:
> is there a reason to use <>'s, if no please use ""'s
> also typically seperate the 'main' include from the others with a blank line

Done.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
File chrome/browser/engagement/important_sites_util_unittest.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/engagement/i...
chrome/browser/engagement/important_sites_util_unittest.cc:5: #include
<chrome/browser/engagement/important_sites_util.h>
On 2016/10/05 23:27:45, michaeln wrote:
> ditto <>'s and blank line

Done.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:30: namespace {} 
// namespace
On 2016/10/05 23:04:27, raymes wrote:
> nit: is this still needed?

Done.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:69: static const
size_t kMaxImportantResults = 10;
On 2016/10/05 23:27:45, michaeln wrote:
> why static? i dont see the need for static here
> and why limit it to 10?

We discussed this on the bug, we are limiting the important sites to the top 10.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context.cc:82: if
(important_site.registerable_domain == registerable_domain) {
On 2016/10/05 23:27:45, michaeln wrote:
> maybe not for this cl, but nice utlits could be
> ImportantSitesUtil::IsOriginImportant(ori);
> ImportantSitesUtil::IsRegistarableDomainImportant(string);

SGTM, I'll make that in another CL.

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/2393103002/diff/1/chrome/browser/storage/dura...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:85:
SiteEngagementScore::SetParamValuesForTesting();
On 2016/10/05 23:27:45, michaeln wrote:
> would it work/makesense to put this call inside of MakeOriginImportant() since
> this is needed for that? the dependency between this and that, and the need
for
> this call, is not obvious

Done.

[commit-bot: I haz the power, 2016-10-06 01:03: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, 2016-10-06 01:39:01]

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

[commit-bot: I haz the power, 2016-10-06 01:39:02]

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

[dmurph, 2016-10-06 18:27:56]

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

[commit-bot: I haz the power, 2016-10-06 18:28:32]

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

[michaeln, 2016-10-06 19:26:50]

lgtm!

[dmurph, 2016-10-06 20:00:03]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-10-06 20:00:09]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-10-06 20:00:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, tedchoc@chromium.org, raymes@chromium.org
Link to the patchset: https://codereview.chromium.org/2393103002/#ps60001
(title: "test & compile fix")

[commit-bot: I haz the power, 2016-10-06 20:00:37]

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

[dmurph, 2016-10-06 20:36:18]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2016-10-06 20:36:58]

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

[dmurph, 2016-10-06 20:37:30]

I had to windows fix as it had different behavior w/ sorting items that have the
same equality. No functionality change.

[dmurph, 2016-10-06 20:37:36]

The CQ bit was unchecked by dmurph@chromium.org

[commit-bot: I haz the power, 2016-10-06 20:37:41]

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

[dmurph, 2016-10-06 20:37:41]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2016-10-06 20:37:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, tedchoc@chromium.org, raymes@chromium.org,
michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/2393103002/#ps80001
(title: "windows test fix")

[commit-bot: I haz the power, 2016-10-06 21:02:05]

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-10-06 21:42:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-06 21:45:09]

Description was changed from

==========
[Durable] Updated Durable heuristic to use 'important sites'

This also involves moving the important sites util into a location
visible to the durable storage permission context.

BUG=595433
==========

to

==========
[Durable] Updated Durable heuristic to use 'important sites'

This also involves moving the important sites util into a location
visible to the durable storage permission context.

BUG=595433

Committed: https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121
Cr-Commit-Position: refs/heads/master@{#423695}
==========

[commit-bot: I haz the power, 2016-10-06 21:45:10]

Patchset 5 (id:??) landed as
https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121
Cr-Commit-Position: refs/heads/master@{#423695}