Fix ExtensionSpecialStoragePolicy.IsInstalledApp

chrome/browser/extensions/extension_special_storage_policy.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_special_storage_policy.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 30 matching lines @@
   base::AutoLock locker(lock_);
   return unlimited_extensions_.Contains(origin);
 }
 
 bool ExtensionSpecialStoragePolicy::IsStorageSessionOnly(const GURL& origin) {
   if (cookie_settings_ == NULL)
     return false;
   return cookie_settings_->IsCookieSessionOnly(origin);
 }
 
 bool ExtensionSpecialStoragePolicy::IsInstalledApp(const GURL& origin) {

[michaeln, 2013/04/15 20:59:30]

not for this CL but...

This seems like an unfortunate chrome->content abstraction leakage? InstalledApp
doesn't mean anything in the content lib. Based on how this is used (based on
the comment in the SpecialStoragePolicy base class), maybe this method could
have a different name...

bool HasActualDiskSizeRights(origin);

[kinuko, 2013/04/16 07:20:13]

I had the same feeling about layering. Could I create another patch for fixing
this naming while landing this one just to fix the logic?

[kinuko, 2013/04/16 07:50:35]

Created a new patch for this: https://codereview.chromium.org/14237003/

   return installed_apps_.Contains(origin);
 }
 
 bool ExtensionSpecialStoragePolicy::HasSessionOnlyOrigins() {
   if (cookie_settings_ == NULL)
     return false;
   if (cookie_settings_->GetDefaultCookieSetting(NULL) ==
       CONTENT_SETTING_SESSION_ONLY)
     return true;
   ContentSettingsForOneType entries;
@@ skipping 22 matching lines @@
   return protected_apps_.ExtensionsContaining(origin);
 }
 
 void ExtensionSpecialStoragePolicy::GrantRightsForExtension(
     const extensions::Extension* extension) {
   DCHECK(extension);
   if (!NeedsProtection(extension) &&
       !extension->HasAPIPermission(
           APIPermission::kUnlimitedStorage) &&
       !extension->HasAPIPermission(
-          APIPermission::kFileBrowserHandler)) {
+          APIPermission::kFileBrowserHandler) &&
+      !extension->is_app()) {

[asargent_no_longer_on_chrome, 2013/04/15 18:43:11]

nit: it would be good to keep the order of tests above here the same as the
order of tests inside the autolock'ed code below. E.g. protection, app,
unlimited permission, file browser handler. 

also optional suggestion: would this block be easier to read if it  was written
like this:

  if (!(A || B || C))

instead of like this:

  if (!A && !B && !C)

?

[kinuko, 2013/04/16 07:20:13]

Done.

     return;
   }
   {
     base::AutoLock locker(lock_);
     if (NeedsProtection(extension))
       protected_apps_.Add(extension);
     // FIXME: Does GrantRightsForExtension imply |extension| is installed?
     if (extension->is_app())
       installed_apps_.Add(extension);
     if (extension->HasAPIPermission(APIPermission::kUnlimitedStorage))
       unlimited_extensions_.Add(extension);
     if (extension->HasAPIPermission(
             APIPermission::kFileBrowserHandler))
       file_handler_extensions_.Add(extension);
   }
   NotifyChanged();
 }
 
 void ExtensionSpecialStoragePolicy::RevokeRightsForExtension(
     const extensions::Extension* extension) {
   DCHECK(extension);
   if (!NeedsProtection(extension) &&
       !extension->HasAPIPermission(
           APIPermission::kUnlimitedStorage) &&
       !extension->HasAPIPermission(
           APIPermission::kFileBrowserHandler)) {

[michaeln, 2013/04/15 20:59:30]

does the is_app() test need to go here too so the grant/revoke logic is
balanced?

[kinuko, 2013/04/16 07:20:13]

Good catch, done.

     return;
   }
   {
     base::AutoLock locker(lock_);
     if (NeedsProtection(extension))
       protected_apps_.Remove(extension);
     if (extension->is_app())
       installed_apps_.Remove(extension);
     if (extension->HasAPIPermission(APIPermission::kUnlimitedStorage))
       unlimited_extensions_.Remove(extension);
@@ skipping 74 matching lines @@
 
 void ExtensionSpecialStoragePolicy::SpecialCollection::Clear() {
   ClearCache();
   extensions_.Clear();
 }
 
 void ExtensionSpecialStoragePolicy::SpecialCollection::ClearCache() {
   STLDeleteValues(&cached_results_);
   cached_results_.clear();
 }