Sync the launch type pref for apps.

Sync the launch type pref for apps.

This CL syncs the launch type pref for apps by adding a property to
app_specifics.proto. The sync data is processed in ExtensionSyncService.

extensions::SetLaunchType has been changed to take an ExtensionService
so that it is able to sync the pref change when a launch type is updated.

A value has been added to the LaunchType enum to represent an invalid
launch type preference.

BUG=181576
TBR=sky@chromium.org

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

[calamity, 2013-12-17 04:40:13]

+benwells for chrome/browser/extensions and chrome/common/extensions/ OWNERS.
+zea for sync/ OWNERS and high level sync review.

Thanks.

[Nicolas Zea, 2013-12-17 19:45:58]

High level questions: how is the launch type for a particular app set? Is it a
user-controlled preference? Do we ever change it automatically? Are there any
invalid values?

[calamity, 2013-12-18 04:58:30]

On 2013/12/17 19:45:58, Nicolas Zea wrote:
> High level questions: how is the launch type for a particular app set? Is it a
> user-controlled preference? Do we ever change it automatically? Are there any
> invalid values?

It's set by the user from the NTP, app launcher or shelf context menu.

It doesn't get changed automatically. Prefs do not begin with the value set so
it's nonexistent until set by the user. We return a default value in the case
that the preference hasn't been explicitly set.

Anything outside of the available options for the launch type (0-3) is an
invalid value. Changed checks to enforce this a bit more.

[Nicolas Zea, 2013-12-18 21:14:03]

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/app_sync_data.cc (right):

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/app_sync_data.cc:78: launch_type_ =
specifics.has_launch_type()
It might be worth considering what might happen if future versions introduce new
launch types. Not sure what the appropriate behavior should be in that case. I'm
guessing you'd want to preserve the "unknown" launch type, so that it isn't
overwritten.

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/launch_util.cc:90:
prefs->ReadPrefAsInteger(extension->id(), kPrefLaunchType, &value);
Just to double check, this will return false if the user has not set a specific
launch type for the extension right? (i.e. the default value case)

https://codereview.chromium.org/93883004/diff/80001/sync/protocol/app_specifi...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/93883004/diff/80001/sync/protocol/app_specifi...
sync/protocol/app_specifics.proto:61: optional int32 launch_type = 5;
Make this an enum as well?

[calamity, 2013-12-19 06:27:52]

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/app_sync_data.cc (right):

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/app_sync_data.cc:78: launch_type_ =
specifics.has_launch_type()
On 2013/12/18 21:14:04, Nicolas Zea wrote:
> It might be worth considering what might happen if future versions introduce
new
> launch types. Not sure what the appropriate behavior should be in that case.
I'm
> guessing you'd want to preserve the "unknown" launch type, so that it isn't
> overwritten.

Hmm yeah. We also wouldn't want to write an invalid value to the down-syncing
old client. I guess I could change the DCHECK in SetLaunchType to just ignore
invalid values for such a case?

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/launch_util.cc (right):

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/launch_util.cc:90:
prefs->ReadPrefAsInteger(extension->id(), kPrefLaunchType, &value);
On 2013/12/18 21:14:04, Nicolas Zea wrote:
> Just to double check, this will return false if the user has not set a
specific
> launch type for the extension right? (i.e. the default value case)

Yep.

https://codereview.chromium.org/93883004/diff/80001/sync/protocol/app_specifi...
File sync/protocol/app_specifics.proto (right):

https://codereview.chromium.org/93883004/diff/80001/sync/protocol/app_specifi...
sync/protocol/app_specifics.proto:61: optional int32 launch_type = 5;
On 2013/12/18 21:14:04, Nicolas Zea wrote:
> Make this an enum as well?

Done.

[Nicolas Zea, 2013-12-19 19:56:59]

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
File chrome/browser/extensions/app_sync_data.cc (right):

https://codereview.chromium.org/93883004/diff/80001/chrome/browser/extensions...
chrome/browser/extensions/app_sync_data.cc:78: launch_type_ =
specifics.has_launch_type()
On 2013/12/19 06:27:52, calamity wrote:
> On 2013/12/18 21:14:04, Nicolas Zea wrote:
> > It might be worth considering what might happen if future versions introduce
> new
> > launch types. Not sure what the appropriate behavior should be in that case.
> I'm
> > guessing you'd want to preserve the "unknown" launch type, so that it isn't
> > overwritten.
> 
> Hmm yeah. We also wouldn't want to write an invalid value to the down-syncing
> old client. I guess I could change the DCHECK in SetLaunchType to just ignore
> invalid values for such a case?

See my other comment. If we're introducing a mismatch between the local data and
the sync data, we need to be careful about how sync changes are triggered.

https://codereview.chromium.org/93883004/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/93883004/diff/100001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:316:
app_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {
I suspect this is actually going to result in automatically overwriting the sync
data if it contains a value that the local version doesn't understand (launch
type > num_launch_types).

In other datatypes we typically have a scoped value that's true in
ProcessSyncChanges, which prevents triggering the sync code from sync-initiated
changes. From what I can tell extensions/apps don't have this, but it might be
they should (i.e. return immediately in SyncExtensionChangeIfNeeded if the value
is set).

Otherwise what happens is that, as soon as any change comes in, the extension
update code gets triggered, which itself calls back into sync, but generates the
sync data from the in-memory representation, which won't match for launch_type.

Some unit tests that verify the future launch type case would probably be
useful.

[calamity, 2013-12-30 06:13:31]

Also had to change sync_app_helper.cc which did the comparison of synced apps.
The tests weren't actually checking the launch type before.

https://codereview.chromium.org/93883004/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/93883004/diff/100001/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:316:
app_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {
On 2013/12/19 19:57:01, Nicolas Zea wrote:
> I suspect this is actually going to result in automatically overwriting the
sync
> data if it contains a value that the local version doesn't understand (launch
> type > num_launch_types).
> 
> In other datatypes we typically have a scoped value that's true in
> ProcessSyncChanges, which prevents triggering the sync code from
sync-initiated
> changes. From what I can tell extensions/apps don't have this, but it might be
> they should (i.e. return immediately in SyncExtensionChangeIfNeeded if the
value
> is set).
> 
> Otherwise what happens is that, as soon as any change comes in, the extension
> update code gets triggered, which itself calls back into sync, but generates
the
> sync data from the in-memory representation, which won't match for
launch_type.
> 
> Some unit tests that verify the future launch type case would probably be
> useful.

Ack, it's tricky to test this case because there are so many checks around it
not happening. In particular, since we're using an enum for the LaunchType in
AppSpecifics, I can't even sync an invalid value to test this case.

Currently, the system will be robust to down-syncing bad values because protobuf
will treat the invalid value as an unknown message as outlined at
https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#enum.

The ExtensionSyncService ignores invalid values and doesn't trigger a resync.

I wrote a test that processes an invalid AppSyncData. Let me know if you can
think of anything else.

[Nicolas Zea, 2014-01-02 22:13:30]

sync LGTM

[benwells, 2014-01-06 05:56:31]

lgtm with two baby nits

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:314:
app_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {
Nit: Could this check move into AppSyncData::PopulateFromAppSpecifics? It would
then match the check in PopulateAppSpecifics and feels more naturaly there.

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:316:
static_cast<extensions::LaunchType>(app_sync_data.launch_type()));
Nit: why is this static cast needed? The type is already extensions::LaunchType.

[calamity, 2014-01-07 03:29:41]

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:314:
app_sync_data.launch_type() < extensions::NUM_LAUNCH_TYPES) {
On 2014/01/06 05:56:31, benwells wrote:
> Nit: Could this check move into AppSyncData::PopulateFromAppSpecifics? It
would
> then match the check in PopulateAppSpecifics and feels more naturaly there.

Done.

https://codereview.chromium.org/93883004/diff/190002/chrome/browser/extension...
chrome/browser/extensions/extension_sync_service.cc:316:
static_cast<extensions::LaunchType>(app_sync_data.launch_type()));
On 2014/01/06 05:56:31, benwells wrote:
> Nit: why is this static cast needed? The type is already
extensions::LaunchType.

Done.

[calamity, 2014-01-07 04:05:25]

TBR-ing sky@ for refactoring fallout.

extensions::SetLaunchType now takes an ExtensionService instead of
ExtensionPrefs so that it can sync changes.

chrome/browser/ui/ash/launcher/launcher_context_menu.cc needed a change to
handle a new enum value.

[calamity, 2014-01-07 04:10:09]

On 2014/01/07 04:05:25, calamity wrote:
> TBR-ing sky@ for refactoring fallout.
> 
> extensions::SetLaunchType now takes an ExtensionService instead of
> ExtensionPrefs so that it can sync changes.
> 
> chrome/browser/ui/ash/launcher/launcher_context_menu.cc needed a change to
> handle a new enum value.

Relevant files are:
- chrome/browser/automation/testing_automation_provider.cc
- chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
- chrome/browser/ui/startup/startup_browser_creator_browsertest.cc
- chrome/browser/ui/webui/ntp/app_launcher_handler.cc

[sky, 2014-01-07 15:51:37]

Said files LGTM

[commit-bot: I haz the power, 2014-01-09 05:22:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/93883004/680001

[commit-bot: I haz the power, 2014-01-09 06:32:55]

Retried try job too often on mac_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-10 05:18:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/calamity@chromium.org/93883004/980001

[commit-bot: I haz the power, 2014-01-10 06:48:39]

Change committed as 244112