Make SyncBackendRegistrar aware of loaded data

Make SyncBackendRegistrar aware of loaded data

Adds a parameter to the SyncManager::Initialize() callback to inform the
caller which sync data types were successfully loaded from disk. This
allows the SyncBackendHost and related classes make better decisions
during initialization.

If necessary, the SyncBackendHost will send a configure request to the
syncer during early initialization asking it to download the nigori
node. Now we can guarantee that the node will be available by the type
ProfileSyncService::OnBackendInitialized() is called.

The SyncBackendHost test expectations had to be amended to account for
this test. Most of the changes are related to the fact that our
behaviour no longer depends on the SyncPrefs.

The ProfileSyncService*Tests were very much affected by this change.
Those tests are parameterized with a callback that is used to initialize
the sync DB and pretend that it had been loaded from disk. Previously,
this callback would be executed later on during initialization, around
the time of ProfileSyncService::OnBackendInitialize(). That approach was
incompatible with this change, which requires that we have access to the
fully initialized database around the time we return from
SyncBackendHost::Initialize(). The callback had to be moved, which meant
that it now required an explicit UserShare parameter, which meant that a
lot of call sites had to be updated.

BUG=129825
TEST=

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

[rlarocque, 2012-07-20 17:28:40]

This is a big patch, so let's split it up.

Tim: Please review the tests.
Nicolas: Please review everything else and the sync_backend_host_unittests.

[tim (not reviewing), 2012-07-20 21:00:57]

Cool patch! Preliminary comment.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:104:
callback_.Run(GetUserShareForTest());
Plumbing the UserShare* around sure got a bit messy :/  Especially since long
term it would be nice to not actually require UserShare (or even expose it
further than SyncBackendHost).

The callbacks all seem to have access to the AbstractProfileSyncServiceTest*, so
it shouldn't actually be required since, if you're going to keep a
GetUserShareForTest method around, they should be able to query for it on-demand
via the TestProfileSyncService?

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:156: static void
DoNothingCallback(syncer::UserShare*) {
I think you can get rid of this (and more boilerplate elsewhere) and just use
base::Callback() if you can avoid plumbing UserShare*.

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager_impl.cc:926: bool success = SignIn(credentials);
It makes me a bit nervous to start doing "real work" like SignIn before we've
created a SyncSessionContext / SyncScheduler.  I'm reminded of crashes we've had
in the past with notifications being sent (for dir open) and listeners expecting
things to be around that aren't.

[rlarocque, 2012-07-20 21:17:59]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:104:
callback_.Run(GetUserShareForTest());
On 2012/07/20 21:00:57, timsteele wrote:
> Plumbing the UserShare* around sure got a bit messy :/  Especially since long
> term it would be nice to not actually require UserShare (or even expose it
> further than SyncBackendHost).
> 
> The callbacks all seem to have access to the AbstractProfileSyncServiceTest*,
so
> it shouldn't actually be required since, if you're going to keep a
> GetUserShareForTest method around, they should be able to query for it
on-demand
> via the TestProfileSyncService?

The main problem is that that no one is allowed to touch UserShare until
PSS::OnBackendInitialized() returns.  There are DCHECKs that ensure this, and
for good reason.

I consider the callbacks to be running in the SyncManager context because
they're trying to replace the SyncManager's directory initialization routines. 
In a way, each one of them is like a fragment of a MockSyncManager that no one
wanted to write the boilerplate for.  I guess that means I could give them a
pointer to the SyncManager, but I'd prefer to give them only the UserShare.

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager_impl.cc:926: bool success = SignIn(credentials);
On 2012/07/20 21:00:57, timsteele wrote:
> It makes me a bit nervous to start doing "real work" like SignIn before we've
> created a SyncSessionContext / SyncScheduler.  I'm reminded of crashes we've
had
> in the past with notifications being sent (for dir open) and listeners
expecting
> things to be around that aren't.

Good point.

Could we get rid of the SignIn() function now that we're refactoring out the
"for test" code?  I'd like to move the directory opening part of it to here,
then leave UpdateCredentials() until later.

[Nicolas Zea, 2012-07-20 21:35:39]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:592: // never send a new configure
request until the current request succeeds.
I think it'd be useful to also mention that the backend doesn't know about any
newly supported types until after the backend is initialized. During
initialization time the backend only knows about pre-existing types.

Also probably worth adding a TODO that once we stop purging all partial types,
we're going to need to change this to not attempt to download anything other
than the nigori as part of initialization.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:607: //
types_to_remove_with_nigori from the list then adding
it's actually the other way around (add, then remove)

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:608: // types_to_add_with_nigori. 
Any types which have are not in either of those
which are not

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:611: // Types which were not in
previously in the list are not cached locally, so
not in the list previously

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:660: syncer::UserShare*
SyncBackendHost::GetUserShareForTest() const {
I'd almost rather just get rif of DCHECK(initialized()) than have this extra
ForTest method. If we're not initialized, the above method is going to crash
anyways right?

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.h:20: #include
"chrome/browser/sync/glue/sync_backend_registrar.h"
Any reason you're not forward declaring this anymore?

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:287: // All partial types
should have been purged with nothing downloaded as part
update comment

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:291:
syncer::ModelTypeSet(syncer::NIGORI)));
Fix indents in this test and the next.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:300: // nigori and
bookmarks.
this comment too

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:319: // initializing.
Initialization should not affect the datatypes.
and this

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_registrar.cc:85: void
SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
add UI thread dcheck. Also probably good to add a DCHECK that routing info is
empty?

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_registrar.cc:107: 
remove newline

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
sync/internal_api/public/sync_manager.h:301: bool success, syncer::ModelTypeSet
restored_types) = 0;
nit: prefer having last arg on separate line

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
File sync/internal_api/sync_manager_impl.cc (left):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager_impl.cc:940: model_safe_routing_info,
This concerns me a bit. Could you set the routing info manually right before
notifying OnInitializationComplete?

[rlarocque, 2012-07-20 23:20:05]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:592: // never send a new configure
request until the current request succeeds.
On 2012/07/20 21:35:39, nzea wrote:
> I think it'd be useful to also mention that the backend doesn't know about any
> newly supported types until after the backend is initialized. During
> initialization time the backend only knows about pre-existing types.

Why add that comment here?  Is the configuration for newly supported types
handled differently?  
 
> Also probably worth adding a TODO that once we stop purging all partial types,
> we're going to need to change this to not attempt to download anything other
> than the nigori as part of initialization.

Why put that comment here, as opposed to
HandleInitializationCompleteOnFrontendLoop()?

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:660: syncer::UserShare*
SyncBackendHost::GetUserShareForTest() const {
On 2012/07/20 21:35:39, nzea wrote:
> I'd almost rather just get rif of DCHECK(initialized()) than have this extra
> ForTest method. If we're not initialized, the above method is going to crash
> anyways right?

The intent of this CL is to ensure that the ProfileSyncService can never get a
hold of an improperly initialized UserShare object.  Allowing the
ProfileSyncService to access the UserShare before OnBackendInitialize makes it
harder to enforce this.

I must admit that the ProfileSyncServiceTest code does provide a fairly strong
practical argument for allowing it.  On the other hand, that work has been done,
so the argument that it lets us avoid updating a bunch of test code carries less
weight now.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.h:20: #include
"chrome/browser/sync/glue/sync_backend_registrar.h"
On 2012/07/20 21:35:39, nzea wrote:
> Any reason you're not forward declaring this anymore?

No.  Fixed.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:291:
syncer::ModelTypeSet(syncer::NIGORI)));
On 2012/07/20 21:35:39, nzea wrote:
> Fix indents in this test and the next.

My editor seems to like these indents as is.  It's adding an extra level of
indentation for each level of nesting.  

I'm using the Google Coding Guidelines Vim plugin, so I'd be surprised if it got
it wrong.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:300: // nigori and
bookmarks.
On 2012/07/20 21:35:39, nzea wrote:
> this comment too

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_registrar.cc:85: void
SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
On 2012/07/20 21:35:39, nzea wrote:
> add UI thread dcheck. Also probably good to add a DCHECK that routing info is
> empty?

Done.  Also added a ScopedLock for extra safety.

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
sync/internal_api/public/sync_manager.h:301: bool success, syncer::ModelTypeSet
restored_types) = 0;
On 2012/07/20 21:35:39, nzea wrote:
> nit: prefer having last arg on separate line

Done.

[Nicolas Zea, 2012-07-21 00:17:35]

On 2012/07/20 23:20:05, rlarocque wrote:
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host.cc:592: // never send a new
configure
> request until the current request succeeds.
> On 2012/07/20 21:35:39, nzea wrote:
> > I think it'd be useful to also mention that the backend doesn't know about
any
> > newly supported types until after the backend is initialized. During
> > initialization time the backend only knows about pre-existing types.
> 
> Why add that comment here?  Is the configuration for newly supported types
> handled differently?  

Because you're already talking about how ConfigureDatatypes works in the context
of the entire system, and it's not clear how newly supported datatypes fit into
that.
If you think there's someplace better that's fine, but my first thought when
reading
this was to wonder why we don't break for newly supported datatypes.

>  
> > Also probably worth adding a TODO that once we stop purging all partial
types,
> > we're going to need to change this to not attempt to download anything other
> > than the nigori as part of initialization.
> 
> Why put that comment here, as opposed to
> HandleInitializationCompleteOnFrontendLoop()?

That's fine, as long as it's clear that we rely on the lack of any partial
datatypes,
else we risk downloading types that are susceptible to migration before the
backend
is initialzied.

> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host.cc:660: syncer::UserShare*
> SyncBackendHost::GetUserShareForTest() const {
> On 2012/07/20 21:35:39, nzea wrote:
> > I'd almost rather just get rif of DCHECK(initialized()) than have this extra
> > ForTest method. If we're not initialized, the above method is going to crash
> > anyways right?
> 
> The intent of this CL is to ensure that the ProfileSyncService can never get a
> hold of an improperly initialized UserShare object.  Allowing the
> ProfileSyncService to access the UserShare before OnBackendInitialize makes it
> harder to enforce this.
> 
> I must admit that the ProfileSyncServiceTest code does provide a fairly strong
> practical argument for allowing it.  On the other hand, that work has been
done,
> so the argument that it lets us avoid updating a bunch of test code carries
less
> weight now.
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_host.h (right):
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host.h:20: #include
> "chrome/browser/sync/glue/sync_backend_registrar.h"
> On 2012/07/20 21:35:39, nzea wrote:
> > Any reason you're not forward declaring this anymore?
> 
> No.  Fixed.
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host_unittest.cc:291:
> syncer::ModelTypeSet(syncer::NIGORI)));
> On 2012/07/20 21:35:39, nzea wrote:
> > Fix indents in this test and the next.
> 
> My editor seems to like these indents as is.  It's adding an extra level of
> indentation for each level of nesting.  
> 
> I'm using the Google Coding Guidelines Vim plugin, so I'd be surprised if it
got
> it wrong.

If anything I'd say adjust for the sake of code consistency. If you prefer to
have the
call to fake manager start on the next line, then the third line have the 8
space
indent, that would fit better too. I don't think I've ever seen chromium code
that indents by more than four spaces from the start of the previous line
(excluding when the code lines up with the parenthesis).

The style guide itself just says indent by four spaces for wrapped parenthesis.
There's no mention of four spaces per level of nesting, so I'd rather just be
consistent.

> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host_unittest.cc:300: // nigori and
> bookmarks.
> On 2012/07/20 21:35:39, nzea wrote:
> > this comment too
> 
> Done.
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_registrar.cc (right):
> 
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_registrar.cc:85: void
> SyncBackendRegistrar::SetInitialTypes(syncer::ModelTypeSet initial_types) {
> On 2012/07/20 21:35:39, nzea wrote:
> > add UI thread dcheck. Also probably good to add a DCHECK that routing info
is
> > empty?
> 
> Done.  Also added a ScopedLock for extra safety.
> 
>
http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
> File sync/internal_api/public/sync_manager.h (right):
> 
>
http://codereview.chromium.org/10804039/diff/1/sync/internal_api/public/sync_...
> sync/internal_api/public/sync_manager.h:301: bool success,
syncer::ModelTypeSet
> restored_types) = 0;
> On 2012/07/20 21:35:39, nzea wrote:
> > nit: prefer having last arg on separate line
> 
> Done.

[rlarocque, 2012-07-21 01:42:50]

On 2012/07/21 00:17:35, nzea wrote:
> On 2012/07/20 23:20:05, rlarocque wrote:
> >
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> > File chrome/browser/sync/glue/sync_backend_host.cc (right):
> > 
> >
>
http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
> > chrome/browser/sync/glue/sync_backend_host.cc:592: // never send a new
> configure
> > request until the current request succeeds.
> > On 2012/07/20 21:35:39, nzea wrote:
> > > I think it'd be useful to also mention that the backend doesn't know about
> any
> > > newly supported types until after the backend is initialized. During
> > > initialization time the backend only knows about pre-existing types.
> > 
> > Why add that comment here?  Is the configuration for newly supported types
> > handled differently?  
> 
> Because you're already talking about how ConfigureDatatypes works in the
context
> of the entire system, and it's not clear how newly supported datatypes fit
into
> that.
> If you think there's someplace better that's fine, but my first thought when
> reading
> this was to wonder why we don't break for newly supported datatypes.
> 

Done.

> >  
> > > Also probably worth adding a TODO that once we stop purging all partial
> types,
> > > we're going to need to change this to not attempt to download anything
other
> > > than the nigori as part of initialization.
> > 
> > Why put that comment here, as opposed to
> > HandleInitializationCompleteOnFrontendLoop()?
> 
> That's fine, as long as it's clear that we rely on the lack of any partial
> datatypes,
> else we risk downloading types that are susceptible to migration before the
> backend
> is initialzied.
> 

Now I remember which MIGRATION_DONE hazard you're talking about.  Documented.

> > chrome/browser/sync/glue/sync_backend_host_unittest.cc:291:
> > syncer::ModelTypeSet(syncer::NIGORI)));
> > On 2012/07/20 21:35:39, nzea wrote:
> > > Fix indents in this test and the next.
> > 
> > My editor seems to like these indents as is.  It's adding an extra level of
> > indentation for each level of nesting.  
> > 
> > I'm using the Google Coding Guidelines Vim plugin, so I'd be surprised if it
> got
> > it wrong.
> 
> If anything I'd say adjust for the sake of code consistency. If you prefer to
> have the
> call to fake manager start on the next line, then the third line have the 8
> space
> indent, that would fit better too. I don't think I've ever seen chromium code
> that indents by more than four spaces from the start of the previous line
> (excluding when the code lines up with the parenthesis).
> 
> The style guide itself just says indent by four spaces for wrapped
parenthesis.
> There's no mention of four spaces per level of nesting, so I'd rather just be
> consistent.
> 

I've modified my code to align better with the indentation used in the rest of
the file.

[Nicolas Zea, 2012-07-23 19:12:16]

By the way, I don't think you addressed all my comments from the first review
round?

http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:634: // TODO(137550): It's
dangerous to configure types that have progress markers.
nit: the more common format is TODO(sync/username): crbug.com/number.

http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1196: // NIGORI is the only
enabled type.
Note: as discussed offline, the server does not protect us here. The purging of
any partial nigori node at startup is what is protecting us here.

[rlarocque, 2012-07-23 20:05:16]

Patch updated.  

I think we should talk offline about the
GetUserShareForTest()/TestProfileSyncService init issues.

On 2012/07/23 19:12:16, nzea wrote:
> By the way, I don't think you addressed all my comments from the first review
> round?

I did miss a few.  I skimmed over those comments again and addressed some more
of them in this patch.  Please let me know if I've missed any others.

> 
>
http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
> chrome/browser/sync/glue/sync_backend_host.cc:634: // TODO(137550): It's
> dangerous to configure types that have progress markers.
> nit: the more common format is TODO(sync/username): crbug.com/number.

Fixed.

http://codereview.chromium.org/10804039/diff/10004/chrome/browser/sync/glue/s...
> chrome/browser/sync/glue/sync_backend_host.cc:1196: // NIGORI is the only
> enabled type.
> Note: as discussed offline, the server does not protect us here. The purging
of
> any partial nigori node at startup is what is protecting us here.

You're right, I don't know what I was thinking there. 

I modified and moved most of the comment to the configure code, where it's a bit
more relevant.  I left a comment about the partially synced nigori hazard in
this area.

[tim (not reviewing), 2012-07-23 20:33:19]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:104:
callback_.Run(GetUserShareForTest());
On 2012/07/20 21:17:59, rlarocque wrote:
> On 2012/07/20 21:00:57, timsteele wrote:
> > Plumbing the UserShare* around sure got a bit messy :/  Especially since
long
> > term it would be nice to not actually require UserShare (or even expose it
> > further than SyncBackendHost).
> > 
> > The callbacks all seem to have access to the
AbstractProfileSyncServiceTest*,
> so
> > it shouldn't actually be required since, if you're going to keep a
> > GetUserShareForTest method around, they should be able to query for it
> on-demand
> > via the TestProfileSyncService?
> 
> The main problem is that that no one is allowed to touch UserShare until
> PSS::OnBackendInitialized() returns.  There are DCHECKs that ensure this, and
> for good reason.

I agree it's good to keep the DCHECKs in the general case. Note there are
DCHECKs in SyncManagerImpl::GetUserShare as well, though. I don't want to make
code access the UserShare before it's safe to do so, I'm just looking for a way
other than passing it around in this callback -- if we're calling it here and
passing it through to Run(), then it is safe to query for it instead from within
the callback Run() impl.  We could possibly even enforce that tests *only*
access it during this callback, if we really wanted to.

Alternatively, we could remove SyncManager::GetUserShare altogether and pass it
back on the OnInitializationComplete callback. That's the most extreme way to
disallow access to an uninitialized UserShare, but it would require caching the
value so that things can open transactions (though the actual value should never
change aside from null -> !null -> null).

> 
> I consider the callbacks to be running in the SyncManager context because
> they're trying to replace the SyncManager's directory initialization routines.

> In a way, each one of them is like a fragment of a MockSyncManager that no one
> wanted to write the boilerplate for.  I guess that means I could give them a
> pointer to the SyncManager, but I'd prefer to give them only the UserShare.

[Nicolas Zea, 2012-07-23 20:47:15]

Mostly nits

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:614: //
types_to_remove_with_nigori from the list then adding
add then remove.

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:618: // Types which were not in
the list previously are not cached locally, so we
Cached seems kind of ambiguous. Perhaps just say are not fully downloaded?

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:623: // The SyncBackendRegistrar's
state was initially derrived from the types
derrived ->derived

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1207: // backend to ensure that
this type never has both progress markers and
did you mean rely on the sync manager purging? Since the backend isn't really
involved in that aspect.

http://codereview.chromium.org/10804039/diff/16001/sync/internal_api/public/s...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10804039/diff/16001/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:375: syncer::ExtensionsActivityMonitor*
extensions_activity_monitor,
remove syncer::

http://codereview.chromium.org/10804039/diff/16001/sync/sessions/sync_session...
File sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/10804039/diff/16001/sync/sessions/sync_session...
sync/sessions/sync_session_unittest.cc:63: NULL, NULL, workers,
&extensions_activity_monitor_,
nit: these args should be on their own lines unless they're intimately related
to one another.

[rlarocque, 2012-07-23 20:50:29]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:607: //
types_to_remove_with_nigori from the list then adding
On 2012/07/20 21:35:39, nzea wrote:
> it's actually the other way around (add, then remove)

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:608: // types_to_add_with_nigori. 
Any types which have are not in either of those
On 2012/07/20 21:35:39, nzea wrote:
> which are not

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:611: // Types which were not in
previously in the list are not cached locally, so
On 2012/07/20 21:35:39, nzea wrote:
> not in the list previously

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:287: // All partial types
should have been purged with nothing downloaded as part
On 2012/07/20 21:35:39, nzea wrote:
> update comment

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:319: // initializing.
Initialization should not affect the datatypes.
On 2012/07/20 21:35:39, nzea wrote:
> and this

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:513: // Purge the partial
types.
Fixed this comment, too.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_registrar.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_registrar.cc:107: 
On 2012/07/20 21:35:39, nzea wrote:
> remove newline

Done.

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:104:
callback_.Run(GetUserShareForTest());
On 2012/07/23 20:33:19, timsteele wrote:
> On 2012/07/20 21:17:59, rlarocque wrote:
> > On 2012/07/20 21:00:57, timsteele wrote:
> > > Plumbing the UserShare* around sure got a bit messy :/  Especially since
> long
> > > term it would be nice to not actually require UserShare (or even expose it
> > > further than SyncBackendHost).
> > > 
> > > The callbacks all seem to have access to the
> AbstractProfileSyncServiceTest*,
> > so
> > > it shouldn't actually be required since, if you're going to keep a
> > > GetUserShareForTest method around, they should be able to query for it
> > on-demand
> > > via the TestProfileSyncService?
> > 
> > The main problem is that that no one is allowed to touch UserShare until
> > PSS::OnBackendInitialized() returns.  There are DCHECKs that ensure this,
and
> > for good reason.
> 
> I agree it's good to keep the DCHECKs in the general case. Note there are
> DCHECKs in SyncManagerImpl::GetUserShare as well, though. I don't want to make
> code access the UserShare before it's safe to do so, I'm just looking for a
way
> other than passing it around in this callback -- if we're calling it here and
> passing it through to Run(), then it is safe to query for it instead from
within
> the callback Run() impl.  We could possibly even enforce that tests *only*
> access it during this callback, if we really wanted to.

The callbacks only have access to a TestProfileSyncService.  At the point of the
callback, the TestProfileSyncService is waiting for an OnBackendInitialized()
call, and (in theory) has no idea what state the backed is in.  It felt wrong to
add a getter to the TestProfileSyncService that would grab the directory at that
point.  Even if we did add such a getter, I think we'd still need to go through
the SyncBackendHost, so the SyncBackendHost::GetUserShareForTest() would still
be necessary.  

> 
> Alternatively, we could remove SyncManager::GetUserShare altogether and pass
it
> back on the OnInitializationComplete callback. That's the most extreme way to
> disallow access to an uninitialized UserShare, but it would require caching
the
> value so that things can open transactions (though the actual value should
never
> change aside from null -> !null -> null).
> 

I considered that.  Although it works great for init, it doesn't work as well
during shutdown.  If we put the PSS in charge of storing a pointer to the
UserShare, it's hard (though not impossible) to guarantee there are no pointers
to it left by the time it's deleted.  

> > 
> > I consider the callbacks to be running in the SyncManager context because
> > they're trying to replace the SyncManager's directory initialization
routines.
> 
> > In a way, each one of them is like a fragment of a MockSyncManager that no
one
> > wanted to write the boilerplate for.  I guess that means I could give them a
> > pointer to the SyncManager, but I'd prefer to give them only the UserShare.
>

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
File sync/internal_api/sync_manager_impl.cc (left):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager_impl.cc:940: model_safe_routing_info,
On 2012/07/20 21:35:39, nzea wrote:
> This concerns me a bit. Could you set the routing info manually right before
> notifying OnInitializationComplete?

This shouldn't have any practical impact because this will eventually be set by
the post-OnBackendInit() configure job, right?

Whether or not this makes sense depends on what the purpose of the context's
routing info is supposed to be.  

I'd prefer to think of the context's routing info as the set of types that the
SyncManager has been asked to sync.  I don't think initializing it based on the
initial_sync_ended types makes sense in all cases.

[tim (not reviewing), 2012-07-23 21:01:58]

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
File chrome/browser/sync/test_profile_sync_service.cc (right):

http://codereview.chromium.org/10804039/diff/1/chrome/browser/sync/test_profi...
chrome/browser/sync/test_profile_sync_service.cc:104:
callback_.Run(GetUserShareForTest());
On 2012/07/23 20:50:30, rlarocque wrote:
> On 2012/07/23 20:33:19, timsteele wrote:
> > On 2012/07/20 21:17:59, rlarocque wrote:
> > > On 2012/07/20 21:00:57, timsteele wrote:
> > > > Plumbing the UserShare* around sure got a bit messy :/  Especially since
> > long
> > > > term it would be nice to not actually require UserShare (or even expose
it
> > > > further than SyncBackendHost).
> > > > 
> > > > The callbacks all seem to have access to the
> > AbstractProfileSyncServiceTest*,
> > > so
> > > > it shouldn't actually be required since, if you're going to keep a
> > > > GetUserShareForTest method around, they should be able to query for it
> > > on-demand
> > > > via the TestProfileSyncService?
> > > 
> > > The main problem is that that no one is allowed to touch UserShare until
> > > PSS::OnBackendInitialized() returns.  There are DCHECKs that ensure this,
> and
> > > for good reason.
> > 
> > I agree it's good to keep the DCHECKs in the general case. Note there are
> > DCHECKs in SyncManagerImpl::GetUserShare as well, though. I don't want to
make
> > code access the UserShare before it's safe to do so, I'm just looking for a
> way
> > other than passing it around in this callback -- if we're calling it here
and
> > passing it through to Run(), then it is safe to query for it instead from
> within
> > the callback Run() impl.  We could possibly even enforce that tests *only*
> > access it during this callback, if we really wanted to.
> 
> The callbacks only have access to a TestProfileSyncService.  At the point of
the
> callback, the TestProfileSyncService is waiting for an OnBackendInitialized()
> call, and (in theory) has no idea what state the backed is in.  It felt wrong
to
> add a getter to the TestProfileSyncService that would grab the directory at
that
> point.  Even if we did add such a getter, I think we'd still need to go
through
> the SyncBackendHost, so the SyncBackendHost::GetUserShareForTest() would still
> be necessary.

Yeah, I think it would be too (my initial comment noted 'if you're going to keep
a GetUserShareForTest method around'). The extra plumbing in this patch though
still seems unnecessary.  If we're passing it in the callback, then we're
effectively telling the Run implementation that it's safe to use that pointer,
right?  So can we just make that explicit either in the documentation or extra
DCHECKs?

   
> 
> > 
> > Alternatively, we could remove SyncManager::GetUserShare altogether and pass
> it
> > back on the OnInitializationComplete callback. That's the most extreme way
to
> > disallow access to an uninitialized UserShare, but it would require caching
> the
> > value so that things can open transactions (though the actual value should
> never
> > change aside from null -> !null -> null).
> > 
> 
> I considered that.  Although it works great for init, it doesn't work as well
> during shutdown.  If we put the PSS in charge of storing a pointer to the
> UserShare, it's hard (though not impossible) to guarantee there are no
pointers
> to it left by the time it's deleted. 

PSS definitely shouldn't concern itself with this, but SBH could (in theory).
Seems like it could be cleared at exactly the right time in SBH::Shutdown (so
after StopSyncingForShutdown, and after ChangeProcessors are stopped).
 
> 
> > > 
> > > I consider the callbacks to be running in the SyncManager context because
> > > they're trying to replace the SyncManager's directory initialization
> routines.
> > 
> > > In a way, each one of them is like a fragment of a MockSyncManager that no
> one
> > > wanted to write the boilerplate for.  I guess that means I could give them
a
> > > pointer to the SyncManager, but I'd prefer to give them only the
UserShare.
> > 
>

[rlarocque, 2012-07-23 23:16:10]

Responding to Nicolas' comments.

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:614: //
types_to_remove_with_nigori from the list then adding
On 2012/07/23 20:47:15, nzea wrote:
> add then remove.

You mentioned this in your first set of comments, so I fixed it.  Then, when I
was re-reading your comments, decided to swap them again.

Fixed.  Again.

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:618: // Types which were not in
the list previously are not cached locally, so we
On 2012/07/23 20:47:15, nzea wrote:
> Cached seems kind of ambiguous. Perhaps just say are not fully downloaded?

Done.

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:623: // The SyncBackendRegistrar's
state was initially derrived from the types
On 2012/07/23 20:47:15, nzea wrote:
> derrived ->derived

Done.

http://codereview.chromium.org/10804039/diff/16001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1207: // backend to ensure that
this type never has both progress markers and
On 2012/07/23 20:47:15, nzea wrote:
> did you mean rely on the sync manager purging? Since the backend isn't really
> involved in that aspect.

I'm using "backend" as a generic term to refer to the code on the sync thread. 
I figured that would make the comment less likely to go stale if we changed the
specifics of our purging implementation.  

However, the comment's not very useful if people don't understand it.  I'll
change it to sync manager.

http://codereview.chromium.org/10804039/diff/16001/sync/internal_api/public/s...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10804039/diff/16001/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:375: syncer::ExtensionsActivityMonitor*
extensions_activity_monitor,
On 2012/07/23 20:47:15, nzea wrote:
> remove syncer::

Done.

http://codereview.chromium.org/10804039/diff/16001/sync/sessions/sync_session...
File sync/sessions/sync_session_unittest.cc (right):

http://codereview.chromium.org/10804039/diff/16001/sync/sessions/sync_session...
sync/sessions/sync_session_unittest.cc:63: NULL, NULL, workers,
&extensions_activity_monitor_,
On 2012/07/23 20:47:15, nzea wrote:
> nit: these args should be on their own lines unless they're intimately related
> to one another.

Done.

[Nicolas Zea, 2012-07-23 23:54:55]

my part LGTM

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
File sync/internal_api/sync_manager_impl.cc (left):

http://codereview.chromium.org/10804039/diff/1/sync/internal_api/sync_manager...
sync/internal_api/sync_manager_impl.cc:940: model_safe_routing_info,
On 2012/07/23 20:50:30, rlarocque wrote:
> On 2012/07/20 21:35:39, nzea wrote:
> > This concerns me a bit. Could you set the routing info manually right before
> > notifying OnInitializationComplete?
> 
> This shouldn't have any practical impact because this will eventually be set
by
> the post-OnBackendInit() configure job, right?
> 
> Whether or not this makes sense depends on what the purpose of the context's
> routing info is supposed to be.  
> 
> I'd prefer to think of the context's routing info as the set of types that the
> SyncManager has been asked to sync.  I don't think initializing it based on
the
> initial_sync_ended types makes sense in all cases.  

Hmm, it'll in fact get set by the initial Nigori-configure job. Yeah, I guess
that's fine, although I'm a bit concerned about something in the scheduler
trying to use it in some way before then. But theoretically the scheduler
shouldn't be doing anything until it gets configured I think, so this is
probably fine.

[rlarocque, 2012-07-24 00:35:18]

Removed the UserShare callback changes.  PTAL.

[tim (not reviewing), 2012-07-24 01:01:39]

On 2012/07/24 00:35:18, rlarocque wrote:
> Removed the UserShare callback changes.  PTAL.

Getting back to the SignIn comment, where did you want to move UpdateCredentials
to?  It's important we do that before anything tries to start / nudge / config
the scheduler.

[rlarocque, 2012-07-25 18:44:51]

On 2012/07/24 01:01:39, timsteele wrote:
> On 2012/07/24 00:35:18, rlarocque wrote:
> > Removed the UserShare callback changes.  PTAL.
> 
> Getting back to the SignIn comment, where did you want to move
UpdateCredentials
> to?  It's important we do that before anything tries to start / nudge / config
> the scheduler.

I inlined SignIn() and its call to UpdateCredentials().  I figure there's no
reason to make something a function if it's only going to be called from one
context and it accesses a bunch of member variables.

This has very little impact on client behaviour, but it does make the
implications of moving the SignIn() call a bit easier to see.  My hope was that
some of the calls would obviously belong either before or after the context is
initialized.  It turns out that doing everything early looks fairly safe.

I don't care if we do or do not decide to commit this refactoring.  Either way,
I think it does demonstrate that moving the SignIn() call is safe.  (Safer than
I expected, at least.)

[rlarocque, 2012-07-25 18:45:08]

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
File sync/internal_api/sync_manager_impl.cc (right):

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:415: share_.name = credentials.email;
Formerly part of SignIn()

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:430: DVLOG(1) << "Username: " <<
username_for_share();
Inlined SignIn code begins here.

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:450:
connection_manager_->set_auth_token(credentials.sync_token);
These two lines are effectively what UpdateCredentials does in this context. 
Normally, it would also:
- set observering_ip_address_changes_, but that's redundant because we just set
it a few lines earlier.
- not update the notifier's credentials if set_auth_token fails, but this call
to set_auth_token will not fail.
- Call scheduler_->OnCredentialsUpdated() if initialized_, but initialized_ is
not set right now.

Not calling scheduler_->OnCredentialsUpdated() is a real change.  We would have
called it when SignIn() was in its traditional location.  Fortunately, the call
has no effect during startup; it's used mainly to clear auth errors.

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:451:
sync_notifier_->UpdateCredentials(credentials.email,
I think it's safe to initialize the sync_notifier here.  We're on the sync
thread, and won't release the thread until the SyncScheduler and context are
prepared to handle notifications.  

We could move it to below the session_context_ initialization and
scheduler_->Start() calls if you're worried, though.

http://codereview.chromium.org/10804039/diff/18001/sync/internal_api/sync_man...
sync/internal_api/sync_manager_impl.cc:459: session_context_ =
internal_components_factory->BuildContext(
Warning: Real code change here.

We used to create the session context regardless of whether or not the directory
open failed.  With this refactoring, it is initialized only when the directory
opened successfully.  This is probably better, though we should be wary of any
change.

[tim (not reviewing), 2012-07-26 18:18:31]

On 2012/07/25 18:44:51, rlarocque wrote:
> On 2012/07/24 01:01:39, timsteele wrote:
> > On 2012/07/24 00:35:18, rlarocque wrote:
> > > Removed the UserShare callback changes.  PTAL.
> > 
> > Getting back to the SignIn comment, where did you want to move
> UpdateCredentials
> > to?  It's important we do that before anything tries to start / nudge /
config
> > the scheduler.
> 
> I inlined SignIn() and its call to UpdateCredentials().  I figure there's no
> reason to make something a function if it's only going to be called from one
> context and it accesses a bunch of member variables.
> 
> This has very little impact on client behaviour, but it does make the
> implications of moving the SignIn() call a bit easier to see.  My hope was
that
> some of the calls would obviously belong either before or after the context is
> initialized.  It turns out that doing everything early looks fairly safe.
> 
> I don't care if we do or do not decide to commit this refactoring.  Either
way,
> I think it does demonstrate that moving the SignIn() call is safe.  (Safer
than
> I expected, at least.)

A big reason to prefer more stepwise functions is readability.  Being able to
see an entire function (like Init) on one screen and having it call well-named
methods makes it much easier to get a feel for the full scope of what the
function is responsible for.  See the style guide,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Write_...

Now that you've inlined SignIn, is there a new way we could recombine things
into logical chunks?  Taking a look at your next set of comments...

[tim (not reviewing), 2012-07-26 18:48:05]

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:432: if (success) {
I know this code did if (success) { before, but now there's even more
indentation.  What do we have to worry about if (!success)?  Could we bail?  I
think the only thing we're factoring out this way is firing the notification,
which already has a ternary to distinguish |success| anyway.  I think it'd be
clearer to do

if (!success) {
  FOR_EACH_OBSERVER(...)
  return false;
}

// carry on

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:450:
connection_manager_->set_auth_token(credentials.sync_token);
On 2012/07/25 18:45:08, rlarocque wrote:
> These two lines are effectively what UpdateCredentials does in this context. 
> Normally, it would also:
> - set observering_ip_address_changes_, but that's redundant because we just
set
> it a few lines earlier.
> - not update the notifier's credentials if set_auth_token fails, but this call
> to set_auth_token will not fail.
> - Call scheduler_->OnCredentialsUpdated() if initialized_, but initialized_ is
> not set right now.
> 
> Not calling scheduler_->OnCredentialsUpdated() is a real change.  We would
have
> called it when SignIn() was in its traditional location.  Fortunately, the
call
> has no effect during startup; it's used mainly to clear auth errors.

What is the benefit of not calling UpdateCredentials? There's a lot to gain from
reusing a single code path and set of assumptions to handle all this and rely on
pre/post conditions.

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:451:
sync_notifier_->UpdateCredentials(credentials.email,
On 2012/07/25 18:45:08, rlarocque wrote:
> I think it's safe to initialize the sync_notifier here.  We're on the sync
> thread, and won't release the thread until the SyncScheduler and context are
> prepared to handle notifications.  
> 
> We could move it to below the session_context_ initialization and
> scheduler_->Start() calls if you're worried, though.

I think this is okay too - the call to AddObserver is the critical one below,
because calling sync_notifier->UpdateCredentials (and SetState) can trigger
notifications, and we don't want to handle them if we're !initialized_.

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:459: session_context_ =
internal_components_factory->BuildContext(
On 2012/07/25 18:45:08, rlarocque wrote:
> Warning: Real code change here.
> 
> We used to create the session context regardless of whether or not the
directory
> open failed.  With this refactoring, it is initialized only when the directory
> opened successfully.  This is probably better, though we should be wary of any
> change.

Agreed, it would be strange if we were relying on a notification (from the
context) in the case where we failed to open the directory, which is the only
possible side effect I can see.  Although SendInitialSnapshot comes to mind...
do the integration tests like this?

[rlarocque, 2012-07-26 21:59:54]

Uploaded two new patches.

The first one implements Tim's suggestion to re-arrange the failure path for
OpenDirectory.  Along the way, I decided to move the call to
PurgePartiallySyncedTypes() into OpenDirectory and had to remove a few DCHECKs
in the process.

The second patch is a rebase.  I decided to remove the expectations around
enabled types in the SyncBackendHost unit tests for the *first* configure, since
we'll never perform a regular sync cycle from that state, and the enabled types
will be clobbered by the second configure.  In other words, I think it was safe
to remove those expectations because we don't care what the set of enables types
was at that time.

I left the call to UpdateCredentials during init inlined for now.  Tim, let me
know if you think it should be un-inlined.

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:432: if (success) {
On 2012/07/26 18:48:05, timsteele wrote:
> I know this code did if (success) { before, but now there's even more
> indentation.  What do we have to worry about if (!success)?  Could we bail?  I
> think the only thing we're factoring out this way is firing the notification,
> which already has a ternary to distinguish |success| anyway.  I think it'd be
> clearer to do
> 
> if (!success) {
>   FOR_EACH_OBSERVER(...)
>   return false;
> }
> 
> // carry on

Good idea.

https://chromiumcodereview.appspot.com/10804039/diff/18001/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:450:
connection_manager_->set_auth_token(credentials.sync_token);
On 2012/07/26 18:48:05, timsteele wrote:
> On 2012/07/25 18:45:08, rlarocque wrote:
> > These two lines are effectively what UpdateCredentials does in this context.

> > Normally, it would also:
> > - set observering_ip_address_changes_, but that's redundant because we just
> set
> > it a few lines earlier.
> > - not update the notifier's credentials if set_auth_token fails, but this
call
> > to set_auth_token will not fail.
> > - Call scheduler_->OnCredentialsUpdated() if initialized_, but initialized_
is
> > not set right now.
> > 
> > Not calling scheduler_->OnCredentialsUpdated() is a real change.  We would
> have
> > called it when SignIn() was in its traditional location.  Fortunately, the
> call
> > has no effect during startup; it's used mainly to clear auth errors.
> 
> What is the benefit of not calling UpdateCredentials? There's a lot to gain
from
> reusing a single code path and set of assumptions to handle all this and rely
on
> pre/post conditions. 

It's debatable.  The arguments for are:
- UpdateCredentials() behaves differently in init and non-init situations.  By
moving the initialization work here, we could remove its if (initialized_)
block.
- The inlined call gets simpler too.  We no longer need to check if
set_auth_token() fails and there's no need to set the boolean flag.
- Since so many member variables are involved, the initialization process is
made more readable by limiting the number of functions we use.  Otherwise it's
difficult to tell which member variables have and have not been initialized.

I prefer it this way, but I'm also willing to put it back the way it was.  There
are good arguments for reusing the code, too.

[tim (not reviewing), 2012-07-26 22:33:48]

still looking at s_b_h test changes

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (left):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:295: DCHECK(initialized_);
It still seems wrong from a client of SyncManager (i.e. SyncBackendHost) point
of view to call anything on us before we're initialized :/  Can we keep this
DCHECK somehow?

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:724: if (initialized_) {
Do we actually need this if (initialized_) block anyway?  Seems like it should
be safe to always call OnCredentialsUpdated, and the scheduler should figure
what matters itself?  I think just removing the if() is safe today, but looking
at scheduler_impl I see an argument that it should not pay attention if it
hasn't even been started yet (it surely hasn't seen a SyncSession go by and had
UpdateServerConnectionManagerStatus called!).

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:435: }
Yay, this looks way nicer.

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:455:
sync_notifier_->UpdateCredentials(credentials.email,
If we can get rid of the initialized_ special logic in UpdateCredentials (see
comment below) and still use it safely from here, then I'd say we should call
it.  If we can't, and the special casing is required, then I agree with you it's
just as good or better to make the if (initialized_) block in UpdateCredentials
a DCHECK(initialized_) and manually do the work here as you have it.

[rlarocque, 2012-07-26 22:52:20]

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (left):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:295: DCHECK(initialized_);
On 2012/07/26 22:33:48, timsteele wrote:
> It still seems wrong from a client of SyncManager (i.e. SyncBackendHost) point
> of view to call anything on us before we're initialized :/  Can we keep this
> DCHECK somehow?

My first instinct was to downgrade this to a DCHECK(directory()).  directory()
and initialized_ correlate very well, from the SBH's point of view.

Then I realized that there's no point in asserting on a pointer we're about to
de-reference anyway, so I removed it.

I know that doesn't quite answer your question, but it was enough to convince me
that removing the DCHECK made sense.

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:724: if (initialized_) {
On 2012/07/26 22:33:48, timsteele wrote:
> Do we actually need this if (initialized_) block anyway?  Seems like it should
> be safe to always call OnCredentialsUpdated, and the scheduler should figure
> what matters itself?  I think just removing the if() is safe today, but
looking
> at scheduler_impl I see an argument that it should not pay attention if it
> hasn't even been started yet (it surely hasn't seen a SyncSession go by and
had
> UpdateServerConnectionManagerStatus called!). 

See comment above.

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:455:
sync_notifier_->UpdateCredentials(credentials.email,
On 2012/07/26 22:33:48, timsteele wrote:
> If we can get rid of the initialized_ special logic in UpdateCredentials (see
> comment below) and still use it safely from here, then I'd say we should call
> it.  If we can't, and the special casing is required, then I agree with you
it's
> just as good or better to make the if (initialized_) block in
UpdateCredentials
> a DCHECK(initialized_) and manually do the work here as you have it.

The scheduler doesn't exist at this point, so the initialized_ check would is
necessary if we were to call UpdateCredentials() on this line.

If we were to save the UpdateCredentials call until later, as was the case when
SignIn() was invoked immediately after the scheduler was initialized, then it
might be safe to remove it.

[tim (not reviewing), 2012-07-27 01:54:32]

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (left):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:295: DCHECK(initialized_);
On 2012/07/26 22:52:20, rlarocque wrote:
> On 2012/07/26 22:33:48, timsteele wrote:
> > It still seems wrong from a client of SyncManager (i.e. SyncBackendHost)
point
> > of view to call anything on us before we're initialized :/  Can we keep this
> > DCHECK somehow?
> 
> My first instinct was to downgrade this to a DCHECK(directory()).  directory()
> and initialized_ correlate very well, from the SBH's point of view.
> 
> Then I realized that there's no point in asserting on a pointer we're about to
> de-reference anyway, so I removed it.
> 
> I know that doesn't quite answer your question, but it was enough to convince
me
> that removing the DCHECK made sense.
True re: asserting on a pointer we're about to deref isn't useful. But I think
the DCHECK is more about obeying the "Don't use SyncManager before it's
initialized" contract than it is about directory() being null or not.  There may
be other cases where we won't necessarily crash if a method is invoked
pre-initialization, but the result is unexpected / undefined, and DCHECK(init)
is needed. It could change here in the future, too.  So I would argue the best
practice is to have the dchecks in any public function exposed here.

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:455:
sync_notifier_->UpdateCredentials(credentials.email,
On 2012/07/26 22:52:20, rlarocque wrote:
> On 2012/07/26 22:33:48, timsteele wrote:
> > If we can get rid of the initialized_ special logic in UpdateCredentials
(see
> > comment below) and still use it safely from here, then I'd say we should
call
> > it.  If we can't, and the special casing is required, then I agree with you
> it's
> > just as good or better to make the if (initialized_) block in
> UpdateCredentials
> > a DCHECK(initialized_) and manually do the work here as you have it.
> 
> The scheduler doesn't exist at this point, so the initialized_ check would is
> necessary if we were to call UpdateCredentials() on this line.
> 
> If we were to save the UpdateCredentials call until later, as was the case
when
> SignIn() was invoked immediately after the scheduler was initialized, then it
> might be safe to remove it.

Oh, right, you mentioned that above too.  Can we try that (move it later, remove
check)?  It's nice to reuse if it's not special, and doing the work once the
same components exist in all cases eliminates a bunch questions that may come up
when debugging (e.g.) a crash in a posted task, or bug that gets introduced
later.

[tim (not reviewing), 2012-07-27 17:35:26]

LGTM! 3 cheers!

[rlarocque, 2012-07-27 17:47:19]

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (left):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:295: DCHECK(initialized_);
On 2012/07/27 01:54:32, timsteele wrote:
> On 2012/07/26 22:52:20, rlarocque wrote:
> > On 2012/07/26 22:33:48, timsteele wrote:
> > > It still seems wrong from a client of SyncManager (i.e. SyncBackendHost)
> point
> > > of view to call anything on us before we're initialized :/  Can we keep
this
> > > DCHECK somehow?
> > 
> > My first instinct was to downgrade this to a DCHECK(directory()). 
directory()
> > and initialized_ correlate very well, from the SBH's point of view.
> > 
> > Then I realized that there's no point in asserting on a pointer we're about
to
> > de-reference anyway, so I removed it.
> > 
> > I know that doesn't quite answer your question, but it was enough to
convince
> me
> > that removing the DCHECK made sense.
> True re: asserting on a pointer we're about to deref isn't useful. But I think
> the DCHECK is more about obeying the "Don't use SyncManager before it's
> initialized" contract than it is about directory() being null or not.  There
may
> be other cases where we won't necessarily crash if a method is invoked
> pre-initialization, but the result is unexpected / undefined, and DCHECK(init)
> is needed. It could change here in the future, too.  So I would argue the best
> practice is to have the dchecks in any public function exposed here.

Resolved offline.

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
File sync/internal_api/sync_manager_impl.cc (right):

https://chromiumcodereview.appspot.com/10804039/diff/23002/sync/internal_api/...
sync/internal_api/sync_manager_impl.cc:455:
sync_notifier_->UpdateCredentials(credentials.email,
On 2012/07/27 01:54:32, timsteele wrote:
> On 2012/07/26 22:52:20, rlarocque wrote:
> > On 2012/07/26 22:33:48, timsteele wrote:
> > > If we can get rid of the initialized_ special logic in UpdateCredentials
> (see
> > > comment below) and still use it safely from here, then I'd say we should
> call
> > > it.  If we can't, and the special casing is required, then I agree with
you
> > it's
> > > just as good or better to make the if (initialized_) block in
> > UpdateCredentials
> > > a DCHECK(initialized_) and manually do the work here as you have it.
> > 
> > The scheduler doesn't exist at this point, so the initialized_ check would
is
> > necessary if we were to call UpdateCredentials() on this line.
> > 
> > If we were to save the UpdateCredentials call until later, as was the case
> when
> > SignIn() was invoked immediately after the scheduler was initialized, then
it
> > might be safe to remove it.
> 
> Oh, right, you mentioned that above too.  Can we try that (move it later,
remove
> check)?  It's nice to reuse if it's not special, and doing the work once the
> same components exist in all cases eliminates a bunch questions that may come
up
> when debugging (e.g.) a crash in a posted task, or bug that gets introduced
> later.

Done.

I also moved the IPAddressChangeObserver registration because its callback
references the scheduler.  It would be very unlikely to cause problems in
practice, but it feels like it's one safe-looking CL away from developing into a
bug.

[commit-bot: I haz the power, 2012-07-30 01:54:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10804039/29003

[commit-bot: I haz the power, 2012-07-30 04:37:50]

Change committed as 148926