[Sync] Allow sync start without sign-in if the local sync backend is on.

[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests

Committed: https://crrev.com/645df3089e27103dbe55998c0c3692236d281e53
Cr-Commit-Position: refs/heads/master@{#440075}

[pastarmovj, 2016-11-11 10:24:30]

Description was changed from

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

BUG=651409
TEST=components_unittests
==========

to

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests
==========

[pastarmovj, 2016-11-14 13:50:12]

pastarmovj@chromium.org changed reviewers:
+ pavely@chromium.org

[pastarmovj, 2016-11-14 13:50:12]

Hello Pavel,

this is the next piece of the puzzle. Please review it when you have time for
it. I took extra care with this one to not change the behavior for the general
case and covered the new code paths with unit tests.

Btw one refactoring I made to code from the last CL which I believe spearates
the responsibilites better was to move the flags handling logic up to the
SyncProfileService as it is part of the chrome browser logic and not intrinsic
to the Sync code itself. I think this makes the code more maintable and
testable.

Best,
Julian

[pastarmovj, 2016-11-17 13:35:22]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 13:35:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pastarmovj, 2016-11-17 13:40:29]

On 2016/11/14 13:50:12, pastarmovj wrote:
> Hello Pavel,
> 
> this is the next piece of the puzzle. Please review it when you have time for
> it. I took extra care with this one to not change the behavior for the general
> case and covered the new code paths with unit tests.
> 
> Btw one refactoring I made to code from the last CL which I believe spearates
> the responsibilites better was to move the flags handling logic up to the
> SyncProfileService as it is part of the chrome browser logic and not intrinsic
> to the Sync code itself. I think this makes the code more maintable and
> testable.
> 
> Best,
> Julian

gentle ping?

btw if you are wondering why the dry run of the CQ - it's not to push you but to
verify that the CL still passes on all bots after the rebase which required some
work. :)

[commit-bot: I haz the power, 2016-11-17 13:42:15]

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

[commit-bot: I haz the power, 2016-11-17 13:42:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Nicolas Zea, 2016-11-19 00:36:41]

zea@chromium.org changed reviewers:
+ zea@chromium.org

[Nicolas Zea, 2016-11-19 00:36:42]

I'm concerned about introducing this new boolean that you have to remember to
check everywhere. I'd much prefer to instead have e.g. an overriden signin
manager that returns the appropriate values, and in general trying to preserve
Sync's behavior as much as possible by simply injecting in any dependencies that
would change.

Do you think that's feasible?

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:167: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
should all of this logic be dependent on local_sync_backend_enabled? Perhaps put
within a conditional?

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:225:
init_params.start_behavior = ProfileSyncService::AUTO_START;
I think it's cleaner to move this up to the #if defined(OS_WIN) section

https://codereview.chromium.org/2494873003/diff/40001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2494873003/diff/40001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:252:
(local_sync_backend_enabled_ || IsSignedIn()));
It seems like IsSyncRequested/IsSignedIn/IsSyncAllowed should all return true if
the sync backend is enabled. I worry overriding CanSyncStart but not those will
lead to bugs.

[pastarmovj, 2016-11-22 12:38:08]

Hi Nicolas,

Thanks for the review!

I considered the option of mocking the sign-in manager but couldn't see a way to
go around still spreading some more checks in the Sync code because it does also
rely on checking network status etc. I was actually exactly careful to make sure
there are as few places that need the new check as possible and I can get rid of
some of them which are only present as to make coding mistakes easy to catch.
For example making DCHECKs for credentials conditional on the flag instead of
filling them with mock data. Also not populating the whole arguments structure
in the ProfileSycnServiceFactory. Technically not needed but added to make it
easily visible crash whenever the Sync system tries to make a network call while
in local mode. I counted 9 places that are now conditionally doing something
different according to the flag out of which at least 3 or 4 can easily be
removed in favor of filling in some fake credentials.

Happy to reconsider any part of this CL on your guidance.

Thanks,
Julian

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:167: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2016/11/19 00:36:42, Nicolas Zea wrote:
> should all of this logic be dependent on local_sync_backend_enabled? Perhaps
put
> within a conditional?

Done.

https://codereview.chromium.org/2494873003/diff/40001/chrome/browser/sync/pro...
chrome/browser/sync/profile_sync_service_factory.cc:225:
init_params.start_behavior = ProfileSyncService::AUTO_START;
On 2016/11/19 00:36:42, Nicolas Zea wrote:
> I think it's cleaner to move this up to the #if defined(OS_WIN) section

Done.

Btw technically all of the above can always be initialized (up to setting the
right start_behavior). The whole reason I put this in a condition here is to
more easily catch errors where the network is used when in local mode. If you'd
rather wish that we don't do this here I can remove this condition altogether.

https://codereview.chromium.org/2494873003/diff/40001/components/browser_sync...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2494873003/diff/40001/components/browser_sync...
components/browser_sync/profile_sync_service.cc:252:
(local_sync_backend_enabled_ || IsSignedIn()));
On 2016/11/19 00:36:42, Nicolas Zea wrote:
> It seems like IsSyncRequested/IsSignedIn/IsSyncAllowed should all return true
if
> the sync backend is enabled. I worry overriding CanSyncStart but not those
will
> lead to bugs.

I don't think this is the case but I might be misunderstanding something in
which case please correct my reasoning below:

IsSyncRequested is checking whether the prefs::kSyncSuppressStart is not set. I
think this is independent of the sync backend used.

IsSignedIn is technically false in local sync therefore I only ever OR it with
the local sync state.

IsSyncAllowed checks if sync is not disabled e.g. by flag which I believe again
should be respected even in local sync mode. Right now it might seem
counter-intuitive that someone will add both the local sync enabled flag and
disable sync flag but remember that eventually the local sync mode will be
controlled by policy too.

Happy to change something if you believe the reasoning above is wrong or
misguided.

[Nicolas Zea, 2016-11-23 01:37:13]

That raises a good point actually. One thing we need to ensure is that other
services that are depending on Sync's state (e.g. checking if history sync is
enabled) before interacting with Google in some way don't do so here. Have you
verified that's not the case? For example the PrecacheManager and the
SuggestionsService both look at this.

Perhaps that's a good reason to not trick sync into thinking it's syncing
normally, when in fact this is a new state. Perhaps this shuold be treated as a
first class state in fact, which would imply we can expose it in
chrome://sync-internals somehow, and query whether we have a local backend or
not externally.

[pastarmovj, 2016-11-23 14:23:06]

On 2016/11/23 01:37:13, Nicolas Zea wrote:
> That raises a good point actually. One thing we need to ensure is that other
> services that are depending on Sync's state (e.g. checking if history sync is
> enabled) before interacting with Google in some way don't do so here. Have you
> verified that's not the case? For example the PrecacheManager and the
> SuggestionsService both look at this.
> 
> Perhaps that's a good reason to not trick sync into thinking it's syncing
> normally, when in fact this is a new state. Perhaps this shuold be treated as
a
> first class state in fact, which would imply we can expose it in
> chrome://sync-internals somehow, and query whether we have a local backend or
> not externally.

I would like to see that not so much as a new state as only a different backend
without client facing implications. :) But it was good thing you point out to
look into the ways other services interact with the Sync Service and if they
need to act differently in that case but I can not find a reason why would this
be needed/wanted.

On the other hand showing this in sync-internals is a clear goal for me. And I
just added https://bugs.chromium.org/p/chromium/issues/detail?id=668145 to track
this as part of the whole effort. Certainly we can make the SyncService
interface expose the local sync status programmatically too but I don't see any
immediate use for it? Do you think it is still useful to have it until we
identify clear need for that? I see it more as an implementation detail than a
client visible feature. You can see from the past CLs that actually adding it
there was very undisruptive change and this CL is more dealing with the tight
coupling of Sync and signin really.

[Nicolas Zea, 2016-11-23 22:49:11]

On 2016/11/23 14:23:06, pastarmovj wrote:
> On 2016/11/23 01:37:13, Nicolas Zea wrote:
> > That raises a good point actually. One thing we need to ensure is that other
> > services that are depending on Sync's state (e.g. checking if history sync
is
> > enabled) before interacting with Google in some way don't do so here. Have
you
> > verified that's not the case? For example the PrecacheManager and the
> > SuggestionsService both look at this.
> > 
> > Perhaps that's a good reason to not trick sync into thinking it's syncing
> > normally, when in fact this is a new state. Perhaps this shuold be treated
as
> a
> > first class state in fact, which would imply we can expose it in
> > chrome://sync-internals somehow, and query whether we have a local backend
or
> > not externally.
> 
> I would like to see that not so much as a new state as only a different
backend
> without client facing implications. :) But it was good thing you point out to
> look into the ways other services interact with the Sync Service and if they
> need to act differently in that case but I can not find a reason why would
this
> be needed/wanted.

I'm not sure I understand your comment here. Are you saying that the
PrecacheManager and SuggestionsService do not need changes? My point earlier was
those places (and possibly others) are using whether the user has enabled sync
(as determined by whether sync is running) as permission to do other actions
which might involve Google services. By forcing sync to run in this mode (which
IsSyncActive will reflect), those other places will now wrongly think that the
user enabled Sync.

chrome://history is another example that will now think that Sync is running
with history enabled, and start trying to fetch synced history from
history.google.com. It shouldn't do that for a local sync backend, as there's
nothing to fetch.

> 
> On the other hand showing this in sync-internals is a clear goal for me. And I
> just added https://bugs.chromium.org/p/chromium/issues/detail?id=668145 to
track
> this as part of the whole effort. Certainly we can make the SyncService
> interface expose the local sync status programmatically too but I don't see
any
> immediate use for it? Do you think it is still useful to have it until we
> identify clear need for that? I see it more as an implementation detail than a
> client visible feature. You can see from the past CLs that actually adding it
> there was very undisruptive change and this CL is more dealing with the tight
> coupling of Sync and signin really.

[pastarmovj, 2016-11-24 10:41:39]

On 2016/11/23 22:49:11, Nicolas Zea wrote:
> On 2016/11/23 14:23:06, pastarmovj wrote:
> > On 2016/11/23 01:37:13, Nicolas Zea wrote:
> > > That raises a good point actually. One thing we need to ensure is that
other
> > > services that are depending on Sync's state (e.g. checking if history sync
> is
> > > enabled) before interacting with Google in some way don't do so here. Have
> you
> > > verified that's not the case? For example the PrecacheManager and the
> > > SuggestionsService both look at this.
> > > 
> > > Perhaps that's a good reason to not trick sync into thinking it's syncing
> > > normally, when in fact this is a new state. Perhaps this shuold be treated
> as
> > a
> > > first class state in fact, which would imply we can expose it in
> > > chrome://sync-internals somehow, and query whether we have a local backend
> or
> > > not externally.
> > 
> > I would like to see that not so much as a new state as only a different
> backend
> > without client facing implications. :) But it was good thing you point out
to
> > look into the ways other services interact with the Sync Service and if they
> > need to act differently in that case but I can not find a reason why would
> this
> > be needed/wanted.
> 
> I'm not sure I understand your comment here. Are you saying that the
> PrecacheManager and SuggestionsService do not need changes? My point earlier
was
> those places (and possibly others) are using whether the user has enabled sync
> (as determined by whether sync is running) as permission to do other actions
> which might involve Google services. By forcing sync to run in this mode
(which
> IsSyncActive will reflect), those other places will now wrongly think that the
> user enabled Sync.
> 
> chrome://history is another example that will now think that Sync is running
> with history enabled, and start trying to fetch synced history from
> http://history.google.com. It shouldn't do that for a local sync backend, as
there's
> nothing to fetch.
> 
> > 
> > On the other hand showing this in sync-internals is a clear goal for me. And
I
> > just added https://bugs.chromium.org/p/chromium/issues/detail?id=668145 to
> track
> > this as part of the whole effort. Certainly we can make the SyncService
> > interface expose the local sync status programmatically too but I don't see
> any
> > immediate use for it? Do you think it is still useful to have it until we
> > identify clear need for that? I see it more as an implementation detail than
a
> > client visible feature. You can see from the past CLs that actually adding
it
> > there was very undisruptive change and this CL is more dealing with the
tight
> > coupling of Sync and signin really.

I wanted to say that as far as those other services are using Sync only for
synchronizing data between instances through the regular data models they don't
care whether Sync runs in local or full mode. On the other hand I now understand
what you were saying that some of those (like history) might try to reach other
backends on the premise that if Sync is running connectivity and sign-in are
granted. 

I tried opening the chrome://history page on a fresh profile (seeded through
local Sync) and it worked just fine showing me the history stored in the local
sync data and augmenting it with any new pages I opened. It correctly detects
that the user is not signed in and shows a background interstitial that if
history should be synced between devices the user should sign in, which seems to
be WAI.

I admit that this discussion is not exhaustive and some issues might appear but
I guess we will not be able to cover them all and will have to act upon then as
they are discovered the major goal here still being that proper Sync should not
be broken whatever.

[Nicolas Zea, 2016-11-29 18:21:12]

On 2016/11/24 10:41:39, pastarmovj wrote:
> On 2016/11/23 22:49:11, Nicolas Zea wrote:
> > On 2016/11/23 14:23:06, pastarmovj wrote:
> > > On 2016/11/23 01:37:13, Nicolas Zea wrote:
> > > > That raises a good point actually. One thing we need to ensure is that
> other
> > > > services that are depending on Sync's state (e.g. checking if history
sync
> > is
> > > > enabled) before interacting with Google in some way don't do so here.
Have
> > you
> > > > verified that's not the case? For example the PrecacheManager and the
> > > > SuggestionsService both look at this.
> > > > 
> > > > Perhaps that's a good reason to not trick sync into thinking it's
syncing
> > > > normally, when in fact this is a new state. Perhaps this shuold be
treated
> > as
> > > a
> > > > first class state in fact, which would imply we can expose it in
> > > > chrome://sync-internals somehow, and query whether we have a local
backend
> > or
> > > > not externally.
> > > 
> > > I would like to see that not so much as a new state as only a different
> > backend
> > > without client facing implications. :) But it was good thing you point out
> to
> > > look into the ways other services interact with the Sync Service and if
they
> > > need to act differently in that case but I can not find a reason why would
> > this
> > > be needed/wanted.
> > 
> > I'm not sure I understand your comment here. Are you saying that the
> > PrecacheManager and SuggestionsService do not need changes? My point earlier
> was
> > those places (and possibly others) are using whether the user has enabled
sync
> > (as determined by whether sync is running) as permission to do other actions
> > which might involve Google services. By forcing sync to run in this mode
> (which
> > IsSyncActive will reflect), those other places will now wrongly think that
the
> > user enabled Sync.
> > 
> > chrome://history is another example that will now think that Sync is running
> > with history enabled, and start trying to fetch synced history from
> > http://history.google.com. It shouldn't do that for a local sync backend, as
> there's
> > nothing to fetch.
> > 
> > > 
> > > On the other hand showing this in sync-internals is a clear goal for me.
And
> I
> > > just added https://bugs.chromium.org/p/chromium/issues/detail?id=668145 to
> > track
> > > this as part of the whole effort. Certainly we can make the SyncService
> > > interface expose the local sync status programmatically too but I don't
see
> > any
> > > immediate use for it? Do you think it is still useful to have it until we
> > > identify clear need for that? I see it more as an implementation detail
than
> a
> > > client visible feature. You can see from the past CLs that actually adding
> it
> > > there was very undisruptive change and this CL is more dealing with the
> tight
> > > coupling of Sync and signin really.
> 
> I wanted to say that as far as those other services are using Sync only for
> synchronizing data between instances through the regular data models they
don't
> care whether Sync runs in local or full mode. On the other hand I now
understand
> what you were saying that some of those (like history) might try to reach
other
> backends on the premise that if Sync is running connectivity and sign-in are
> granted. 
> 
> I tried opening the chrome://history page on a fresh profile (seeded through
> local Sync) and it worked just fine showing me the history stored in the local
> sync data and augmenting it with any new pages I opened. It correctly detects
> that the user is not signed in and shows a background interstitial that if
> history should be synced between devices the user should sign in, which seems
to
> be WAI.
> 
> I admit that this discussion is not exhaustive and some issues might appear
but
> I guess we will not be able to cover them all and will have to act upon then
as
> they are discovered the major goal here still being that proper Sync should
not
> be broken whatever.

Apologies for the slow response.

The services I mention (precache manager, suggestions manager) are not
performing synchronization. They're sending data/fetching data from Google for
orthogonal reasons, but should only be enabled if real sync is enabled (and
talking to Google). It would be a privacy violation if they continued to do so
for a local backend (otherwise why would the device bother to have a local
backend vs the real google-backed one). I think we need to ensure we audit all
of these scenarios before enabling this.

[pastarmovj, 2016-12-01 08:00:19]

On 2016/11/29 18:21:12, Nicolas Zea wrote:
> On 2016/11/24 10:41:39, pastarmovj wrote:
> > On 2016/11/23 22:49:11, Nicolas Zea wrote:
> > > On 2016/11/23 14:23:06, pastarmovj wrote:
> > > > On 2016/11/23 01:37:13, Nicolas Zea wrote:
> > > > > That raises a good point actually. One thing we need to ensure is that
> > other
> > > > > services that are depending on Sync's state (e.g. checking if history
> sync
> > > is
> > > > > enabled) before interacting with Google in some way don't do so here.
> Have
> > > you
> > > > > verified that's not the case? For example the PrecacheManager and the
> > > > > SuggestionsService both look at this.
> > > > > 
> > > > > Perhaps that's a good reason to not trick sync into thinking it's
> syncing
> > > > > normally, when in fact this is a new state. Perhaps this shuold be
> treated
> > > as
> > > > a
> > > > > first class state in fact, which would imply we can expose it in
> > > > > chrome://sync-internals somehow, and query whether we have a local
> backend
> > > or
> > > > > not externally.
> > > > 
> > > > I would like to see that not so much as a new state as only a different
> > > backend
> > > > without client facing implications. :) But it was good thing you point
out
> > to
> > > > look into the ways other services interact with the Sync Service and if
> they
> > > > need to act differently in that case but I can not find a reason why
would
> > > this
> > > > be needed/wanted.
> > > 
> > > I'm not sure I understand your comment here. Are you saying that the
> > > PrecacheManager and SuggestionsService do not need changes? My point
earlier
> > was
> > > those places (and possibly others) are using whether the user has enabled
> sync
> > > (as determined by whether sync is running) as permission to do other
actions
> > > which might involve Google services. By forcing sync to run in this mode
> > (which
> > > IsSyncActive will reflect), those other places will now wrongly think that
> the
> > > user enabled Sync.
> > > 
> > > chrome://history is another example that will now think that Sync is
running
> > > with history enabled, and start trying to fetch synced history from
> > > http://history.google.com. It shouldn't do that for a local sync backend,
as
> > there's
> > > nothing to fetch.
> > > 
> > > > 
> > > > On the other hand showing this in sync-internals is a clear goal for me.
> And
> > I
> > > > just added https://bugs.chromium.org/p/chromium/issues/detail?id=668145
to
> > > track
> > > > this as part of the whole effort. Certainly we can make the SyncService
> > > > interface expose the local sync status programmatically too but I don't
> see
> > > any
> > > > immediate use for it? Do you think it is still useful to have it until
we
> > > > identify clear need for that? I see it more as an implementation detail
> than
> > a
> > > > client visible feature. You can see from the past CLs that actually
adding
> > it
> > > > there was very undisruptive change and this CL is more dealing with the
> > tight
> > > > coupling of Sync and signin really.
> > 
> > I wanted to say that as far as those other services are using Sync only for
> > synchronizing data between instances through the regular data models they
> don't
> > care whether Sync runs in local or full mode. On the other hand I now
> understand
> > what you were saying that some of those (like history) might try to reach
> other
> > backends on the premise that if Sync is running connectivity and sign-in are
> > granted. 
> > 
> > I tried opening the chrome://history page on a fresh profile (seeded through
> > local Sync) and it worked just fine showing me the history stored in the
local
> > sync data and augmenting it with any new pages I opened. It correctly
detects
> > that the user is not signed in and shows a background interstitial that if
> > history should be synced between devices the user should sign in, which
seems
> to
> > be WAI.
> > 
> > I admit that this discussion is not exhaustive and some issues might appear
> but
> > I guess we will not be able to cover them all and will have to act upon then
> as
> > they are discovered the major goal here still being that proper Sync should
> not
> > be broken whatever.
> 
> Apologies for the slow response.
> 
> The services I mention (precache manager, suggestions manager) are not
> performing synchronization. They're sending data/fetching data from Google for
> orthogonal reasons, but should only be enabled if real sync is enabled (and
> talking to Google). It would be a privacy violation if they continued to do so
> for a local backend (otherwise why would the device bother to have a local
> backend vs the real google-backed one). I think we need to ensure we audit all
> of these scenarios before enabling this.

Thanks, now I got you. I was under the impression that all of those "proxied"
through sync but now I understand (and saw in the code that) that they only
really use it as a signal for consent. 

While thinking about the possible solution two things come to mind which I would
like to bounce off of you as ideas and hear your opinion on them:

- Short term I can expose the local sync flag in two ways - the
IsBackendInitialized can be returned to GetBackendInitializationStatus return an
enum instead of a bool { UNINITIALZED, INITIALIZED, LOCALLY_INITIALIZED } and
through a IsLocalBackendEnabled method on the SyncService. Then these services
will check for example instead of if(IsBackendInitialized)
if(GetBackendInitializationStatus == INITIALIZED) for those that care and
if(GetBackendInitializationStatus != UNINITIALIZED) for those that don't.

- Long term I think instead of checking for the sync status explicitly those
services should be checking if the user has signed in into the browser. The
consent is really expressed by the signing in effort and not by the sync state
imo.

What do you think. If you agree to that plan I can already in this CL try to
catch and cover as many of the services (hopefully they are not all that many)
with extra checks for full sync. And open up bugs for each and every one of them
to convert the checks to sign-in checks rather than sync checks in the future?

[Nicolas Zea, 2016-12-01 19:08:27]

On 2016/12/01 08:00:19, pastarmovj wrote:
> On 2016/11/29 18:21:12, Nicolas Zea wrote:
> > On 2016/11/24 10:41:39, pastarmovj wrote:
> > > On 2016/11/23 22:49:11, Nicolas Zea wrote:
> > > > On 2016/11/23 14:23:06, pastarmovj wrote:
> > > > > On 2016/11/23 01:37:13, Nicolas Zea wrote:
> > > > > > That raises a good point actually. One thing we need to ensure is
that
> > > other
> > > > > > services that are depending on Sync's state (e.g. checking if
history
> > sync
> > > > is
> > > > > > enabled) before interacting with Google in some way don't do so
here.
> > Have
> > > > you
> > > > > > verified that's not the case? For example the PrecacheManager and
the
> > > > > > SuggestionsService both look at this.
> > > > > > 
> > > > > > Perhaps that's a good reason to not trick sync into thinking it's
> > syncing
> > > > > > normally, when in fact this is a new state. Perhaps this shuold be
> > treated
> > > > as
> > > > > a
> > > > > > first class state in fact, which would imply we can expose it in
> > > > > > chrome://sync-internals somehow, and query whether we have a local
> > backend
> > > > or
> > > > > > not externally.
> > > > > 
> > > > > I would like to see that not so much as a new state as only a
different
> > > > backend
> > > > > without client facing implications. :) But it was good thing you point
> out
> > > to
> > > > > look into the ways other services interact with the Sync Service and
if
> > they
> > > > > need to act differently in that case but I can not find a reason why
> would
> > > > this
> > > > > be needed/wanted.
> > > > 
> > > > I'm not sure I understand your comment here. Are you saying that the
> > > > PrecacheManager and SuggestionsService do not need changes? My point
> earlier
> > > was
> > > > those places (and possibly others) are using whether the user has
enabled
> > sync
> > > > (as determined by whether sync is running) as permission to do other
> actions
> > > > which might involve Google services. By forcing sync to run in this mode
> > > (which
> > > > IsSyncActive will reflect), those other places will now wrongly think
that
> > the
> > > > user enabled Sync.
> > > > 
> > > > chrome://history is another example that will now think that Sync is
> running
> > > > with history enabled, and start trying to fetch synced history from
> > > > http://history.google.com. It shouldn't do that for a local sync
backend,
> as
> > > there's
> > > > nothing to fetch.
> > > > 
> > > > > 
> > > > > On the other hand showing this in sync-internals is a clear goal for
me.
> > And
> > > I
> > > > > just added
https://bugs.chromium.org/p/chromium/issues/detail?id=668145
> to
> > > > track
> > > > > this as part of the whole effort. Certainly we can make the
SyncService
> > > > > interface expose the local sync status programmatically too but I
don't
> > see
> > > > any
> > > > > immediate use for it? Do you think it is still useful to have it until
> we
> > > > > identify clear need for that? I see it more as an implementation
detail
> > than
> > > a
> > > > > client visible feature. You can see from the past CLs that actually
> adding
> > > it
> > > > > there was very undisruptive change and this CL is more dealing with
the
> > > tight
> > > > > coupling of Sync and signin really.
> > > 
> > > I wanted to say that as far as those other services are using Sync only
for
> > > synchronizing data between instances through the regular data models they
> > don't
> > > care whether Sync runs in local or full mode. On the other hand I now
> > understand
> > > what you were saying that some of those (like history) might try to reach
> > other
> > > backends on the premise that if Sync is running connectivity and sign-in
are
> > > granted. 
> > > 
> > > I tried opening the chrome://history page on a fresh profile (seeded
through
> > > local Sync) and it worked just fine showing me the history stored in the
> local
> > > sync data and augmenting it with any new pages I opened. It correctly
> detects
> > > that the user is not signed in and shows a background interstitial that if
> > > history should be synced between devices the user should sign in, which
> seems
> > to
> > > be WAI.
> > > 
> > > I admit that this discussion is not exhaustive and some issues might
appear
> > but
> > > I guess we will not be able to cover them all and will have to act upon
then
> > as
> > > they are discovered the major goal here still being that proper Sync
should
> > not
> > > be broken whatever.
> > 
> > Apologies for the slow response.
> > 
> > The services I mention (precache manager, suggestions manager) are not
> > performing synchronization. They're sending data/fetching data from Google
for
> > orthogonal reasons, but should only be enabled if real sync is enabled (and
> > talking to Google). It would be a privacy violation if they continued to do
so
> > for a local backend (otherwise why would the device bother to have a local
> > backend vs the real google-backed one). I think we need to ensure we audit
all
> > of these scenarios before enabling this.
> 
> Thanks, now I got you. I was under the impression that all of those "proxied"
> through sync but now I understand (and saw in the code that) that they only
> really use it as a signal for consent. 
> 
> While thinking about the possible solution two things come to mind which I
would
> like to bounce off of you as ideas and hear your opinion on them:
> 
> - Short term I can expose the local sync flag in two ways - the
> IsBackendInitialized can be returned to GetBackendInitializationStatus return
an
> enum instead of a bool { UNINITIALZED, INITIALIZED, LOCALLY_INITIALIZED } and
> through a IsLocalBackendEnabled method on the SyncService. Then these services
> will check for example instead of if(IsBackendInitialized)
> if(GetBackendInitializationStatus == INITIALIZED) for those that care and
> if(GetBackendInitializationStatus != UNINITIALIZED) for those that don't.
> 
> - Long term I think instead of checking for the sync status explicitly those
> services should be checking if the user has signed in into the browser. The
> consent is really expressed by the signing in effort and not by the sync state
> imo.
> 
> What do you think. If you agree to that plan I can already in this CL try to
> catch and cover as many of the services (hopefully they are not all that many)
> with extra checks for full sync. And open up bugs for each and every one of
them
> to convert the checks to sign-in checks rather than sync checks in the future?

Unfortunately the signin isn't really the consent (you can have sync disabled
while signed in for example). Sync, and in particular sync's datatype/encryption
state are the consent.

That said, I think your short term solution makes sense. Will that mean that
backend_initialized_ will switch from a bool to an enum? Most things outside of
Sync shouldn't consider a local backend valid, while internal to sync it should.
This might involve doing an audit of everything that checks IsSyncActive().

[pastarmovj, 2016-12-09 12:57:19]

Patchset #6 (id:100001) has been deleted

[pastarmovj, 2016-12-09 12:59:34]

I finally managed to scrape time to address our discussion. Sorry for the delay
there and thanks for the insightful comments!

From digging through the code I identified 4 places that needed to know about
local sync:

WebHistoryServiceFactory
SuggestionsService
PrecacheManager
SyncPromoUI.

[Nicolas Zea, 2016-12-12 19:16:38]

Thanks for making those changes! This mostly looks right, but I still am
concerned that it's too easy for something external to Sync to forget to check
the IsLocalSyncEnabled method. 

What do you think about having IsSyncActive return false when when local sync is
enabled? Active will basically be defined as sync is running and talking to a
real server.

That way a developer can't accidentally do some action thinking Sync is talking
to our real server. We should keep IsLocalSyncEnabled around though, in case
there are actions that can be taken if the local sync service is on.

[pastarmovj, 2016-12-13 10:46:06]

On 2016/12/12 19:16:38, Nicolas Zea wrote:
> Thanks for making those changes! This mostly looks right, but I still am
> concerned that it's too easy for something external to Sync to forget to check
> the IsLocalSyncEnabled method. 
> 
> What do you think about having IsSyncActive return false when when local sync
is
> enabled? Active will basically be defined as sync is running and talking to a
> real server.
> 
> That way a developer can't accidentally do some action thinking Sync is
talking
> to our real server. We should keep IsLocalSyncEnabled around though, in case
> there are actions that can be taken if the local sync service is on.

I see your point. I went on and listed all places that currently branch on
IsSyncActive and also tried to figure out for each one of them if it should see
local sync as active too or not. For most of them it will actually force us to
make the check be IsSyncActive || IsLocalSyncEnabled and even the few that will
actually not need this they are either already covered with the changes in this
CL or are so few that rather we could say IsSyncActive && !IsLocalSyncEnabled
for those few. If you feel strongly about that I can implement the change (or
maybe we can think of an even better way to achieve that) but I see more
overhead than benefit at this point. 

Here is the list I compiled while I researched that: 

https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?q=Is...
- No need to change sync promo is not needed.
https://cs.chromium.org/chromium/src/components/sync/driver/about_sync_util.c...
+ Needs to change or else the info will not be populated
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/app_sync_ui_state....
+ Needs to change to properly update the ui.
https://cs.chromium.org/chromium/src/chrome/browser/ui/sync/one_click_signin_...
- No need to promo sign-in sync.
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_um...
- Don't have to report permission UMAs without proper Sync.
https://cs.chromium.org/chromium/src/components/browsing_data/core/history_no...
- Nothing is stored in web history without full Sync.
https://cs.chromium.org/chromium/src/chrome/browser/android/foreign_session_h...
+ Always retirn the delegate. (android only but better fix now).
https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_servi...
- Web History should see sync as off.
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/recent_tabs_su...
+ Always retirn the delegate. 
https://cs.chromium.org/chromium/src/chrome/browser/ui/passwords/manage_passw...
+ Passwords and prefs are stored either way.
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/upd...
+ Updates have to happen either way.
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/clear_br...
+ Warn about other ways of storing the data.
https://cs.chromium.org/chromium/src/components/history/core/browser/history_...
+ Local sync needs same treatment.
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
+ Local sync needs same treatment.
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
+ Local sync needs same treatment.
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...
+ Local sync needs same treatment.
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...
+ Local sync needs same treatment.
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/media_router/med...
- Sync is used as a consent check.
(legend: + after the filename means this has to treat local sync mode as remote
sync and - that if IsSyncActuve returned false the logic will be ok and a short
justification about the choice)

PS. While writing all that another idea came to my mind and this is already
achievable today (with a separate policy) but we can enforce it for local sync
(assuming this is anyway what admins want in this mode) - local sync will force
sign in to be disabled. In other words
https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...
will return false when local sync is on. This will efectively make any Google
web service unavailable. WDYT?

[Nicolas Zea, 2016-12-15 00:37:14]

On 2016/12/13 10:46:06, pastarmovj wrote:
> On 2016/12/12 19:16:38, Nicolas Zea wrote:
> > Thanks for making those changes! This mostly looks right, but I still am
> > concerned that it's too easy for something external to Sync to forget to
check
> > the IsLocalSyncEnabled method. 
> > 
> > What do you think about having IsSyncActive return false when when local
sync
> is
> > enabled? Active will basically be defined as sync is running and talking to
a
> > real server.
> > 
> > That way a developer can't accidentally do some action thinking Sync is
> talking
> > to our real server. We should keep IsLocalSyncEnabled around though, in case
> > there are actions that can be taken if the local sync service is on.
> 
> I see your point. I went on and listed all places that currently branch on
> IsSyncActive and also tried to figure out for each one of them if it should
see
> local sync as active too or not. For most of them it will actually force us to
> make the check be IsSyncActive || IsLocalSyncEnabled and even the few that
will
> actually not need this they are either already covered with the changes in
this
> CL or are so few that rather we could say IsSyncActive && !IsLocalSyncEnabled
> for those few. If you feel strongly about that I can implement the change (or
> maybe we can think of an even better way to achieve that) but I see more
> overhead than benefit at this point. 
> 
> Here is the list I compiled while I researched that: 
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/sync_ui_util.cc?q=Is...
> - No need to change sync promo is not needed.
>
https://cs.chromium.org/chromium/src/components/sync/driver/about_sync_util.c...
> + Needs to change or else the info will not be populated
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/ash/app_sync_ui_state....
> + Needs to change to properly update the ui.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/sync/one_click_signin_...
> - No need to promo sign-in sync.
>
https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_um...
> - Don't have to report permission UMAs without proper Sync.
>
https://cs.chromium.org/chromium/src/components/browsing_data/core/history_no...
> - Nothing is stored in web history without full Sync.
>
https://cs.chromium.org/chromium/src/chrome/browser/android/foreign_session_h...
> + Always retirn the delegate. (android only but better fix now).
>
https://cs.chromium.org/chromium/src/chrome/browser/history/web_history_servi...
> - Web History should see sync as off.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/toolbar/recent_tabs_su...
> + Always retirn the delegate. 
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/passwords/manage_passw...
> + Passwords and prefs are stored either way.
>
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/upd...
> + Updates have to happen either way.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/options/clear_br...
> + Warn about other ways of storing the data.
>
https://cs.chromium.org/chromium/src/components/history/core/browser/history_...
> + Local sync needs same treatment.
>
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
> + Local sync needs same treatment.
>
https://cs.chromium.org/chromium/src/components/password_manager/core/browser...
> + Local sync needs same treatment.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/settings/setting...
> + Local sync needs same treatment.
>
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/chrome_p...
> + Local sync needs same treatment.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/media_router/med...
> - Sync is used as a consent check.
> (legend: + after the filename means this has to treat local sync mode as
remote
> sync and - that if IsSyncActuve returned false the logic will be ok and a
short
> justification about the choice)
> 
> PS. While writing all that another idea came to my mind and this is already
> achievable today (with a separate policy) but we can enforce it for local sync
> (assuming this is anyway what admins want in this mode) - local sync will
force
> sign in to be disabled. In other words
>
https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_ma...
> will return false when local sync is on. This will efectively make any Google
> web service unavailable. WDYT?

I think that's probably a good thing to do either way, but I don't think it
would help that much with most of these cases.

It's a fair point that most of the callsites would now need this new check.
Also, the more I think about it at this point most of these services would
likely be disabled by the admin (and we shouldn't be adding any new services
moving forward), this seems fine.

Mainly have some nits.

[Nicolas Zea, 2016-12-15 00:37:29]

(forgot to send the comments in the last message)

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile.cc:224: bool Profile::IsLocalSyncEnabled() {
Do we really need this exposed here? Can we just have the callsites depend on
HasProfileSyncService directly?

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_factory.cc:172: // one should be
considered roamed.
nit: should this refer to an open bug?

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/sync_promo_ui.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/ui/sync...
chrome/browser/ui/sync/sync_promo_ui.cc:17: if (!profile->IsSyncAllowed() ||
profile->IsLocalSyncEnabled())
See other comment. Is this the only callsite that uses
Profile::IsLocalSyncEnabled? Seems unnecessary to expose that at the profile
level. I wonder if this place should just be checking the pref directly then via
SyncPrefs (same goes for IsSyncAllowed)

https://codereview.chromium.org/2494873003/diff/120001/components/browser_syn...
File components/browser_sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/browser_syn...
components/browser_sync/profile_sync_service_unittest.cc:252: void
CreateServiceWithoutSignIn() {
nit: for clarity would be good to rename to CreateSErviceWithLocalSyncBackend

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
defined(OS_WIN)
May as well remove the OS requirement here right? The test should pass no matter
the platform.

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl.h (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.h:45: bool
local_sync_backend_enabled);
nit: how about renaming this to ignore_auth_credentials or something like that,
to make it more clear what behavior this is controlling. It seems like a
layering violation to be exposing the backend state here.

This also applies to SyncManager and the other layers below PSS that don't
really care what kind of backend there is.

[pastarmovj, 2016-12-16 17:04:48]

PTAL.

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/profile...
File chrome/browser/profiles/profile.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/profile...
chrome/browser/profiles/profile.cc:224: bool Profile::IsLocalSyncEnabled() {
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> Do we really need this exposed here? Can we just have the callsites depend on
> HasProfileSyncService directly?

As suggested reverted this change.

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_factory.cc:172: // one should be
considered roamed.
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> nit: should this refer to an open bug?

Done.

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/ui/sync...
File chrome/browser/ui/sync/sync_promo_ui.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/chrome/browser/ui/sync...
chrome/browser/ui/sync/sync_promo_ui.cc:17: if (!profile->IsSyncAllowed() ||
profile->IsLocalSyncEnabled())
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> See other comment. Is this the only callsite that uses
> Profile::IsLocalSyncEnabled? Seems unnecessary to expose that at the profile
> level. I wonder if this place should just be checking the pref directly then
via
> SyncPrefs (same goes for IsSyncAllowed)

Yes this is the only site that needs IsLocalSyncEnabled but there are many that
use IsSyncAllowed. I removed my function in favor of direct access to the pref.

https://codereview.chromium.org/2494873003/diff/120001/components/browser_syn...
File components/browser_sync/profile_sync_service_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/browser_syn...
components/browser_sync/profile_sync_service_unittest.cc:252: void
CreateServiceWithoutSignIn() {
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> nit: for clarity would be good to rename to CreateSErviceWithLocalSyncBackend

Done.

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
defined(OS_WIN)
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> May as well remove the OS requirement here right? The test should pass no
matter
> the platform.

Actually I added it after I tried compiling on the linux bots.

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_scheduler_impl.h (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_scheduler_impl.h:45: bool
local_sync_backend_enabled);
On 2016/12/15 00:37:28, Nicolas Zea wrote:
> nit: how about renaming this to ignore_auth_credentials or something like
that,
> to make it more clear what behavior this is controlling. It seems like a
> layering violation to be exposing the backend state here.
> 
> This also applies to SyncManager and the other layers below PSS that don't
> really care what kind of backend there is.

Done.

[Nicolas Zea, 2016-12-16 18:50:35]

Mostly LGTM with the one concern

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
defined(OS_WIN)
On 2016/12/16 17:04:48, pastarmovj wrote:
> On 2016/12/15 00:37:28, Nicolas Zea wrote:
> > May as well remove the OS requirement here right? The test should pass no
> matter
> > the platform.
> 
> Actually I added it after I tried compiling on the linux bots.

That implies to me a layering violation. Everything in the engine should ideally
be abstracted from platform specific conditions. What was the error?

[pastarmovj, 2016-12-19 11:11:59]

pastarmovj@chromium.org changed reviewers:
+ jochen@chromium.org

[pastarmovj, 2016-12-19 11:12:00]

Hi Jochen,
can you please review the changes in:

chrome/browser/history/web_history_service_factory.cc
chrome/browser/prefs/chrome_command_line_pref_store.cc
components/browsing_data/core/history_notice_utils_unittest.cc
components/precache/content/precache_manager.cc
components/suggestions/suggestions_service.cc

Thank you,
Julian

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):

https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
defined(OS_WIN)
On 2016/12/16 18:50:35, Nicolas Zea wrote:
> On 2016/12/16 17:04:48, pastarmovj wrote:
> > On 2016/12/15 00:37:28, Nicolas Zea wrote:
> > > May as well remove the OS requirement here right? The test should pass no
> > matter
> > > the platform.
> > 
> > Actually I added it after I tried compiling on the linux bots.
> 
> That implies to me a layering violation. Everything in the engine should
ideally
> be abstracted from platform specific conditions. What was the error?

it is because of this ifdef here:
https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_manager...
the reason for it is that all the local backend code is conditionally compiled
only in the Windows version for now to avoid bloating the executable on other
platforms as discussed here https://codereview.chromium.org/2360703002/#msg14 . 

I think it will be hard to get this done in any other way but to always compile
the code in on all platforms that run this test.

[jochen (gone - plz use gerrit), 2016-12-19 11:59:22]

how will you make sure that all future uses of this system also check for local
sync?

[pastarmovj, 2016-12-19 12:46:03]

On 2016/12/19 11:59:22, jochen wrote:
> how will you make sure that all future uses of this system also check for
local
> sync?

excellent question :) 

this is mostly what the discussion going above is all about. In a nutshell these
checks are mostly just belt-and-suspenders approach and for good UI in some
cases. The plan is to tie the sign-in disabled policy to the local sync policy
and therefore prevent signin which in turn disables any service talking to our
cloud services. Until then if you are signed in and a new service is added that
use the sync consent as a consent for itself it will work however this will only
be possible for users running with local sync turned on by the command line flag
which seems fine to me for enterprise users the policies will prevent this in
all cases.

[jochen (gone - plz use gerrit), 2016-12-19 12:52:03]

lgtm

[Nicolas Zea, 2016-12-19 17:29:02]

On 2016/12/19 11:12:00, pastarmovj wrote:
> Hi Jochen,
> can you please review the changes in:
> 
> chrome/browser/history/web_history_service_factory.cc
> chrome/browser/prefs/chrome_command_line_pref_store.cc
> components/browsing_data/core/history_notice_utils_unittest.cc
> components/precache/content/precache_manager.cc
> components/suggestions/suggestions_service.cc
> 
> Thank you,
> Julian
> 
>
https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
> File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
> components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
> defined(OS_WIN)
> On 2016/12/16 18:50:35, Nicolas Zea wrote:
> > On 2016/12/16 17:04:48, pastarmovj wrote:
> > > On 2016/12/15 00:37:28, Nicolas Zea wrote:
> > > > May as well remove the OS requirement here right? The test should pass
no
> > > matter
> > > > the platform.
> > > 
> > > Actually I added it after I tried compiling on the linux bots.
> > 
> > That implies to me a layering violation. Everything in the engine should
> ideally
> > be abstracted from platform specific conditions. What was the error?
> 
> it is because of this ifdef here:
>
https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_manager...
> the reason for it is that all the local backend code is conditionally compiled
> only in the Windows version for now to avoid bloating the executable on other
> platforms as discussed here https://codereview.chromium.org/2360703002/#msg14
. 
> 
> I think it will be hard to get this done in any other way but to always
compile
> the code in on all platforms that run this test.

I think at this layer, it would be cleaner to just compile it all (and remove
the ifdef). I'd rather keep the abstractions clean than introduce the platform
specific complexity at this layer (local_backend_enabled is already set
conditionally at a higher layer, and should be enough).

[pastarmovj, 2016-12-20 13:16:26]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 13:16:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pastarmovj, 2016-12-20 13:17:00]

On 2016/12/19 17:29:02, Nicolas Zea wrote:
> On 2016/12/19 11:12:00, pastarmovj wrote:
> > Hi Jochen,
> > can you please review the changes in:
> > 
> > chrome/browser/history/web_history_service_factory.cc
> > chrome/browser/prefs/chrome_command_line_pref_store.cc
> > components/browsing_data/core/history_notice_utils_unittest.cc
> > components/precache/content/precache_manager.cc
> > components/suggestions/suggestions_service.cc
> > 
> > Thank you,
> > Julian
> > 
> >
>
https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
> > File components/sync/engine_impl/sync_manager_impl_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2494873003/diff/120001/components/sync/engine...
> > components/sync/engine_impl/sync_manager_impl_unittest.cc:2700: #if
> > defined(OS_WIN)
> > On 2016/12/16 18:50:35, Nicolas Zea wrote:
> > > On 2016/12/16 17:04:48, pastarmovj wrote:
> > > > On 2016/12/15 00:37:28, Nicolas Zea wrote:
> > > > > May as well remove the OS requirement here right? The test should pass
> no
> > > > matter
> > > > > the platform.
> > > > 
> > > > Actually I added it after I tried compiling on the linux bots.
> > > 
> > > That implies to me a layering violation. Everything in the engine should
> > ideally
> > > be abstracted from platform specific conditions. What was the error?
> > 
> > it is because of this ifdef here:
> >
>
https://cs.chromium.org/chromium/src/components/sync/engine_impl/sync_manager...
> > the reason for it is that all the local backend code is conditionally
compiled
> > only in the Windows version for now to avoid bloating the executable on
other
> > platforms as discussed here
https://codereview.chromium.org/2360703002/#msg14
> . 
> > 
> > I think it will be hard to get this done in any other way but to always
> compile
> > the code in on all platforms that run this test.
> 
> I think at this layer, it would be cleaner to just compile it all (and remove
> the ifdef). I'd rather keep the abstractions clean than introduce the platform
> specific complexity at this layer (local_backend_enabled is already set
> conditionally at a higher layer, and should be enough).

Agree. I changed the code as we discussed.

[commit-bot: I haz the power, 2016-12-20 13:25:08]

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

[commit-bot: I haz the power, 2016-12-20 13:25:09]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Nicolas Zea, 2016-12-20 18:37:20]

Thanks Julian! LGTM

[pastarmovj, 2016-12-20 18:41:19]

On 2016/12/20 18:37:20, Nicolas Zea wrote:
> Thanks Julian! LGTM

I thank you for the guidance! 

Now I just have to get it to build again on all platforms (I guess there are a
few more ifdefs scattered around the codebase) and we are good to go.

[pastarmovj, 2016-12-21 11:36:14]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-12-21 11:36:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/2494873003/#ps200001
(title: "Remove ifdefs around include.")

[commit-bot: I haz the power, 2016-12-21 11:36:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 12:45:25]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1482320174227400,
"parent_rev": "749e30ebaae4919871afbc02eb46d2754ad01c19", "commit_rev":
"3019dd9993b40acf6e50566a2436321d3a6fdd93"}

[commit-bot: I haz the power, 2016-12-21 12:45:53]

Description was changed from

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests
==========

to

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2494873003
==========

[commit-bot: I haz the power, 2016-12-21 12:45:54]

Committed patchset #10 (id:200001)

[commit-bot: I haz the power, 2016-12-21 12:48:22]

Description was changed from

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests

Review-Url: https://codereview.chromium.org/2494873003
==========

to

==========
[Sync] Allow sync start without sign-in if the local sync backend is on.

Makes all signed-in checks conditional on the local_sync_backend_enabled
flag. Great care is taken for this change to have no side-effects on sync
running in non-local mode.

There are two major changes in this CL:
  1. Move the logic checking the local sync flags from ProfileSyncService
     to ProfileSyncServiceFactory::BuildServiceInstanceFor.
  2. Add checks and conditionally do checks if sign-in has happened only
     if the local sync backend flag is not set. Back up with tests that
     this works as expected.

BUG=651409
TEST=components_unittests

Committed: https://crrev.com/645df3089e27103dbe55998c0c3692236d281e53
Cr-Commit-Position: refs/heads/master@{#440075}
==========

[commit-bot: I haz the power, 2016-12-21 12:48:23]

Patchset 10 (id:??) landed as
https://crrev.com/645df3089e27103dbe55998c0c3692236d281e53
Cr-Commit-Position: refs/heads/master@{#440075}