Fix ExtensionSpecialStoragePolicy.IsInstalledApp

Fix ExtensionSpecialStoragePolicy.IsInstalledApp

We weren't adding apps to the installed_app set when it doesn't have
any storage permissions.

BUG=231167
TEST=ExtensionSpecialStoragePolicyTest.IsInstalled

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

[kinuko, 2013-04-15 02:40:09]

This logic seemed to have been wrong for non-unlimited apps etc.

michaeln: Can you take a look at the SpecialStoragePolicy change?
asargent: Can you take a look at the change in general as extension OWNER

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

lgtm

https://codereview.chromium.org/13994010/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://codereview.chromium.org/13994010/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_special_storage_policy.cc:95:
!extension->is_app()) {
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)

?

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

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:51: bool
ExtensionSpecialStoragePolicy::IsInstalledApp(const GURL& origin) {
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);

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:121:
APIPermission::kFileBrowserHandler)) {
does the is_app() test need to go here too so the grant/revoke logic is
balanced?

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

Thanks, addressed comments.

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:51: bool
ExtensionSpecialStoragePolicy::IsInstalledApp(const GURL& origin) {
On 2013/04/15 20:59:30, michaeln wrote:
> 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);

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?

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:95:
!extension->is_app()) {
On 2013/04/15 18:43:11, Antony Sargent wrote:
> 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)
> 
> ?

Done.

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:121:
APIPermission::kFileBrowserHandler)) {
On 2013/04/15 20:59:30, michaeln wrote:
> does the is_app() test need to go here too so the grant/revoke logic is
> balanced?

Good catch, done.

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

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/13994010/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_special_storage_policy.cc:51: bool
ExtensionSpecialStoragePolicy::IsInstalledApp(const GURL& origin) {
On 2013/04/16 07:20:13, kinuko wrote:
> On 2013/04/15 20:59:30, michaeln wrote:
> > 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);
> 
> 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?

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

[michaeln, 2013-04-16 21:14:09]

lgtm!

https://chromiumcodereview.appspot.com/13994010/diff/17001/chrome/browser/ext...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/13994010/diff/17001/chrome/browser/ext...
chrome/browser/extensions/extension_special_storage_policy.cc:93:
extension->is_app())) {
much more readable that way :)

[kinuko, 2013-04-17 01:54:15]

Committed patchset #2 manually as r194511 (presubmit successful).