Extension Settings API: move the API functions into an object SettingsNamepace,

Extension Settings API: move the API functions into an object SettingsNamepace,
and separate the API into a "sync" and a "local" namespace.  Make calls into
the "local" namespace not synced.


BUG=103863
TEST=*ExtensionSetting* (browser_tests, unit_tests)


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

[not at google - send to devlin, 2011-11-23 02:24:43]

Ok, last big change*, I promise.

Except for the one which renames settings to whatever it will be renamed to.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:81: class
SettingsFrontend::BackendWrapper
This is basically what used to be Core, and replacing the functionality of
Backends.  It's a much nicer abstraction that actually scales to more namespaces
(particularly if we add an "inmemory" namespace or something).

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:189: factory,
This (and the other 3 occurrences) is basically why I needed to make Factory
ref-counted.  Other ideas?

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:210:
ExtensionService::kSyncExtensionSettingsDirectoryName));
except this is kinda yuck

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:256:
backends_[settings_namespace::SYNC][false]->RunWithBackend(callback);
as is this

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.h:89: class BackendWrapper;
I tried a bunch of things and this was just... the nicest I could think of. 
Other ideas welcome.

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:1038: "id": "StorageNamespace",
so apparently rietveld is bad at diffs

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
File chrome/common/extensions/docs/samples.json (left):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
chrome/common/extensions/docs/samples.json:75:
"chrome.experimental.settings.set": "experimental.settings.html#method-set",
umm, what is this page, is it bad that it no longer includes these samples?

[Matt Perry, 2011-11-23 03:35:04]

I mostly have stylistic suggestions. LGTM)

(I'll be OOO nov 23-25 so I won't be able to review any changes until next
Monday. I'll trust you to make the right changes, so you have my LG to commit
this when you want.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:157: SettingsBackend*
backend_;
add a comment to point out the ownership and thread safety

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:189: factory,
On 2011/11/23 02:24:43, kalman wrote:
> This (and the other 3 occurrences) is basically why I needed to make Factory
> ref-counted.  Other ideas?

This seems fine to me. If you really hate it, here's an idea:
Backend** backends[] = {&local_app, &local_extension, &sync_app, &sync_ext};
const char* dirs[] = {local_app_dir, etc};
for (int i = 0; i < 4; ++i)
  *backends[i] = Create(bla, bla, dirs[i]);

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:256:
backends_[settings_namespace::SYNC][false]->RunWithBackend(callback);
On 2011/11/23 02:24:43, kalman wrote:
> as is this

again, seems ok (and better than the alternatives)

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.h:89: class BackendWrapper;
On 2011/11/23 02:24:43, kalman wrote:
> I tried a bunch of things and this was just... the nicest I could think of. 
> Other ideas welcome.

How about just being explicit?

struct Backends {
  scoped_refptr<BackendWrapper> app_backend;
  scoped_refptr<BackendWrapper> extension_backend;
};

Backends local_backends;
Backends sync_backends;

and maybe a helper function GetBackend(namespace, is_app)

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_namespace.h (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_namespace.h:24: std::string
NamespaceToString(Namespace settings_namespace);
nit: I like ToString and FromString for these types of conversions

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:1169: "description": "Settings
under the \"local\" namespace are local to each browser.",
"local to each machine"?

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
File chrome/common/extensions/docs/samples.json (left):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
chrome/common/extensions/docs/samples.json:75:
"chrome.experimental.settings.set": "experimental.settings.html#method-set",
On 2011/11/23 02:24:43, kalman wrote:
> umm, what is this page, is it bad that it no longer includes these samples?

it's a map of API functions to definitions. you removed these, so it makes sense
that they're gone. it looks like we need to update the script to recognize
functions on types, though.

http://codereview.chromium.org/8670012/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/settings/simple_test/background.html
(right):

http://codereview.chromium.org/8670012/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/settings/simple_test/background.html:9: //
Make sure chrome.test.succeed() is only called once (see succeed()).
chrome.test.callbackPass might be better suited for this purpose. It creates and
returns a callback, and automatically calls succeed() when all callbacks have
been run.

[not at google - send to devlin, 2011-11-23 11:04:52]

Thanks Matt.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:157: SettingsBackend*
backend_;
On 2011/11/23 03:35:05, Matt Perry wrote:
> add a comment to point out the ownership and thread safety

Done.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:189: factory,
On 2011/11/23 03:35:05, Matt Perry wrote:
> On 2011/11/23 02:24:43, kalman wrote:
> > This (and the other 3 occurrences) is basically why I needed to make Factory
> > ref-counted.  Other ideas?
> 
> This seems fine to me. If you really hate it, here's an idea:
> Backend** backends[] = {&local_app, &local_extension, &sync_app, &sync_ext};
> const char* dirs[] = {local_app_dir, etc};
> for (int i = 0; i < 4; ++i)
>   *backends[i] = Create(bla, bla, dirs[i]);

Yeah, not so keen on that because of the 3 interdependencies (backends, dirs,
and 4).  This is fine I guess.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.cc:256:
backends_[settings_namespace::SYNC][false]->RunWithBackend(callback);
On 2011/11/23 03:35:05, Matt Perry wrote:
> On 2011/11/23 02:24:43, kalman wrote:
> > as is this
> 
> again, seems ok (and better than the alternatives)

iterator'd

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_frontend.h:89: class BackendWrapper;
On 2011/11/23 03:35:05, Matt Perry wrote:
> On 2011/11/23 02:24:43, kalman wrote:
> > I tried a bunch of things and this was just... the nicest I could think of. 
> > Other ideas welcome.
> 
> How about just being explicit?
> 
> struct Backends {
>   scoped_refptr<BackendWrapper> app_backend;
>   scoped_refptr<BackendWrapper> extension_backend;
> };
> 
> Backends local_backends;
> Backends sync_backends;
> 
> and maybe a helper function GetBackend(namespace, is_app)

Ok, I like the map<settings_namespace::Namespace> but mapping that to a Backends
is nicer than what I have.  Done.

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
File chrome/browser/extensions/settings/settings_namespace.h (right):

http://codereview.chromium.org/8670012/diff/1/chrome/browser/extensions/setti...
chrome/browser/extensions/settings/settings_namespace.h:24: std::string
NamespaceToString(Namespace settings_namespace);
On 2011/11/23 03:35:05, Matt Perry wrote:
> nit: I like ToString and FromString for these types of conversions

Done.

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/api/ex...
chrome/common/extensions/api/extension_api.json:1169: "description": "Settings
under the \"local\" namespace are local to each browser.",
On 2011/11/23 03:35:05, Matt Perry wrote:
> "local to each machine"?

Done.

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
File chrome/common/extensions/docs/samples.json (left):

http://codereview.chromium.org/8670012/diff/1/chrome/common/extensions/docs/s...
chrome/common/extensions/docs/samples.json:75:
"chrome.experimental.settings.set": "experimental.settings.html#method-set",
On 2011/11/23 03:35:05, Matt Perry wrote:
> On 2011/11/23 02:24:43, kalman wrote:
> > umm, what is this page, is it bad that it no longer includes these samples?
> 
> it's a map of API functions to definitions. you removed these, so it makes
sense
> that they're gone. it looks like we need to update the script to recognize
> functions on types, though.

Yep that's what I figured.  Cool.

I've filed a bug, crbug.com/105243

http://codereview.chromium.org/8670012/diff/1/chrome/test/data/extensions/api...
File chrome/test/data/extensions/api_test/settings/simple_test/background.html
(right):

http://codereview.chromium.org/8670012/diff/1/chrome/test/data/extensions/api...
chrome/test/data/extensions/api_test/settings/simple_test/background.html:9: //
Make sure chrome.test.succeed() is only called once (see succeed()).
On 2011/11/23 03:35:05, Matt Perry wrote:
> chrome.test.callbackPass might be better suited for this purpose. It creates
and
> returns a callback, and automatically calls succeed() when all callbacks have
> been run.

Nice.

[commit-bot: I haz the power, 2011-11-23 21:14:23]

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

[commit-bot: I haz the power, 2011-11-23 23:00:02]

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

[commit-bot: I haz the power, 2011-11-23 23:22:08]

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

[commit-bot: I haz the power, 2011-11-24 01:15:37]

Try job failure for 8670012-8031 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-11-24 02:24:20]

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

[commit-bot: I haz the power, 2011-11-24 04:17:26]

Change committed as 111494