Hey @Dru, important sites doesn't quite fit here anymore, right?

As in, since we limit the bookmark signal to 5, then it'll sort of limit this a bit. Perhaps we should combine important + the bookmark signal?

Last question: We limit important sites to some number, should I say, perhaps:

Durable is granted if your origin is bookmarked or if you're one of the top 50 important sites. (50? 10? 5?)

Assign back to me on your answer :)

Ah, good questions! TL;DR:

1. Let's keep the bookmark signal limit for now - so durable storage will only grant for a bookmarked site if there are less than 5 bookmarks (or if another signal holds.)

2. Let's limit the important site signal to 10 - if you're one of the top 10 important sites, that will grant durable

3. Let's make sure we're tracking how many important sites there are so we can revisit the limit in 2 later.

More reasoning:

The bookmark limit could still make sense here - if you have 100+ bookmarks, it's probably not a good signal that a site's data should be granted elevated permissions. But bookmarking is also the only deterministic way that a dev/user can ensure that a site is protected.

All of this being said, it's easier to start with restrictions and open them up than to go the other way, so for now let's keep the bookmark limit. Happy to discuss further if that doesn't seem right to you :)

In the same vain, a limit for important sites makes sense, rather than leaving it unrestricted now and adding a limit later. Let's set the limit at 10. So long as we're tracking the number of important sites, we can look later and see if the limit is helping in a meaningful way or not.

Alrighty, thanks!

Patch committed here:
https://codereview.chromium.org/2393103002/

Not sure what this wasn't put on the bug.


[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}

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ef859ea8e62837fb37e7cba6e02519b1e6f03121

commit ef859ea8e62837fb37e7cba6e02519b1e6f03121
Author: dmurph <dmurph@chromium.org>
Date: Thu Oct 06 21:42:23 2016

[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 

Review-Url: https://codereview.chromium.org/2393103002
Cr-Commit-Position: refs/heads/master@{#423695}

[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/BUILD.gn
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/android/preferences/pref_service_bridge.cc
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/android/preferences/website_preference_bridge.cc
[rename] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/engagement/important_sites_util.cc
[rename] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/engagement/important_sites_util.h
[rename] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/engagement/important_sites_util_unittest.cc
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/engagement/site_engagement_score.h
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/storage/durable_storage_permission_context.cc
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/storage/durable_storage_permission_context.h
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/browser/storage/durable_storage_permission_context_unittest.cc
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/chrome/test/BUILD.gn
[modify] https://crrev.com/ef859ea8e62837fb37e7cba6e02519b1e6f03121/components/content_settings/core/browser/website_settings_registry.cc

[Automated comment] removing mislabelled merge-merged-2840