Add synchronous and asynchronous methods to ManagedUserSyncService to get the list of managed users.

Add synchronous and asynchronous methods to ManagedUserSyncService to get the list of managed users.

BUG=none

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

[Bernhard Bauer, 2013-09-05 08:34:21]

Please review.

[Denis Kuznetsov (DE-MUC), 2013-09-05 08:53:49]

On 2013/09/05 08:34:21, Bernhard Bauer wrote:
> Please review.

It seems reasonable.
As callbacks are run at most once it will not cover case when manager is on
"import user" screen on one device and creates SU on other, but I'm not sure we
want to handle such case anyway.

lgtm

[Pam (message me for reviews), 2013-09-05 11:11:56]

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_sync_service.h:50: // managed users (as
the stored data might be outdated before that).
I don't understand this comment. What does it mean to not be "syncing managed
users"?

Also, is it guaranteed that we've gotten one sync update when this gets called,
or might the data be outdated anyway?

[Bernhard Bauer, 2013-09-05 12:15:41]

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_sync_service.h:50: // managed users (as
the stored data might be outdated before that).
On 2013/09/05 11:11:56, Pam (also PM for reviews) wrote:
> I don't understand this comment. What does it mean to not be "syncing managed
> users"?

A SyncableService needs to do an initial merge of its local data with the data
on the server. After that the service is considered to be syncing its data, and
it will get updates incrementally and be able to send updates to Sync. Before
that happens, the service is not syncing its data. 

> Also, is it guaranteed that we've gotten one sync update when this gets
called,
> or might the data be outdated anyway?

The former. MergeDataAndStartSyncing() gets called with a list of all the data
currently on the server (so Sync does an initial GetUpdates() call, which is why
a SyncableService doesn't start out immediately in the syncing state).

[Pam (message me for reviews), 2013-09-05 12:29:29]

LGTM with a comment update described below.

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_sync_service.h:50: // managed users (as
the stored data might be outdated before that).
On 2013/09/05 12:15:41, Bernhard Bauer wrote:
> On 2013/09/05 11:11:56, Pam (also PM for reviews) wrote:
> > I don't understand this comment. What does it mean to not be "syncing
managed
> > users"?
> 
> A SyncableService needs to do an initial merge of its local data with the data
> on the server. After that the service is considered to be syncing its data,
and
> it will get updates incrementally and be able to send updates to Sync. Before
> that happens, the service is not syncing its data. 
> 
> > Also, is it guaranteed that we've gotten one sync update when this gets
> called,
> > or might the data be outdated anyway?
> 
> The former. MergeDataAndStartSyncing() gets called with a list of all the data
> currently on the server (so Sync does an initial GetUpdates() call, which is
why
> a SyncableService doesn't start out immediately in the syncing state).

OK -- could you add a brief comment explaining both those, please? Something
like

This method should only be called once the service is syncing managed users
(i.e., it has finished its initial merge of local- and server-side data, via
MergeDataAndStartSyncing), as the stored data might be outdated before that.

[Bernhard Bauer, 2013-09-05 12:52:35]

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
File chrome/browser/managed_mode/managed_user_sync_service.h (right):

https://codereview.chromium.org/23910002/diff/1/chrome/browser/managed_mode/m...
chrome/browser/managed_mode/managed_user_sync_service.h:50: // managed users (as
the stored data might be outdated before that).
On 2013/09/05 12:29:30, Pam (also PM for reviews) wrote:
> On 2013/09/05 12:15:41, Bernhard Bauer wrote:
> > On 2013/09/05 11:11:56, Pam (also PM for reviews) wrote:
> > > I don't understand this comment. What does it mean to not be "syncing
> managed
> > > users"?
> > 
> > A SyncableService needs to do an initial merge of its local data with the
data
> > on the server. After that the service is considered to be syncing its data,
> and
> > it will get updates incrementally and be able to send updates to Sync.
Before
> > that happens, the service is not syncing its data. 
> > 
> > > Also, is it guaranteed that we've gotten one sync update when this gets
> > called,
> > > or might the data be outdated anyway?
> > 
> > The former. MergeDataAndStartSyncing() gets called with a list of all the
data
> > currently on the server (so Sync does an initial GetUpdates() call, which is
> why
> > a SyncableService doesn't start out immediately in the syncing state).
> 
> OK -- could you add a brief comment explaining both those, please? Something
> like
> 
> This method should only be called once the service is syncing managed users
> (i.e., it has finished its initial merge of local- and server-side data, via
> MergeDataAndStartSyncing), as the stored data might be outdated before that.

Done.

[commit-bot: I haz the power, 2013-09-05 12:53:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bauerb@chromium.org/23910002/12001

[commit-bot: I haz the power, 2013-09-05 19:05:32]

Change committed as 221486