Split up extensions storage API implementations for sync and local storage.

Split up extensions storage API implementations for sync and local storage.

This will allow the core storage API implementation to move to //extensions where app_shell can use it.

* Split SyncOrLocalValueStoreCache into SyncValueStoreCache and LocalValueStoreCache
* Move SettingsFrontend::GetBackendForSync() to settings_sync_util::GetSyncableService()

BUG=348058
TEST=unit_tests *Settings* and browser_tests ExtensionSettings*
TBR=zea@chromium.org for trivial change to chrome/browser/sync/profile_sync_components_factory_impl.cc

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

[James Cook, 2014-03-05 18:37:39]

Devlin, PTAL

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.h (left):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.h:60: bool
initialized_;
I removed this because it was only used for DCHECKs and the existence of a
non-null app_backend_ or extension_backend_ implies the class was initialized.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.h (left):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.h:88: const
SettingsStorageQuotaEnforcer::Limits local_quota_limit_;
These were only used to init value store caches. I moved the limits into
sync_value_store_cache.cc and local_value_store_cache.cc.

[Devlin, 2014-03-05 19:02:55]

Hmm... I dislike how much shared code there is between the two ValueStoreCaches.
 But I don't really see a way around it yet.  So I guess we'll live with it for
now.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.cc (right):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:67: ValueStore*
storage = backend->GetStorage(extension->id());
If !initialized, we crash and burn at this line.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:72: // But only
if it's local storage (bad stuff would happen if sync'ed
you can take out the stuff about sync'ed storage in the comment now.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:84:
app_backend_->DeleteStorage(extension_id);
that DCHECK(initialized_) made sure we don't dereference NULL.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:91:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
DCHECK(!initialized_) was useful here, too, IMO.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.h (left):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.h:60: bool
initialized_;
On 2014/03/05 18:37:39, James Cook wrote:
> I removed this because it was only used for DCHECKs and the existence of a
> non-null app_backend_ or extension_backend_ implies the class was initialized.

I think I'd still like this.  You're right that non-null implies the class was
initialized, but DCHECK(initialized_) is faster and cleaner than 

DCHECK(app_backend_.get());
DCHECK(extension_backend_.get());

and I think the DCHECKs in general were useful.

See also other comments.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_sync_util.cc (right):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_sync_util.cc:111:
syncer::ModelType type) {
There used to be a DCHECK(OnFileThread) here (back when it was
SettingsFrontend::GetBackendForSync).  Is it no longer needed?

[James Cook, 2014-03-05 23:14:27]

Devlin, please take another look.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.cc (right):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:67: ValueStore*
storage = backend->GetStorage(extension->id());
Added back DCHECK(initialized_).

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:72: // But only
if it's local storage (bad stuff would happen if sync'ed
On 2014/03/05 19:02:55, D Cronin wrote:
> you can take out the stuff about sync'ed storage in the comment now.

Done.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:84:
app_backend_->DeleteStorage(extension_id);
On 2014/03/05 19:02:55, D Cronin wrote:
> that DCHECK(initialized_) made sure we don't dereference NULL.

Done.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.cc:91:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2014/03/05 19:02:55, D Cronin wrote:
> DCHECK(!initialized_) was useful here, too, IMO.

Done.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.h (left):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.h:60: bool
initialized_;
OK, added initialized_ back.

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_sync_util.cc (right):

https://codereview.chromium.org/184043031/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_sync_util.cc:111:
syncer::ModelType type) {
On 2014/03/05 19:02:55, D Cronin wrote:
> There used to be a DCHECK(OnFileThread) here (back when it was
> SettingsFrontend::GetBackendForSync).  Is it no longer needed?

Added back.  (I was relying on the DCHECK in
SyncValueStoreCache::GetSyncableService() but it's clearer to be explicit here.)

[Devlin, 2014-03-05 23:30:53]

lgtm with nits.

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.h (right):

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
nit: I could be wrong, but I think the verdict for a file like this is that it's
considered "new", and should have an updated copyright. (Same for
sync_value_store_cache.)

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.cc (right):

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.cc:27: namespace storage
= api::storage;
nit: since this is now only used on line 53, please get rid of it and just use
api::storage.

[James Cook, 2014-03-05 23:45:19]

yoz, can I get OWNERS for moving a little code into
chrome/browser/extensions/api/chrome_extensions_api_client.cc ?

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/local_value_store_cache.h (right):

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/storage/local_value_store_cache.h:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2014/03/05 23:30:53, D Cronin wrote:
> nit: I could be wrong, but I think the verdict for a file like this is that
it's
> considered "new", and should have an updated copyright. (Same for
> sync_value_store_cache.)

Done.

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.cc (right):

https://codereview.chromium.org/184043031/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.cc:27: namespace storage
= api::storage;
On 2014/03/05 23:30:53, D Cronin wrote:
> nit: since this is now only used on line 53, please get rid of it and just use
> api::storage.

Done.

[Yoyo Zhou, 2014-03-06 00:43:22]

LGTM

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.cc (right):

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.cc:96: new
LocalValueStoreCache(factory, browser_context_->GetPath());
Why not do this one in the APIClient too?

[James Cook, 2014-03-06 00:47:14]

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.cc (right):

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.cc:96: new
LocalValueStoreCache(factory, browser_context_->GetPath());
On 2014/03/06 00:43:22, Yoyo Zhou wrote:
> Why not do this one in the APIClient too?

Do you mean to have the APIClient always be responsible for adding all the value
store caches, have ChromeExtensionsAPIClient add local/managed/sync and
introduce a ShellExtensionsAPIClient to add local?  Right now app_shell just
uses the stub implementation for ExtensionsAPIClient, but I could change that if
you think it would be better.

I'll still end up moving the LocalValueStoreCache implementation into
//extensions, since app_shell needs it regardless.

[Yoyo Zhou, 2014-03-06 01:22:54]

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/storage/settings_frontend.cc (right):

https://codereview.chromium.org/184043031/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/storage/settings_frontend.cc:96: new
LocalValueStoreCache(factory, browser_context_->GetPath());
On 2014/03/06 00:47:14, James Cook wrote:
> On 2014/03/06 00:43:22, Yoyo Zhou wrote:
> > Why not do this one in the APIClient too?
> 
> Do you mean to have the APIClient always be responsible for adding all the
value
> store caches, have ChromeExtensionsAPIClient add local/managed/sync and
> introduce a ShellExtensionsAPIClient to add local?  Right now app_shell just
> uses the stub implementation for ExtensionsAPIClient, but I could change that
if
> you think it would be better.
> 
> I'll still end up moving the LocalValueStoreCache implementation into
> //extensions, since app_shell needs it regardless.

I don't have a strong preference. If it requires adding a
ShellExtensionsAPIClient then I say don't do it yet.

[James Cook, 2014-03-06 18:20:35]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-06 18:25:17]

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

[commit-bot: I haz the power, 2014-03-06 18:30:54]

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

[commit-bot: I haz the power, 2014-03-06 18:30:54]

Try jobs failed on following builders: mac_chromium_compile_dbg,
mac_chromium_rel

[James Cook, 2014-03-06 18:41:10]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-06 18:45:45]

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

[commit-bot: I haz the power, 2014-03-06 18:57:34]

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

[commit-bot: I haz the power, 2014-03-06 18:57:34]

Try jobs failed on following builders: mac_chromium_rel

[James Cook, 2014-03-06 19:03:33]

The CQ bit was checked by jamescook@chromium.org

[James Cook, 2014-03-06 19:03:34]

The CQ bit was unchecked by jamescook@chromium.org

[James Cook, 2014-03-06 19:03:36]

The CQ bit was checked by jamescook@chromium.org

[commit-bot: I haz the power, 2014-03-06 19:08:52]

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

[commit-bot: I haz the power, 2014-03-06 20:58:40]

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

[commit-bot: I haz the power, 2014-03-06 21:44:46]

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

[commit-bot: I haz the power, 2014-03-06 22:20:39]

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

[commit-bot: I haz the power, 2014-03-07 03:52:28]

Change committed as 255515