Switching ExtensionSpecialStoragePolicy::IsStorageDurable to use DurableStoragePermissionContext.

Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman

Committed: https://crrev.com/4a19a66d254415f7afb67d6f3c6e6354f280109d
Cr-Commit-Position: refs/heads/master@{#424165}

[dougt, 2016-09-19 17:26:17]

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

[commit-bot: I haz the power, 2016-09-19 17:26:49]

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-09-19 17:41:22]

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

[commit-bot: I haz the power, 2016-09-19 17:41:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dougt, 2016-09-19 20:19:39]

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

[commit-bot: I haz the power, 2016-09-19 20:20:06]

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-09-19 20:30:49]

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

[commit-bot: I haz the power, 2016-09-19 20:30:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dgrogan, 2016-09-19 20:52:42]

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

[dgrogan, 2016-09-19 20:52:43]

Seems reasonable, as long as you can get the unit tests to make sense, but
you'll definitely want to get raymes to review, he's the content_settings
person.

[dougt, 2016-09-19 20:54:48]

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

[commit-bot: I haz the power, 2016-09-19 20:55:34]

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-09-19 21:05:37]

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

[commit-bot: I haz the power, 2016-09-19 21:05:38]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[dougt, 2016-09-19 21:19:45]

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

[commit-bot: I haz the power, 2016-09-19 21:20:28]

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-09-19 22:00:43]

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

[commit-bot: I haz the power, 2016-09-19 22:00:44]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dougt, 2016-09-20 00:07:06]

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

[commit-bot: I haz the power, 2016-09-20 00:07:39]

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-09-20 00:38:31]

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

[commit-bot: I haz the power, 2016-09-20 00:38:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dougt, 2016-09-21 02:15:50]

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

[commit-bot: I haz the power, 2016-09-21 02:16:27]

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-09-21 03:36:38]

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

[commit-bot: I haz the power, 2016-09-21 03:36:41]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[raymes, 2016-09-21 07:23:31]

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

[raymes, 2016-09-21 07:23:32]

Sorry for being slow. Thanks for doing this, overall seems good.

https://codereview.chromium.org/2351803002/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/2351803002/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:150: new
DurableStoragePermissionContext(profile));
We should go through the PermissionManager, something like:

PermissionManager::Get(profile)->GetPermissionStatus(origin, origin)

[dougt, 2016-09-21 14:18:18]

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

[commit-bot: I haz the power, 2016-09-21 14:18:37]

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-09-21 15:43:31]

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

[commit-bot: I haz the power, 2016-09-21 15:43:32]

Dry run: This issue passed the CQ dry run.

[dougt, 2016-09-21 18:09:17]

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

[commit-bot: I haz the power, 2016-09-21 18:09:47]

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-09-21 18:47:30]

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

[commit-bot: I haz the power, 2016-09-21 18:47:31]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-09-21 23:50:18]

lgtm

[dougt, 2016-09-23 16:38:57]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2016-09-23 16:39:22]

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-09-23 16:49:41]

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

[commit-bot: I haz the power, 2016-09-23 16:49:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pfeldman, 2016-09-23 17:12:19]

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

[pfeldman, 2016-09-23 17:12:21]

some friendly style nits for you.

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:119: if
(cookie_settings_ == NULL)
Since you touch those lines, you could migrate them to nullptr.

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:150: Profile*
profile = Profile::FromBrowserContext(browser_context_);
Just store a pointer to the profile, no need to convert all the time.

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.h (right):

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.h:34:
content::BrowserContext* browser_context);
As a part of the chrome internal API, you should prefer using Profile over the
context.

https://codereview.chromium.org/2351803002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.h:99:
content::BrowserContext* browser_context_; // weak reference.
It is either WeakPtr or a raw pointer. In this case it is raw, no need to say
weak - it is clear from the code that you use raw.

[dougt, 2016-09-23 18:23:22]

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

[commit-bot: I haz the power, 2016-09-23 18:24:07]

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

[dougt, 2016-09-23 18:57:07]

Description was changed from

==========
Switching ExtensionSpecialStoragePolicy::IsStorageDurable to use
DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations. We found that we could remove the
cookies_settings::IsStorageDurable() and, instead, just have the
ExtensionSpecialStoragePolicy access the DurableStoragePermissionContext.

BUG=539538
==========

to

==========
Switching ExtensionSpecialStoragePolicy::IsStorageDurable to use
DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations. We found that we could remove the
cookies_settings::IsStorageDurable() and, instead, just have the
ExtensionSpecialStoragePolicy access the DurableStoragePermissionContext.

BUG=539538
==========

[dougt, 2016-09-23 18:57:07]

dougt@chromium.org changed reviewers:
+ thakis@chromium.org

[commit-bot: I haz the power, 2016-09-23 19:14:35]

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

[commit-bot: I haz the power, 2016-09-23 19:14:36]

Dry run: This issue passed the CQ dry run.

[dougt, 2016-10-07 16:12:03]

dougt@chromium.org changed reviewers:
+ jochen@chromium.org

[dougt, 2016-10-07 16:12:04]

jochen, please take a look.

[msramek, 2016-10-07 16:17:20]

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

[msramek, 2016-10-07 16:17:22]

Drive by comment! (otherwise browsing_data/ LGTM)

https://codereview.chromium.org/2351803002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/cookies_tree_model_unittest.cc (right):

https://codereview.chromium.org/2351803002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/cookies_tree_model_unittest.cc:85: const char
kExtensionScheme[] = "extensionscheme";
|kExtensionScheme| and |cookie_settings| are now unused

[dougt, 2016-10-07 21:15:37]

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

[commit-bot: I haz the power, 2016-10-07 21:16:21]

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

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

[commit-bot: I haz the power, 2016-10-07 23:16:06]

Dry run: This issue passed the CQ dry run.

[dougt, 2016-10-08 02:19:52]

https://codereview.chromium.org/2351803002/diff/160001/chrome/browser/browsin...
File chrome/browser/browsing_data/cookies_tree_model_unittest.cc (right):

https://codereview.chromium.org/2351803002/diff/160001/chrome/browser/browsin...
chrome/browser/browsing_data/cookies_tree_model_unittest.cc:85: const char
kExtensionScheme[] = "extensionscheme";
On 2016/10/07 16:17:21, msramek wrote:
> |kExtensionScheme| and |cookie_settings| are now unused

Acknowledged.

[jochen (gone - plz use gerrit), 2016-10-10 12:43:34]

lgtm

please reformat the CL description according to
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...

[dougt, 2016-10-10 15:36:43]

The CQ bit was checked by dougt@chromium.org

[dougt, 2016-10-10 15:36:44]

The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org,
msramek@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2351803002/#ps200001
(title: "Switching IsStorageDurable() to use DurableStoragePermissionContext.")

[commit-bot: I haz the power, 2016-10-10 15:36:58]

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

[dougt, 2016-10-10 15:38:45]

Description was changed from

==========
Switching ExtensionSpecialStoragePolicy::IsStorageDurable to use
DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations. We found that we could remove the
cookies_settings::IsStorageDurable() and, instead, just have the
ExtensionSpecialStoragePolicy access the DurableStoragePermissionContext.

BUG=539538
==========

to

==========
Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman
==========

[dougt, 2016-10-10 15:38:45]

dougt@chromium.org changed reviewers:
- thakis@chromium.org

[commit-bot: I haz the power, 2016-10-10 16:24:26]

Description was changed from

==========
Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman
==========

to

==========
Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman
==========

[commit-bot: I haz the power, 2016-10-10 16:24:28]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-10-10 16:26:45]

Description was changed from

==========
Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman
==========

to

==========
Switching IsStorageDurable() to use DurableStoragePermissionContext.

ExtensionSpecialStoragePolicy was using cookies_settings to figure out if the
given origin was allowed to use durable storage. In order to make this work, the
cookies_settings had to use a lower level api (HostContentSettingsMap) to avoid
layer violations.

We found that we could remove the cookies_settings::IsStorageDurable() and,
instead, just have the ExtensionSpecialStoragePolicy access the
PermissionManager.

BUG=539538
R=raymes, jochen, pfeldman

Committed: https://crrev.com/4a19a66d254415f7afb67d6f3c6e6354f280109d
Cr-Commit-Position: refs/heads/master@{#424165}
==========

[commit-bot: I haz the power, 2016-10-10 16:26:47]

Patchset 11 (id:??) landed as
https://crrev.com/4a19a66d254415f7afb67d6f3c6e6354f280109d
Cr-Commit-Position: refs/heads/master@{#424165}

[dougt, 2016-10-18 20:45:25]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/2426133002/ by dougt@chromium.org.

The reason for reverting is: Caused issue 656495..