QuotaManager: Return the remaining free disk space as quota.

QuotaManager: Return the remaining free disk space as quota.

Previously we returned kNoLimit, which prevents apps from managing disk usage.

BUG=141583
TEST=
R=kinuko


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=153248

[kinuko, 2012-08-13 14:07:52]

(Some preliminary comments)

http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc#...
webkit/quota/quota_manager.cc:86: int64 quota = unlimited ?
quota_and_usage.available_disk_space :
(You might be changing this but to make sure) I think we should return the
available disk space if and ONLY if the quota is requested by installed apps
(i.e. for the apps ExtensionService::IsInstalledApp() returns true)

http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc#...
webkit/quota/quota_manager.cc:1352: return
base::SysInfo::AmountOfFreeDiskSpace(profile_path_);
Hmm.  How about using a static function pointer (which is safely guarded for
thread safety) instead of having this virtual method?  We can override only in
the tests and if it's not set we can use the base::SysInfo one.

[James Hawkins, 2012-08-17 21:33:22]

http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc#...
webkit/quota/quota_manager.cc:86: int64 quota = unlimited ?
quota_and_usage.available_disk_space :
On 2012/08/13 14:07:52, kinuko wrote:
> (You might be changing this but to make sure) I think we should return the
> available disk space if and ONLY if the quota is requested by installed apps
> (i.e. for the apps ExtensionService::IsInstalledApp() returns true)

I'm attempting to add support for this check now, but I haven't yet figured out
how to query the ExtensionService from content/.  Any clues?

[James Hawkins, 2012-08-17 21:42:14]

On 2012/08/17 21:33:22, James Hawkins wrote:
> http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc
> File webkit/quota/quota_manager.cc (right):
> 
>
http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc#...
> webkit/quota/quota_manager.cc:86: int64 quota = unlimited ?
> quota_and_usage.available_disk_space :
> On 2012/08/13 14:07:52, kinuko wrote:
> > (You might be changing this but to make sure) I think we should return the
> > available disk space if and ONLY if the quota is requested by installed apps
> > (i.e. for the apps ExtensionService::IsInstalledApp() returns true)
> 
> I'm attempting to add support for this check now, but I haven't yet figured
out
> how to query the ExtensionService from content/.  Any clues?

From webkit/ I mean.

[James Hawkins, 2012-08-17 22:19:06]

On 2012/08/17 21:42:14, James Hawkins wrote:
> On 2012/08/17 21:33:22, James Hawkins wrote:
> > http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc
> > File webkit/quota/quota_manager.cc (right):
> > 
> >
>
http://codereview.chromium.org/10826270/diff/1/webkit/quota/quota_manager.cc#...
> > webkit/quota/quota_manager.cc:86: int64 quota = unlimited ?
> > quota_and_usage.available_disk_space :
> > On 2012/08/13 14:07:52, kinuko wrote:
> > > (You might be changing this but to make sure) I think we should return the
> > > available disk space if and ONLY if the quota is requested by installed
apps
> > > (i.e. for the apps ExtensionService::IsInstalledApp() returns true)
> > 
> > I'm attempting to add support for this check now, but I haven't yet figured
> out
> > how to query the ExtensionService from content/.  Any clues?
> 
> From webkit/ I mean.

Nevermind I got it.

[James Hawkins, 2012-08-17 23:23:40]

Ready for another look.  New version uses a function pointer which can be set by
the test to use a consistent value for the available disk space.  Installed app
status is now plumbed through SpecialStoragePolicy.

There is a question (marked 'FIXME') in extension_special_storage_policy.cc
which you may be able to answer.

[kinuko, 2012-08-20 04:04:44]

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:89: kint64max;
nit: kint64max -> kNoLimit since that's what we check against in the unittest?

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:972: callback_(callback) {
nit: maybe DCHECK(get_disk_space_fn) here?

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:401: // value.
nit: can you also add a comment about what is called if nothing is set.

[michaeln, 2012-08-20 23:04:06]

https://chromiumcodereview.appspot.com/10826270/diff/2003/chrome/browser/exte...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

https://chromiumcodereview.appspot.com/10826270/diff/2003/chrome/browser/exte...
chrome/browser/extensions/extension_special_storage_policy.cc:134:
protected_apps_.Clear();
drive by: also call .Clear() on the new collection?

[Aaron Boodman, 2012-08-21 23:09:46]

I don't understand why we're tracking the installed apps in this class when we
already track them elsewhere in the system.

[kinuko, 2012-08-22 01:22:47]

What would be the recommended way to see if a given origin is for installed
app or not on IO thread?

All Quota code needs is to know the installed status via
SpecialStoragePolicy interface.
 On Aug 22, 2012 8:09 AM, <aa@chromium.org> wrote:

> I don't understand why we're tracking the installed apps in this class
> when we
> already track them elsewhere in the system.
>
>
http://codereview.chromium.**org/10826270/<http://codereview.chromium.org/108...
>

[James Hawkins, 2012-08-22 18:51:35]

http://codereview.chromium.org/10826270/diff/2003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_special_storage_policy.cc (right):

http://codereview.chromium.org/10826270/diff/2003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_special_storage_policy.cc:134:
protected_apps_.Clear();
On 2012/08/20 23:04:06, michaeln wrote:
> drive by: also call .Clear() on the new collection?

Done.

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager.cc
File webkit/quota/quota_manager.cc (right):

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:89: kint64max;
On 2012/08/20 04:04:44, kinuko wrote:
> nit: kint64max -> kNoLimit since that's what we check against in the unittest?

Done.

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.cc:972: callback_(callback) {
On 2012/08/20 04:04:44, kinuko wrote:
> nit: maybe DCHECK(get_disk_space_fn) here?

Done.

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager.h
File webkit/quota/quota_manager.h (right):

http://codereview.chromium.org/10826270/diff/2003/webkit/quota/quota_manager....
webkit/quota/quota_manager.h:401: // value.
On 2012/08/20 04:04:44, kinuko wrote:
> nit: can you also add a comment about what is called if nothing is set.

Done.

[Aaron Boodman, 2012-08-22 21:28:21]

You can get installed extensions and apps on the IO thread via the
ExtensionInfoMap class which is owned by ProfileIOData. Would that work for you?

[kinuko, 2012-08-23 11:00:44]

@aa: thanks, to be more specific, Quota code (which lives in webkit/) used to
access chrome's per-origin/extension information via SpecialStoragePolicy
interface, which is designed as thread-safe class and all of its methods can be
called on any threads.  In my understanding that's why we keep/copy a few maps
inside the class.  Does this make sense?

(Technically yes we'd probably be able to call the ProfileIOData's extension map
but we'll need additional plumbing to work around the layering issue to access
the map from Quota code)

[James Hawkins, 2012-08-23 15:54:12]

On 2012/08/23 11:00:44, kinuko wrote:
> @aa: thanks, to be more specific, Quota code (which lives in webkit/) used to
> access chrome's per-origin/extension information via SpecialStoragePolicy
> interface, which is designed as thread-safe class and all of its methods can
be
> called on any threads.  In my understanding that's why we keep/copy a few maps
> inside the class.  Does this make sense?
> 
> (Technically yes we'd probably be able to call the ProfileIOData's extension
map
> but we'll need additional plumbing to work around the layering issue to access
> the map from Quota code)

Can I get a CL LGTM while we work out the issue aa@ presented?  Just in case
this solution is good to go as is.  I really need to get this in ASAP.

[Aaron Boodman, 2012-08-23 21:42:20]

I see. OK, that seems fine.

[Aaron Boodman, 2012-08-23 21:42:33]

(lgtm)

[James Hawkins, 2012-08-23 22:08:39]

On 2012/08/23 21:42:33, Aaron Boodman wrote:
> (lgtm)

Thank you, Aaron.  Kinuko, waiting for your LG.

[kinuko, 2012-08-24 03:25:14]

lgtm!

[commit-bot: I haz the power, 2012-08-24 03:33:50]

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

[commit-bot: I haz the power, 2012-08-24 07:29:19]

Try job failure for 10826270-17001 (retry) on linux_chromeos for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-24 15:26:48]

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

[commit-bot: I haz the power, 2012-08-24 18:54:54]

Change committed as 153248