Separate the syncing of extension settings and app settings into separate data

Separate the syncing of extension settings and app settings into separate data
types.

This adds syncable::APP_SETTINGS on top of the existing
syncable::EXTENSION_SETTINGS, and restructures code accordingly to accommodate.
This is so they can each be synced independently, and eventually tied to their
respective (EXTENSIONS or APPS) sync settings.


BUG=98488
TEST=ExtensionSettings*


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

[not at google - send to devlin, 2011-10-26 06:38:58]

Most of the changes here are actually restructuring the extensions stuff so that
there are two ExtensionSettingsBackends (one for extensions, one for apps), and
controlling access to them via ExtensionsSettingsFrontend.

Rather than RunWithBackend, split it up into the 3 functions that were being
done on a Backend: getting a Storage, passing it to sync (as a SyncableService),
and deleting extension data.  The former two are now routed by the Frontend to
the correct Backend, and the latter just deletes from both.

[akalin, 2011-10-28 06:04:54]

Looks pretty good.  Mostly some nits.

I'm less familiar with the extensions code (aside from the code I previously
reviewed), so maybe you should get someone on the extensions team to take a look
at this, too.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_apitest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_apitest.cc:60: // TODO(kalman):
test both EXTENSION_SETTINGS and APP_SETTINGS.
Do you know about parametrized tests?
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...

It may be easy enough to use those and get test coverage for APP_SETTINGS.  If
it's still not easy, then I'm okay with leaving this as a TODO.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:74:
backends->extensions_backend_.DeleteStorage(extension_id);
DCHECK FILE thread

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.h (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:45: // from the UI
thread.
The comment re. UI thread is correct, but it also applies to all other
functions.  Perhaps replace this with a comment above the functions like "All
public functions below must be called on the UI thread"?

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend_unittest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend_unittest.cc:49: //
TODO(kalman): explicitly test the two interact correctly.
consider parametrized tests here, too

(Although I guess testing interaction between them can stay as a TODO)

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_sync_unittest.cc:190: // sync by
roughly alternative which one to test.
Consider parametrized tests here

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_test_util.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.cc:41: DictionaryValue
manifest;
I'm pretty sure there's a test function floating around that does exactly this? 
I think it's somewhere in extensions/.  May be worth finding it and simply
calling it.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_test_util.h (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:23: const std::string&
extension_id, ExtensionSettingsFrontend* frontend);
#include <string>

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:23: const std::string&
extension_id, ExtensionSettingsFrontend* frontend);
forward-declare Frontend?

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:33: void
AddExtension(const std::string& id, bool is_app);
I think using Extension::Type (not sure what it's called) would be better.  (And
you can assert if it's THEME or something)

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/app_setting_specifics.proto (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/app_setting_specifics.proto:6: // This is the same
as for an extension setting, but uses a separate datatype
Don't forget to get this in server-side, too

[not at google - send to devlin, 2011-10-31 00:02:23]

(+mpcomplete)

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_apitest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_apitest.cc:60: // TODO(kalman):
test both EXTENSION_SETTINGS and APP_SETTINGS.
On 2011/10/28 06:04:54, akalin wrote:
> Do you know about parametrized tests?
>
http://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Par...
> 
> It may be easy enough to use those and get test coverage for APP_SETTINGS.  If
> it's still not easy, then I'm okay with leaving this as a TODO.

Yep I know about parameterised tests (the ExtensionSettingsStorage
implementations use them), and it's not particularly difficult to set up, though
it requires adding 2 more files so I was going to wait until the extension
settings stuff is moved into a subdirectory.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:74:
backends->extensions_backend_.DeleteStorage(extension_id);
On 2011/10/28 06:04:54, akalin wrote:
> DCHECK FILE thread

Done.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.h (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:45: // from the UI
thread.
On 2011/10/28 06:04:54, akalin wrote:
> The comment re. UI thread is correct, but it also applies to all other
> functions.  Perhaps replace this with a comment above the functions like "All
> public functions below must be called on the UI thread"?

Done.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend_unittest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend_unittest.cc:49: //
TODO(kalman): explicitly test the two interact correctly.
On 2011/10/28 06:04:54, akalin wrote:
> consider parametrized tests here, too
> 
> (Although I guess testing interaction between them can stay as a TODO)

Ditto my last comment.  I have a few things I need to do to clean up testing
actually, will do this once the files have moved directories.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_sync_unittest.cc:190: // sync by
roughly alternative which one to test.
On 2011/10/28 06:04:54, akalin wrote:
> Consider parametrized tests here

Ditto.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_test_util.cc (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.cc:41: DictionaryValue
manifest;
On 2011/10/28 06:04:54, akalin wrote:
> I'm pretty sure there's a test function floating around that does exactly
this? 
> I think it's somewhere in extensions/.  May be worth finding it and simply
> calling it.

There's only 1 caller of CreateWithId (surprisingly), which makes me think that
the test util (wherever it is) might not be suitable?

Btw I've seen other tests do this sort of thing manually.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_test_util.h (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:23: const std::string&
extension_id, ExtensionSettingsFrontend* frontend);
On 2011/10/28 06:04:54, akalin wrote:
> #include <string>

Done, but, why?

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:23: const std::string&
extension_id, ExtensionSettingsFrontend* frontend);
On 2011/10/28 06:04:54, akalin wrote:
> forward-declare Frontend?

Done.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:33: void
AddExtension(const std::string& id, bool is_app);
On 2011/10/28 06:04:54, akalin wrote:
> I think using Extension::Type (not sure what it's called) would be better. 
(And
> you can assert if it's THEME or something)

Done.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/app_setting_specifics.proto (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/app_setting_specifics.proto:6: // This is the same
as for an extension setting, but uses a separate datatype
On 2011/10/28 06:04:54, akalin wrote:
> Don't forget to get this in server-side, too

Which server?

I've updated the test one in net/, anything else?

[akalin, 2011-10-31 20:20:57]

LGTM

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_test_util.h (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_test_util.h:23: const std::string&
extension_id, ExtensionSettingsFrontend* frontend);
On 2011/10/31 00:02:23, kalman wrote:
> On 2011/10/28 06:04:54, akalin wrote:
> > #include <string>
> 
> Done, but, why?

You use it as a parameter.  It's probably pulled in via one of the other
includes, but the general style is include-what-you-use.

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
File chrome/browser/sync/protocol/app_setting_specifics.proto (right):

http://codereview.chromium.org/8375047/diff/5001/chrome/browser/sync/protocol...
chrome/browser/sync/protocol/app_setting_specifics.proto:6: // This is the same
as for an extension setting, but uses a separate datatype
On 2011/10/31 00:02:23, kalman wrote:
> On 2011/10/28 06:04:54, akalin wrote:
> > Don't forget to get this in server-side, too
> 
> Which server?
> 
> I've updated the test one in net/, anything else?

The actual server code (internal).  Not urgent, but good not to forget about it.

[Matt Perry, 2011-10-31 22:04:07]

extensions part lgtm

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_api.cc:70: observers->Notify(
are we assured that the observers list will live to this point? should it be
passed around as ascoped_refptr?

[akalin, 2011-10-31 22:05:56]

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_api.cc:70: observers->Notify(
On 2011/10/31 22:04:07, Matt Perry wrote:
> are we assured that the observers list will live to this point? should it be
> passed around as ascoped_refptr?

Ah, yes, I missed this.

Please pass around ref-counted objects via scoped_refptr<> (or const
scoped_refptr<>&)

[not at google - send to devlin, 2011-11-01 00:40:20]

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8375047/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_api.cc:70: observers->Notify(
On 2011/10/31 22:05:56, akalin wrote:
> On 2011/10/31 22:04:07, Matt Perry wrote:
> > are we assured that the observers list will live to this point? should it be
> > passed around as ascoped_refptr?
> 
> Ah, yes, I missed this.
> 
> Please pass around ref-counted objects via scoped_refptr<> (or const
> scoped_refptr<>&)

Done.

[commit-bot: I haz the power, 2011-11-01 00:40:50]

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

[commit-bot: I haz the power, 2011-11-01 01:56:48]

Change committed as 108055

[Nico, 2011-11-30 21:04:16]

http://codereview.chromium.org/8375047/diff/19001/chrome/browser/sync/syncabl...
File chrome/browser/sync/syncable/model_type.cc (right):

http://codereview.chromium.org/8375047/diff/19001/chrome/browser/sync/syncabl...
chrome/browser/sync/syncable/model_type.cc:641: *model_type = APP_SETTINGS;
This branch is missing a "return true;" as far as I can tell. I'm sending a CL.
I don't know the sync code, so I don't know if his would cause any problems in
practice, or why no tests caught this.

[not at google - send to devlin, 2011-11-30 21:13:29]

LGTM

Yeah my mistake.  I possibly also missed out on a test somewhere which
enumerates this stuff.

Aside: but I also wonder why it's not written as

*model_type = UNSPECIFIED;
if (notification_type == ...)
  *model_type = ...;
else if (...)
  *model_type = ...;
...
return *model_type != UNSPECIFIED;

Eh.

[not at google - send to devlin, 2011-11-30 21:18:06]

oops wrong CL... never mind...