Autogrant the durable storage permission to bookmarked origins

Autogrant the durable storage permission to bookmarked origins

Bookmarked origins still have to request the permission but will get it
without any user interaction.

BUG=533197
Committed: https://crrev.com/53fb744488a4aa0a3e3e3aa22943686da1648989
Cr-Commit-Position: refs/heads/master@{#351119}

[dgrogan, 2015-09-18 02:26:06]

dgrogan@chromium.org changed reviewers:
+ jsbell@chromium.org

[dgrogan, 2015-09-18 02:26:07]

Josh, feel free to take a look but I want to discuss the desired behavior more.

[jsbell, 2015-09-21 17:22:53]

code nits

And yeah, it's slightly odd reading the code that *any* bookmark in the origin
is treated as a signal. But we're treating origins as units of trust, so I think
that's correct. (Key thing being: it's a *signal*, and we'll refine those over
time.)

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:58:
std::vector<bookmarks::BookmarkModel::URLAndTitle> bookmarks;
nit: move into if's block

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:88:
BookmarkComparator);
This might be more readable with a lambda.

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.h:26: static bool
IsOriginBookmarked(
Only exposed for testing? If so, make it private and use FRIEND_TEST.

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:13:
std::vector<BookmarkModel::URLAndTitle> MakeBookmarks(std::string urls[],
nit: static method, const params ?

[dgrogan, 2015-09-24 20:58:57]

Take another look?

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:58:
std::vector<bookmarks::BookmarkModel::URLAndTitle> bookmarks;
On 2015/09/21 17:22:52, jsbell wrote:
> nit: move into if's block

Done.

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:88:
BookmarkComparator);
On 2015/09/21 17:22:53, jsbell wrote:
> This might be more readable with a lambda.

Done.

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.h:26: static bool
IsOriginBookmarked(
On 2015/09/21 17:22:53, jsbell wrote:
> Only exposed for testing? If so, make it private and use FRIEND_TEST.

Done.

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context_unittest.cc
(right):

https://codereview.chromium.org/1356813002/diff/20001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context_unittest.cc:13:
std::vector<BookmarkModel::URLAndTitle> MakeBookmarks(std::string urls[],
On 2015/09/21 17:22:53, jsbell wrote:
> nit: static method, const params ?

Done.

[jsbell, 2015-09-24 22:07:37]

non-owner lgtm w/ nits

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:37: if
(!requesting_origin.is_valid() || !embedding_origin.is_valid()) {
This chunk (lines 37-70) looks like it's pretty much the same as in the base
class. It'd be great to factor that out - maybe TODO/follow-up bug/CL?

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:71: BookmarkModel*
model = BookmarkModelFactory::GetForProfileIfExists(profile());
TODO for A2HS ?

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:95: return
std::binary_search(bookmarks.begin(), bookmarks.end(), looking_for,
#include <algorithm>

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:96: [](const
BookmarkModel::URLAndTitle& a,
I wonder if you could get away with auto here, but maybe that's less readable.

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.h:18: void
DecidePermission(content::WebContents* web_contents,
Can you add a short comment here w/ explanation of what the method does?

[dgrogan, 2015-09-25 20:23:43]

Josh, you are an OWNER, but I won't submit until you've taken another look.

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:37: if
(!requesting_origin.is_valid() || !embedding_origin.is_valid()) {
On 2015/09/24 22:07:36, jsbell wrote:
> This chunk (lines 37-70) looks like it's pretty much the same as in the base
> class. It'd be great to factor that out - maybe TODO/follow-up bug/CL?

Agreed. Added a TODO.

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:71: BookmarkModel*
model = BookmarkModelFactory::GetForProfileIfExists(profile());
On 2015/09/24 22:07:36, jsbell wrote:
> TODO for A2HS ?

Done.

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:95: return
std::binary_search(bookmarks.begin(), bookmarks.end(), looking_for,
On 2015/09/24 22:07:36, jsbell wrote:
> #include <algorithm>

Done.

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:96: [](const
BookmarkModel::URLAndTitle& a,
On 2015/09/24 22:07:36, jsbell wrote:
> I wonder if you could get away with auto here, but maybe that's less readable.

I fall on the "less readable" side. Further, the compiler doesn't like it.
('auto' not allowed in lambda parameter) Even further, the style guide says
"auto is permitted, for local variables only."

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.h (right):

https://codereview.chromium.org/1356813002/diff/40001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.h:18: void
DecidePermission(content::WebContents* web_contents,
On 2015/09/24 22:07:36, jsbell wrote:
> Can you add a short comment here w/ explanation of what the method does?

Done.

[jsbell, 2015-09-28 16:18:18]

still lgtm!

https://codereview.chromium.org/1356813002/diff/60001/chrome/browser/storage/...
File chrome/browser/storage/durable_storage_permission_context.cc (right):

https://codereview.chromium.org/1356813002/diff/60001/chrome/browser/storage/...
chrome/browser/storage/durable_storage_permission_context.cc:76: //
TODO(dgrogan): Remove bookmarks check in favor of Karma. In the meantime
s/Karma/site engagement/

(And maybe not remove but add, merge, etc. Comment is fine, though.)

[dgrogan, 2015-09-28 17:56:00]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-09-28 17:56:00]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1356813002/#ps80001
(title: "s/Karma/site engagement/")

[commit-bot: I haz the power, 2015-09-28 17:57:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356813002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356813002/80001

[commit-bot: I haz the power, 2015-09-28 18:16:45]

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

[commit-bot: I haz the power, 2015-09-28 18:16:45]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[dgrogan, 2015-09-28 18:29:02]

The CQ bit was checked by dgrogan@chromium.org

[dgrogan, 2015-09-28 18:29:02]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1356813002/#ps100001
(title: "EXPECT_EQ -> EXPECT_TRUE/FALSE")

[commit-bot: I haz the power, 2015-09-28 18:37:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356813002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356813002/100001

[commit-bot: I haz the power, 2015-09-28 19:39:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-28 19:40:46]

Patchset 6 (id:??) landed as
https://crrev.com/53fb744488a4aa0a3e3e3aa22943686da1648989
Cr-Commit-Position: refs/heads/master@{#351119}