Turn on SyncedNotifications in Dev and Canary

Turn on SyncedNotifications for Dev, Canary, and Unknown channel

Turning it on in Dev and Canary will allow us to gather feedback during the development process.

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

[Dmitry Titov, 2013-03-19 18:05:22]

Why not just enable it on Canary and Dev builds w/o any additional flag?

[Pete Williamson, 2013-03-19 21:30:14]

On 2013/03/19 18:05:22, Dmitry Titov wrote:
> Why not just enable it on Canary and Dev builds w/o any additional flag?

Done

[Pete Williamson, 2013-03-19 21:30:50]

OK, I removed the switch, and turned it on by default in developer machine
builds, dev, and canary.

[Dmitry Titov, 2013-03-19 21:31:46]

lgtm

[commit-bot: I haz the power, 2013-03-20 20:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/12617010/4001

[commit-bot: I haz the power, 2013-03-20 20:25:19]

Presubmit check for 12617010-4001 failed and returned exit status 1.

INFO:root:Found 1 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/sync/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/sync/profile_sync_components_factory_impl.cc

Presubmit checks took 3.7s to calculate.

[Pete Williamson, 2013-03-20 20:29:59]

zea@ - May I please get OWNERS approval for this change.  Dimich has already
given it approval.

[dcheng, 2013-03-20 20:37:07]

On 2013/03/20 20:29:59, Pete Williamson wrote:
> zea@ - May I please get OWNERS approval for this change.  Dimich has already
> given it approval.

Don't forget to update the CL description. We're not adding anything to
chrome://flags anymore =)

[Pete Williamson, 2013-03-20 20:39:54]

On 2013/03/20 20:37:07, dcheng wrote:
> On 2013/03/20 20:29:59, Pete Williamson wrote:
> > zea@ - May I please get OWNERS approval for this change.  Dimich has already
> > given it approval.
> 
> Don't forget to update the CL description. We're not adding anything to
> chrome://flags anymore =)

Done.

[Nicolas Zea, 2013-03-20 21:05:10]

On 2013/03/20 20:39:54, Pete Williamson wrote:
> On 2013/03/20 20:37:07, dcheng wrote:
> > On 2013/03/20 20:29:59, Pete Williamson wrote:
> > > zea@ - May I please get OWNERS approval for this change.  Dimich has
already
> > > given it approval.
> > 
> > Don't forget to update the CL description. We're not adding anything to
> > chrome://flags anymore =)
> 
> Done.

Are SyncedNotifications really ready to be on by default? A quick glance makes
it look like StopSyncing() is not yet implemented and updates are not yet
handled (i.e. other clients marking notifications as read).

Given that this does not appear to be finished yet, I'd rather have it behind a
flag, and let developers/testers opt in via the flag.

[Dmitry Titov, 2013-03-20 21:12:34]

The motivation to remove an explicit flag in about://flags and enable the
feature in Canary/Dev builds was that it requires multiple steps of opt-in. By
just enabling it in Chrome we don't change any behavior. There is also a
whitelist on the server side and there will be a settings UI.
Also, I think we as a team are going away from multiplying about://flags in
reasonable cases and instead enabling features on Canary/Dev (but not
Beat/Stable) while they are being developed. Obviously, not all cases can be
treated this way - but in this case, because enabling the feature does not
change anything at all (one need to be in a restricted whitelist to see anything
new) it seems like a good way to avoid multiple flags and confusion.
Does it make sense?

[Nicolas Zea, 2013-03-20 21:23:54]

On 2013/03/20 21:12:34, Dmitry Titov wrote:
> The motivation to remove an explicit flag in about://flags and enable the
> feature in Canary/Dev builds was that it requires multiple steps of opt-in. By
> just enabling it in Chrome we don't change any behavior. There is also a
> whitelist on the server side and there will be a settings UI.
> Also, I think we as a team are going away from multiplying about://flags in
> reasonable cases and instead enabling features on Canary/Dev (but not
> Beat/Stable) while they are being developed. Obviously, not all cases can be
> treated this way - but in this case, because enabling the feature does not
> change anything at all (one need to be in a restricted whitelist to see
anything
> new) it seems like a good way to avoid multiple flags and confusion.
> Does it make sense?

I see, so most dev/canary users will not be affected by this.

In that case, could you convert the existing kEnableSyncSyncedNotifications
switch into a kDisableSyncSyncedNotifications switch that prevents the datatype
from being enabled (even for canary/dev)? 

That way, if there is a startup crash/issue, a user can identify which datatype
is causing it by passing that switch (we use this approach for all our
datatypes).

[Pete Williamson, 2013-03-20 22:23:43]

OK, I changed the "enable" switch to a "disable" switch.  Note that this means
we now have no way to turn it on in stable, but that is ok until the feature is
closer to done.

[Nicolas Zea, 2013-03-20 22:37:03]

LGTM, with a nit and the caveat I mentioned offline

https://codereview.chromium.org/12617010/diff/17001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/12617010/diff/17001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_components_factory_impl.cc:249: #if
!defined(OS_ANDROID)
FYI, you can get rid of this #if. RegisterDesktopDatatypes is only called on
desktop OS's. Android will call RegisterCommonDatatypes

[commit-bot: I haz the power, 2013-03-20 23:01:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/12617010/13002

[commit-bot: I haz the power, 2013-03-21 00:40:40]

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

[commit-bot: I haz the power, 2013-03-21 17:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/12617010/32001

[commit-bot: I haz the power, 2013-03-22 18:26:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/12617010/32001

[commit-bot: I haz the power, 2013-03-23 15:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/petewil@chromium.org/12617010/32001

[commit-bot: I haz the power, 2013-03-23 21:47:27]

Change committed as 190089