Add SpecialStoragePolicy::HasIsolatedStorage

Add SpecialStoragePolicy::HasIsolatedStorage

So that we can tell if the origin has isolated storage partition or not.

BUG=242272
TEST=ExtensionSpecialStoragePolicyTest.HasIsolatedStorage
R=asargent@chromium.org, michaeln@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203129

[kinuko, 2013-05-28 11:41:33]

Can we add a method like this to SpecialStoragePolicy? PTL thanks,

[michaeln, 2013-05-28 19:23:19]

lgtm

The code change looks fine and good.

But, what this actually means is kind of elusive. We will have <httporigin>
storage data inside of an apps nested "isolated" StoragePartition, the sort of
partition used by its webviews... and this method will return 'false' for that
origin. This method will only return true for <chrome-extension> origins that
correspond to isolated packaged apps.

Is that right?

[kinuko, 2013-05-29 02:09:31]

On 2013/05/28 19:23:19, michaeln wrote:
> lgtm
> 
> The code change looks fine and good.
> 
> But, what this actually means is kind of elusive. We will have <httporigin>
> storage data inside of an apps nested "isolated" StoragePartition, the sort of
> partition used by its webviews... and this method will return 'false' for that
> origin. This method will only return true for <chrome-extension> origins that
> correspond to isolated packaged apps.
> 
> Is that right?

Right. The name 'IsStorageIsolated' is a bit misnomer maybe.
This method takes 'origin', and I want it to indicate 'the origin has its own
isolated storage'.

How about if we rename this to like...

HasIsolatedStorage(origin) or
HasSingleIsolatedStorage(origin)

The former name may make it easier to relate this to
AppIsolationInfo::HasIsolatedStorage for readers.

[michaeln, 2013-05-29 02:21:28]

> How about if we rename this to like...
> 
> HasIsolatedStorage(origin) or
> HasSingleIsolatedStorage(origin)
> 
> The former name may make it easier to relate this to
> AppIsolationInfo::HasIsolatedStorage for readers.

I think 'HasIsolatedStorage' is helpful, suggests that its somehow different
than those other IsXYZ tests and suggests a contains relationship. Hmmm...
another option... ConstainsIsolatedStorage(ori)

[kinuko, 2013-05-29 02:42:20]

On 2013/05/29 02:21:28, michaeln wrote:
> > How about if we rename this to like...
> > 
> > HasIsolatedStorage(origin) or
> > HasSingleIsolatedStorage(origin)
> > 
> > The former name may make it easier to relate this to
> > AppIsolationInfo::HasIsolatedStorage for readers.
> 
> I think 'HasIsolatedStorage' is helpful, suggests that its somehow different
> than those other IsXYZ tests and suggests a contains relationship. Hmmm...
> another option... ConstainsIsolatedStorage(ori)

I slightly prefer HasIsolatedStorage, mainly for the sake of consistency.
I'll rename the method and add an explicit comment that it means a contains
relationship. (If you feel ok with this rename)

[michaeln, 2013-05-29 02:47:46]

On 2013/05/29 02:42:20, kinuko wrote:
> On 2013/05/29 02:21:28, michaeln wrote:
> > > How about if we rename this to like...
> > > 
> > > HasIsolatedStorage(origin) or
> > > HasSingleIsolatedStorage(origin)
> > > 
> > > The former name may make it easier to relate this to
> > > AppIsolationInfo::HasIsolatedStorage for readers.
> > 
> > I think 'HasIsolatedStorage' is helpful, suggests that its somehow different
> > than those other IsXYZ tests and suggests a contains relationship. Hmmm...
> > another option... ConstainsIsolatedStorage(ori)
> 
> I slightly prefer HasIsolatedStorage, mainly for the sake of consistency.
> I'll rename the method and add an explicit comment that it means a contains
> relationship. (If you feel ok with this rename)

sgtm!

[kinuko, 2013-05-29 03:50:33]

Revised the method name and method-level comment per discussion.

[kinuko, 2013-05-29 03:52:25]

benwells: mind doing an owner's review for c/b/extensions changes? Thanks!

[kinuko, 2013-05-29 14:15:22]

On 2013/05/29 03:52:25, kinuko wrote:
> benwells: mind doing an owner's review for c/b/extensions changes? Thanks!

benwells@ or asargent@: can you do owner review for the
/chrome/browser/extensions changes?  Thanks,

[asargent_no_longer_on_chrome, 2013-05-29 17:11:58]

LGTM

Apologies for review latency

[kinuko, 2013-05-30 02:23:05]

On 2013/05/29 17:11:58, Antony Sargent wrote:
> LGTM
> 
> Apologies for review latency

Thanks!

[commit-bot: I haz the power, 2013-05-30 02:35:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/15862008/17001

[commit-bot: I haz the power, 2013-05-30 06:02:48]

Failed to apply patch for webkit/quota/mock_special_storage_policy.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: webkit/quota/mock_special_storage_policy.cc
  |diff --git a/webkit/quota/mock_special_storage_policy.cc
b/webkit/quota/mock_special_storage_policy.cc
  |index
5aaf21cbd6015f5f47afa5109072fd0e0d599659..6b9db29d7f3242e797d9d56b9a68fbdd918ebba8
100644
  |--- a/webkit/quota/mock_special_storage_policy.cc
  |+++ b/webkit/quota/mock_special_storage_policy.cc
  --------------------------
  No file to patch.  Skipping patch.
  2 out of 2 hunks ignored

Patch:       webkit/quota/mock_special_storage_policy.cc
Index: webkit/quota/mock_special_storage_policy.cc
diff --git a/webkit/quota/mock_special_storage_policy.cc
b/webkit/quota/mock_special_storage_policy.cc
index
5aaf21cbd6015f5f47afa5109072fd0e0d599659..6b9db29d7f3242e797d9d56b9a68fbdd918ebba8
100644
--- a/webkit/quota/mock_special_storage_policy.cc
+++ b/webkit/quota/mock_special_storage_policy.cc
@@ -4,6 +4,8 @@
 
 #include "webkit/quota/mock_special_storage_policy.h"
 
+#include "base/stl_util.h"
+
 namespace quota {
 
 MockSpecialStoragePolicy::MockSpecialStoragePolicy()
@@ -11,25 +13,29 @@ MockSpecialStoragePolicy::MockSpecialStoragePolicy()
 }
 
 bool MockSpecialStoragePolicy::IsStorageProtected(const GURL& origin) {
-  return protected_.find(origin) != protected_.end();
+  return ContainsKey(protected_, origin);
 }
 
 bool MockSpecialStoragePolicy::IsStorageUnlimited(const GURL& origin) {
   if (all_unlimited_)
     return true;
-  return unlimited_.find(origin) != unlimited_.end();
+  return ContainsKey(unlimited_, origin);
 }
 
 bool MockSpecialStoragePolicy::IsStorageSessionOnly(const GURL& origin) {
-  return session_only_.find(origin) != session_only_.end();
+  return ContainsKey(session_only_, origin);
 }
 
 bool MockSpecialStoragePolicy::CanQueryDiskSize(const GURL& origin) {
-  return can_query_disk_size_.find(origin) != can_query_disk_size_.end();
+  return ContainsKey(can_query_disk_size_, origin);
 }
 
 bool MockSpecialStoragePolicy::IsFileHandler(const std::string& extension_id) {
-  return file_handlers_.find(extension_id) != file_handlers_.end();
+  return ContainsKey(file_handlers_, extension_id);
+}
+
+bool MockSpecialStoragePolicy::HasIsolatedStorage(const GURL& origin) {
+  return ContainsKey(isolated_, origin);
 }
 
 bool MockSpecialStoragePolicy::HasSessionOnlyOrigins() {

[commit-bot: I haz the power, 2013-05-30 07:00:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/15862008/40002

[kinuko, 2013-05-30 12:33:56]

Committed patchset #3 manually as r203129 (presubmit successful).