Extension Storage API: expose storage quota information to extensions, via:

Extension Storage API: expose storage quota information to extensions, via:
 - exposing the constants as properties on chrome.storage.{sync,local}, and
 - adding a new API call "getBytesInUse".


BUG=110980, 110583, 110663
TEST=unit_tests --gtest_filter=ExtensionSettingsFrontendTest*, browser_tests --gtest_filter=ExtensionSetting*


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

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

[not at google - send to devlin, 2012-01-24 05:44:50]

Hi guys (in particular mpcomplete/aa)

Check out the bugs to see why I'm doing this (apart from the
s/StorageNamespace/StorageArea/ one, that's incidental).

I seek a couple of opinions before I write any further code: though this is
pretty self-contained as is:

1. Good names? I'm pretty happy with them...

2. Should getBytesInUse mirror get/remove in the arguments they can take, namely
also accept a key or list of keys to compute bytes in use for? Pros: consistent.
Cons: slightly more complicated code, need to pass "null" into all calls to that
method to get total usage, and might not be necessary. Overall I think we
_should_ support the same convention, but before I write the code... just
checking.

3. Should I update the documentation builder to generate info about those
properties-on-properties (i.e. QUOTA_BYTES on sync/local)? At the moment it
isn't. I had to fix that problem in schema_generated_bindings.js. I really like
the idea of having it generated, that was one of the points of defining it in
the JSON (the other so that it's accessible), but... the documentation generator
is pretty intimidating.

[Matt Perry, 2012-01-24 21:54:59]

mostly LG, just not sure about the constants.

I think it makes sense to be able to query bytesInUse by key/keylist.

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
chrome/common/extensions/api/storage.json:174: "QUOTA_BYTES": {
What's our style for API constants? I don't think I've seen the ALL_CAPS style
before.

[not at google - send to devlin, 2012-01-24 21:56:48]

Cool, I'll do that today then.

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
chrome/common/extensions/api/storage.json:174: "QUOTA_BYTES": {
On 2012/01/24 21:54:59, Matt Perry wrote:
> What's our style for API constants? I don't think I've seen the ALL_CAPS style
> before.

Ah, I was basing it off that WINDOW_ID_NONE variable in windows.json (because
I'd seen it before for a different reason). Maverick constant?

[Matt Perry, 2012-01-24 22:02:19]

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/9284013/diff/5001/chrome/common/extens...
chrome/common/extensions/api/storage.json:174: "QUOTA_BYTES": {
On 2012/01/24 21:56:48, kalman wrote:
> On 2012/01/24 21:54:59, Matt Perry wrote:
> > What's our style for API constants? I don't think I've seen the ALL_CAPS
style
> > before.
> 
> Ah, I was basing it off that WINDOW_ID_NONE variable in windows.json (because
> I'd seen it before for a different reason). Maverick constant?

Oh OK, sounds like there's precedent. Carry on, then.

[Aaron Boodman, 2012-01-24 23:40:53]

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/settings/settings_api.cc (right):

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_api.cc:187: Value *input;
*twitch* - initialize primitives.

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
chrome/common/extensions/api/storage.json:184: "type": "integer",
Bummer to duplicate these values with the C++ impl. Can we share them? Either by
fishing out of ExtensionAPI at runtime, or by making these functions that are
backed by the C++ constants?

[not at google - send to devlin, 2012-01-25 00:00:13]

@mpcomplete I added those new methods, and tests, so you might want to check out
that diff again.

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/settings/settings_api.cc (right):

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_api.cc:187: Value *input;
On 2012/01/24 23:40:53, Aaron Boodman wrote:
> *twitch* - initialize primitives.

Done.

oops, fixed up the other cases of this (this class dates back to me as a c++
newbie).

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
File chrome/common/extensions/api/storage.json (right):

https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
chrome/common/extensions/api/storage.json:184: "type": "integer",
On 2012/01/24 23:40:53, Aaron Boodman wrote:
> Bummer to duplicate these values with the C++ impl. Can we share them? Either
by
> fishing out of ExtensionAPI at runtime, or by making these functions that are
> backed by the C++ constants?

Rather not make them functions, they're semantically constants.

As for fishing them out of Extension API at runtime, definitely possible. Though
kind of a pain because
 - you'd be doing unnecessary work every time you're constructing a
SettingsBackend
 - c++ code a bit harder to follow from a self-documentation perspective
(actually... not sure about this, you could make it look ok I think)

Anyway, yeah, that's why I didn't do it initially.

[Aaron Boodman, 2012-01-25 00:11:52]

On 2012/01/25 00:00:13, kalman wrote:
> @mpcomplete I added those new methods, and tests, so you might want to check
out
> that diff again.
> 
>
https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
> File chrome/browser/extensions/settings/settings_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/browser/exten...
> chrome/browser/extensions/settings/settings_api.cc:187: Value *input;
> On 2012/01/24 23:40:53, Aaron Boodman wrote:
> > *twitch* - initialize primitives.
> 
> Done.
> 
> oops, fixed up the other cases of this (this class dates back to me as a c++
> newbie).
> 
>
https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
> File chrome/common/extensions/api/storage.json (right):
> 
>
https://chromiumcodereview.appspot.com/9284013/diff/7001/chrome/common/extens...
> chrome/common/extensions/api/storage.json:184: "type": "integer",
> On 2012/01/24 23:40:53, Aaron Boodman wrote:
> > Bummer to duplicate these values with the C++ impl. Can we share them?
Either
> by
> > fishing out of ExtensionAPI at runtime, or by making these functions that
are
> > backed by the C++ constants?
> 
> Rather not make them functions, they're semantically constants.
> 
> As for fishing them out of Extension API at runtime, definitely possible.
Though
> kind of a pain because
>  - you'd be doing unnecessary work every time you're constructing a
> SettingsBackend
>  - c++ code a bit harder to follow from a self-documentation perspective
> (actually... not sure about this, you could make it look ok I think)
> 
> Anyway, yeah, that's why I didn't do it initially.

Another option would be leave them as properties in the api json, but add a
mechanism to populate those properties dynamically from C++.

[Matt Perry, 2012-01-25 00:14:17]

lgtm

https://chromiumcodereview.appspot.com/9284013/diff/11006/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_storage_quota_enforcer.cc
(right):

https://chromiumcodereview.appspot.com/9284013/diff/11006/chrome/browser/exte...
chrome/browser/extensions/settings/settings_storage_quota_enforcer.cc:107: used
+= it->second;
? Don't you need to look this up in used_per_setting_ ?

[not at google - send to devlin, 2012-01-25 00:31:49]

https://chromiumcodereview.appspot.com/9284013/diff/11006/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_storage_quota_enforcer.cc
(right):

https://chromiumcodereview.appspot.com/9284013/diff/11006/chrome/browser/exte...
chrome/browser/extensions/settings/settings_storage_quota_enforcer.cc:107: used
+= it->second;
On 2012/01/25 00:14:17, Matt Perry wrote:
> ? Don't you need to look this up in used_per_setting_ ?

Oh crap.

Drive by "oh why did I write it that way?" fail...

[not at google - send to devlin, 2012-01-25 00:41:34]

> Another option would be leave them as properties in the api json, but add a
> mechanism to populate those properties dynamically from C++.

Yeah, I figure this is how I'd do it:

Add a method like "LookupCachedValue" or something to ExtensionAPI (just a more
generic way of caching those constants); since ExtensionAPI is actually a
Singleton, it's a nice (probably only) place to put that. Then add a static
"GetLimitFromExtensionAPI(std::string)" or something method to QuotaEnforcer.

So SettingsFrontend it would look like

QuotaEnforcer::Limits sync_limits =
QuotaEnforcer::Limits::GetFromExtensionAPI("sync");
QuotaEnforcer::Limits local_limits =
QuotaEnforcer::Limits::GetFromExtensionAPI("local");

// construct sync backend with sync_limits
// construct local backend with local_limits

[Aaron Boodman, 2012-01-25 00:48:44]

On Tue, Jan 24, 2012 at 4:41 PM,  <kalman@chromium.org> wrote:
>> Another option would be leave them as properties in the api json, but add
>> a
>> mechanism to populate those properties dynamically from C++.
>
>
> Yeah, I figure this is how I'd do it:
>
> Add a method like "LookupCachedValue" or something to ExtensionAPI (just a
> more
> generic way of caching those constants); since ExtensionAPI is actually a
> Singleton, it's a nice (probably only) place to put that. Then add a static
> "GetLimitFromExtensionAPI(std::string)" or something method to
> QuotaEnforcer.
>
> So SettingsFrontend it would look like
>
> QuotaEnforcer::Limits sync_limits =
> QuotaEnforcer::Limits::GetFromExtensionAPI("sync");
> QuotaEnforcer::Limits local_limits =
> QuotaEnforcer::Limits::GetFromExtensionAPI("local");
>
> // construct sync backend with sync_limits
> // construct local backend with local_limits

OK.

[not at google - send to devlin, 2012-01-25 00:48:59]

On 2012/01/25 00:41:34, kalman wrote:
> > Another option would be leave them as properties in the api json, but add a
> > mechanism to populate those properties dynamically from C++.
> 
> Yeah, I figure this is how I'd do it:
> 
> Add a method like "LookupCachedValue" or something to ExtensionAPI (just a
more
> generic way of caching those constants); since ExtensionAPI is actually a
> Singleton, it's a nice (probably only) place to put that. Then add a static
> "GetLimitFromExtensionAPI(std::string)" or something method to QuotaEnforcer.
> 
> So SettingsFrontend it would look like
> 
> QuotaEnforcer::Limits sync_limits =
> QuotaEnforcer::Limits::GetFromExtensionAPI("sync");
> QuotaEnforcer::Limits local_limits =
> QuotaEnforcer::Limits::GetFromExtensionAPI("local");
> 
> // construct sync backend with sync_limits
> // construct local backend with local_limits

Or just not cache it... just realised that API super-cludginess != API
super-slowness (necessarily).

[not at google - send to devlin, 2012-01-25 03:17:09]

done stuff

[not at google - send to devlin, 2012-01-25 03:57:31]

By the way, the base/ changes I've put in a separate patch to submit separately,
too.

[Aaron Boodman, 2012-01-26 23:48:54]

lgtm

[commit-bot: I haz the power, 2012-01-27 00:16:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9284013/11010

[commit-bot: I haz the power, 2012-01-27 01:08:50]

Try job failure for 9284013-11010 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-01-27 03:45:29]

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

[commit-bot: I haz the power, 2012-01-27 04:28:56]

Try job failure for 9284013-17001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[not at google - send to devlin, 2012-01-27 04:34:02]

Oh, cos size_t is different on different platforms. Duh.

Stupid language.

[commit-bot: I haz the power, 2012-01-28 04:34:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9284013/22002

[commit-bot: I haz the power, 2012-01-28 08:46:37]

Change committed as 119598

[commit-bot: I haz the power, 2012-01-29 09:24:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kalman@chromium.org/9284013/18058

[commit-bot: I haz the power, 2012-01-29 11:19:13]

Change committed as 119643