Add ExtensionsApiClient interface, use it in the storage API SettingsFrontend

Add ExtensionsApiClient interface, use it in the storage API SettingsFrontend

app_shell needs the chrome.storage.local extension API so the core storage implementation is moving to //extensions/browser/api. However, the "managed storage" API implementation needs to remain in Chrome.

* Introduce ExtensionsApiClient in extensions/browser/api
* Introduce ChromeExtensionsApiClient in chrome/browser/extensions/api
* Delegate out the storage ManagedValueStoreCache creation to Chrome
* Move settings_namespace.h/cc to e/b/api/storage so we can use an enum in the client interface definition

Most APIs will probably need to make only one or two client interface calls, so we're starting with a single ExtensionsApiClient interface. It we find individual APIs tend to need a lot of methods we can split it into one client interface per API, but that seems like overkill right now.

BUG=348058
TEST=unit_tests *Settings* and browser_tests ExtensionSettings*
R=rdevlin.cronin@chromium.org, rockot@chromium.org, yoz@chromium.org

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

[James Cook, 2014-03-01 00:24:29]

Devlin, PTAL at chrome/browser/extensions/api/storage, in particular at
settings_frontend.cc for use of the client interface. The rest is mostly fallout
from the settings_namespace file move.  (Aside: Should we get rid of that
settings_namespace::Namespace enum and just use the string constants?)

Ken, PTAL at extensions/browser/api/* and
chrome/browser/extensions/api/chrome_extensions_api_client.h/cc.  I think this
is the shape of the interface we discussed.

[Ken Rockot(use gerrit already), 2014-03-01 02:16:35]

On 2014/03/01 00:24:29, James Cook wrote:
> Devlin, PTAL at chrome/browser/extensions/api/storage, in particular at
> settings_frontend.cc for use of the client interface. The rest is mostly
fallout
> from the settings_namespace file move.  (Aside: Should we get rid of that
> settings_namespace::Namespace enum and just use the string constants?)
> 
> Ken, PTAL at extensions/browser/api/* and
> chrome/browser/extensions/api/chrome_extensions_api_client.h/cc.  I think this
> is the shape of the interface we discussed.

ExtensionAPIClient stuff LGTM. I'm sure you're no more fond than I am of all the
arguments needed by AddAdditionalFoo, but I believe it's fine for now.

[Devlin, 2014-03-03 17:07:59]

storage stuff lgtm, a few questions on the rest (but, of course, I'm happy to
defer to others' opinions there).

On a larger note, I do think that having one ExtensionsApiClient with all the
relevant API calls is going to quickly get messy (bear in mind that even if we
have only one call in the ExtensionsApiClient per API, that's still going to be
on the order of 100 different calls*).  I'm also not sure that having one
ApiClient for each API is a good idea (for the same reason of there being over
100 different APIs*).

But that's just my opinion, and, as I said, I'm happy to defer to others. :)

* guestimated based on a naive method of counting the directories in c/b/e/api.

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.cc:39:
ExtensionsApiClient::Set(api_client_.get());
I'm quite certain I'm paranoid here, but I might prefer this to go into
ExtensionsApiClient's constructor/destructor, so that we are sure to set
appropriately (pending any option to deliberately reset a client).

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.h:82:
scoped_ptr<ChromeExtensionsApiClient> api_client_;
Would it make sense to put this in ExtensionsBrowserClient (since it will
presumably be in all implementations)?

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
File extensions/browser/api/extensions_api_client.cc (right):

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
extensions/browser/api/extensions_api_client.cc:29: g_client = client;
Should we ever be able to quietly reset a client?  That is, could we
DCHECK(!g_client) here?

[James Cook, 2014-03-03 19:53:08]

yoz, PTAL for OWNERS and your opinion on:
A) Creation of the ExtensionsApiClient interface, and
B) Who tracks the instance of ExtensionsApiClient

If you like devlin's suggestion on instance tracking in the
constructor/destructor I'll convert the other *Client interfaces to use that
pattern in a separate CL.

Devlin, yeah, I suspect we might need one client/delegate interface per API. But
it seemed like overengineering it to build that for the very first API we
extract. Ken is working on the sockets API and apparently doesn't need to
delegate out anything.

I figured we could refactor the client interface when/if we need to. I added a
comment to ExtensionsApiClient encouraging it to be split up if it gains too
many methods.

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.cc:39:
ExtensionsApiClient::Set(api_client_.get());
On 2014/03/03 17:07:59, D Cronin wrote:
> I'm quite certain I'm paranoid here, but I might prefer this to go into
> ExtensionsApiClient's constructor/destructor, so that we are sure to set
> appropriately (pending any option to deliberately reset a client).

SGTM. Done.

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.h:82:
scoped_ptr<ChromeExtensionsApiClient> api_client_;
On 2014/03/03 17:07:59, D Cronin wrote:
> Would it make sense to put this in ExtensionsBrowserClient (since it will
> presumably be in all implementations)?

Unless you feel strongly I'd like to keep it here. It seems like we can either
have ExtensionsBrowserClient take care of Set(instance) and Set(NULL) in its
constructor/destructor (which SGTM) or we can have a public Set(foo) method that
takes ownership of |foo|.

Maybe there's some way to accomplish both but it seems like it would be ornate
and I would prefer the former.

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
File extensions/browser/api/extensions_api_client.cc (right):

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
extensions/browser/api/extensions_api_client.cc:29: g_client = client;
On 2014/03/03 17:07:59, D Cronin wrote:
> Should we ever be able to quietly reset a client?  That is, could we
> DCHECK(!g_client) here?

We used to have DCHECK(!g_client) in the other extension client interfaces but I
had to take them out because it's useful to reset them during tests.

[Devlin, 2014-03-03 20:25:26]

(still lgtm)

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.h:82:
scoped_ptr<ChromeExtensionsApiClient> api_client_;
On 2014/03/03 19:53:09, James Cook wrote:
> On 2014/03/03 17:07:59, D Cronin wrote:
> > Would it make sense to put this in ExtensionsBrowserClient (since it will
> > presumably be in all implementations)?
> 
> Unless you feel strongly I'd like to keep it here. It seems like we can either
> have ExtensionsBrowserClient take care of Set(instance) and Set(NULL) in its
> constructor/destructor (which SGTM) or we can have a public Set(foo) method
that
> takes ownership of |foo|.
> 
> Maybe there's some way to accomplish both but it seems like it would be ornate
> and I would prefer the former.

I don't feel strongly at all, so this is fine.  But, for what it's worth, all I
was thinking was:
class ExtensionsBrowserClient {
ExtensionsBrowserClient(ExtensionsApiClient* client) : api_client_(client) {}
...
scoped_ptr<ExtensionsApiClient> api_client_;
};

ChromeExtensionsBrowserClient::ChromeExtensionsBrowserClient()
    : ExtensionsBrowserClient(new ChromeExtensionsApiClient) {...}

ShellExtensionsBrowserClient::ShellExtensionsBrowserClient()
    : ExtensionsBrowserClient(new ShellExtensionsApiClient) {...}

which doesn't seem overlay ornate.  But again, I'm fine with this.

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
File extensions/browser/api/extensions_api_client.cc (right):

https://codereview.chromium.org/180213008/diff/1/extensions/browser/api/exten...
extensions/browser/api/extensions_api_client.cc:29: g_client = client;
On 2014/03/03 19:53:09, James Cook wrote:
> On 2014/03/03 17:07:59, D Cronin wrote:
> > Should we ever be able to quietly reset a client?  That is, could we
> > DCHECK(!g_client) here?
> 
> We used to have DCHECK(!g_client) in the other extension client interfaces but
I
> had to take them out because it's useful to reset them during tests.

Ah, okay.

[Yoyo Zhou, 2014-03-04 19:55:03]

LGTM

I think this is ok. I think traditionally there would be something like a
SettingsFrontendDelegate interface that could be implemented variously (e.g. 1
per API, or 1 for all APIs), except that we have what are essentially global
delegate objects. If we're going to have more than 1 ApiClient, then we need to
come up with a way to wrangle them, so I'm happy with 1 ApiClient for all APIs
for now, and re-evaluating that if it gets out of hand.

(By the way, I think the preferred spelling is "APIClient", but I know we're not
very consistent about it.)

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
File chrome/browser/extensions/chrome_extensions_browser_client.h (right):

https://codereview.chromium.org/180213008/diff/1/chrome/browser/extensions/ch...
chrome/browser/extensions/chrome_extensions_browser_client.h:82:
scoped_ptr<ChromeExtensionsApiClient> api_client_;
On 2014/03/03 20:25:27, D Cronin wrote:
> On 2014/03/03 19:53:09, James Cook wrote:
> > On 2014/03/03 17:07:59, D Cronin wrote:
> > > Would it make sense to put this in ExtensionsBrowserClient (since it will
> > > presumably be in all implementations)?
> > 
> > Unless you feel strongly I'd like to keep it here. It seems like we can
either
> > have ExtensionsBrowserClient take care of Set(instance) and Set(NULL) in its
> > constructor/destructor (which SGTM) or we can have a public Set(foo) method
> that
> > takes ownership of |foo|.
> > 
> > Maybe there's some way to accomplish both but it seems like it would be
ornate
> > and I would prefer the former.
> 
> I don't feel strongly at all, so this is fine.  But, for what it's worth, all
I
> was thinking was:
> class ExtensionsBrowserClient {
> ExtensionsBrowserClient(ExtensionsApiClient* client) : api_client_(client) {}
> ...
> scoped_ptr<ExtensionsApiClient> api_client_;
> };
> 
> ChromeExtensionsBrowserClient::ChromeExtensionsBrowserClient()
>     : ExtensionsBrowserClient(new ChromeExtensionsApiClient) {...}
> 
> ShellExtensionsBrowserClient::ShellExtensionsBrowserClient()
>     : ExtensionsBrowserClient(new ShellExtensionsApiClient) {...}
> 
> which doesn't seem overlay ornate.  But again, I'm fine with this.

Do we want to be able to handle ExtensionsApiClient::Get() ever returning NULL?

https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/chrome_extensions_api_client.h (right):

https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/chrome_extensions_api_client.h:13: // Support for
extensions APIs in Chrome.
nit: This is more like extra support.

https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
File extensions/browser/api/extensions_api_client.h (right):

https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:29: class ExtensionsApiClient {
Since this provides a default implementation, I'd mention that in the class
comment.

[James Cook, 2014-03-04 20:55:57]

Changed spelling to APIClient. I swear there used to be a rule that it was
supposed to be ApiClient (and Url and Tts) but I can't find it.  API it is.

https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/chrome_extensions_api_client.h (right):

https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/chrome_extensions_api_client.h:13: // Support for
extensions APIs in Chrome.
On 2014/03/04 19:55:04, Yoyo Zhou wrote:
> nit: This is more like extra support.

Done.

https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
File extensions/browser/api/extensions_api_client.h (right):

https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
extensions/browser/api/extensions_api_client.h:29: class ExtensionsApiClient {
On 2014/03/04 19:55:04, Yoyo Zhou wrote:
> Since this provides a default implementation, I'd mention that in the class
> comment.

Done.

[Ken Rockot(use gerrit already), 2014-03-04 21:02:40]

On 2014/03/04 20:55:57, James Cook wrote:
> Changed spelling to APIClient. I swear there used to be a rule that it was
> supposed to be ApiClient (and Url and Tts) but I can't find it.  API it is.
> 
>
https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
> File chrome/browser/extensions/api/chrome_extensions_api_client.h (right):
> 
>
https://codereview.chromium.org/180213008/diff/180001/chrome/browser/extensio...
> chrome/browser/extensions/api/chrome_extensions_api_client.h:13: // Support
for
> extensions APIs in Chrome.
> On 2014/03/04 19:55:04, Yoyo Zhou wrote:
> > nit: This is more like extra support.
> 
> Done.
> 
>
https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
> File extensions/browser/api/extensions_api_client.h (right):
> 
>
https://codereview.chromium.org/180213008/diff/180001/extensions/browser/api/...
> extensions/browser/api/extensions_api_client.h:29: class ExtensionsApiClient {
> On 2014/03/04 19:55:04, Yoyo Zhou wrote:
> > Since this provides a default implementation, I'd mention that in the class
> > comment.
> 
> Done.

I could have sworn there was a rule that encouraged Acronym instead of ACRONYM.
I've been living a lie.

[James Cook, 2014-03-04 21:17:46]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-04 21:22:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/180213008/200001

[commit-bot: I haz the power, 2014-03-04 22:13:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 22:13:48]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[James Cook, 2014-03-04 22:34:10]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-04 22:35:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/180213008/200001

[commit-bot: I haz the power, 2014-03-05 00:08:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:08:29]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, gfx_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[James Cook, 2014-03-05 00:11:44]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-05 00:13:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/180213008/200001

[James Cook, 2014-03-05 00:14:17]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2014-03-05 00:34:39]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-05 01:23:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/180213008/240001

[commit-bot: I haz the power, 2014-03-05 06:41:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 06:41:14]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ken Rockot(use gerrit already), 2014-03-05 07:14:41]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2014-03-05 07:16:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamescook@chromium.org/180213008/240001

[James Cook, 2014-03-05 17:25:27]

Committed patchset #7 manually as r255069 (presubmit successful).

[Nico, 2014-03-05 19:37:05]

A revert of this CL has been created in
https://codereview.chromium.org/181233011/ by thakis@chromium.org.

The reason for reverting is: Likely broke
GCMProfileServiceSingleProfileTest.Unregister Android..