Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

Revert "Revert 142517 - [Sync] Refactor sync configuration logic."

Relanding with improved tests and support for cleaning up partial nigori.

The original patch was reverted due to users who needed to download a new nigori
node, but who had since been migrated. We do not handle migration before the
backend is initialized, so we now go ahead and detect+delete partial nigori
nodes before redownloading them. By blowing away the old progress marker,
we're no longer at risk of receiving the migration done response.

In order to test this the SyncBackendHost unit tests have been substantially upgraded.

original review at: https://chromiumcodereview.appspot.com/10483015

BUG=129665, 133061, 133219
TEST=unit/integration tests

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

[Nicolas Zea, 2012-07-04 02:05:51]

This patch is diffed off patchset 2 (where I do the actual revert of my old
patch).

+Richard for SBH/SBH unittest, specifically the changes to support
detecting/cleaning the nigori.
+Fred for the SBH unit test refactor + support for injecting a fake sync manager
via the new sync manager factory.

[rlarocque, 2012-07-04 06:39:34]

General comment: Why is the code careful about purging datatypes only when
they're in a very specific invalid state (!empty_progress_markers &&
!initial_sync_ended && type == NIGORI)?  Why not just purge all data for which
the initial_sync_ended bit is set?

I'm skeptical that the client would be able to make much use of the data it has
after being left in such a state.  I'd rather just purge all the data and get it
over with.

Overall I think this change looks pretty good.  The tests are especially
encouraging.  I'm just a bit paranoid after the previous refactor patch had to
be reverted.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.cc:374: if (!StartSyncThread())
Since the function is only a few lines long, non-virtual and only used once, I'd
prefer to see it inlined.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1179: case CLEANING_NIGORI: {
I'm not sure it makes sense to share code between the two states in this way.

For example, the UMA_HISTOGRAM_COUNTS code will be executed twice, but only if
the CLEANING_NIGORI step is executed.  We would be unable to differentiate
between a count due to the partially synced type situation this code is intended
to handle, and one triggered by the failure of this code.  

I'd suggest breaking this up into two states.  If we need to share code (ie. the
bits about calculating the partially synced types) we can move that out into a
function.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1197: // We apparently failed to
clean the nigori somehow. Attempting
Do you really expect this could happen?

If so, I think we need better handling than this.  We ought to delete the
database file, which is something that backend initialize failure no longer
does.  

Come to think of it, following the un-revert, we could start deleting the
directory on backend initialize failure once again.  Let's just do that in a
separate patch.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.cc:1210: nigori_state =
WITHOUT_NIGORI;
I spent a long time thinking about it, and I'm mostly convinced this will work.

It's a bit safer to do this here than elsewhere, since we've barely started init
and the PSS hasn't set the backend_initialized flag yet.  The scheduler is still
in configure mode, so I don't expect any callbacks to the cryptographer from the
syncer side of things, either.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
File chrome/browser/sync/glue/sync_backend_host.h (right):

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.h:28: #include
"sync/internal_api/public/sync_manager_factory.h"
Interesting... Is this part of the un-revert, or was it part of your original
patch?

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.h:281: typedef
base::Callback<scoped_ptr<syncer::HttpPostProviderFactory>(void)>
I'm pretty sure our scoped_ptr class supports this, and it seems like a good
idea.  I can't help but wonder if there's a reason why we don't see this pattern
more often.

I'm probably just being paranoid.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.h:334: MessageLoop* sync_loop() const
{ return sync_thread_.message_loop(); }
This should be on several lines.

Also, are you sure it makes sense to make this function const?  It returns a
non-const pointer.

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host.h:348: // InitializationComplete
passes through the SyncBackendHost to forward
Was this move necessary?  If not, could you undo it to preserve 'git blame'
history?

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

https://chromiumcodereview.appspot.com/10701085/diff/2004/chrome/browser/sync...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:113: void PumpSyncLoop()
{
I'm amazed that there isn't a better way to do this.

Would calling MessageLoop::Quit() not work?  I know it won't help us if one of
the tasks on that loop got stuck, but I think that's not really the test's
responsibility.  We can handle that outside the tests, with a wrapper script
that kills the test execution if it takes too long.

[Nicolas Zea, 2012-07-09 18:40:45]

On 2012/07/04 06:39:34, rlarocque wrote:
> General comment: Why is the code careful about purging datatypes only when
> they're in a very specific invalid state (!empty_progress_markers &&
> !initial_sync_ended && type == NIGORI)?  Why not just purge all data for which
> the initial_sync_ended bit is set?
> 
> I'm skeptical that the client would be able to make much use of the data it
has
> after being left in such a state.  I'd rather just purge all the data and get
it
> over with.


You mean unset? I special case nigori because we can handle partially synced
non-nigori's. What benefit do we get from blowing them away? It's a valid state
to be in to have a progress marker without initial sync ended for the other
types (we may have simply closed the browser before apply updates but after
persisting to disk for example).

> 
> Overall I think this change looks pretty good.  The tests are especially
> encouraging.  I'm just a bit paranoid after the previous refactor patch had to
> be reverted.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:374: if (!StartSyncThread())
On 2012/07/04 06:39:35, rlarocque wrote:
> Since the function is only a few lines long, non-virtual and only used once,
I'd
> prefer to see it inlined.  

Chromium style guide generally discourages inlining unless you actually know
it'll be useful (see
http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).

As you mentioned, this method is rarely used, so there's not going to be much
benefit to explicitly inlining it (and it's likely the compiler will inline
anyways).

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1179: case CLEANING_NIGORI: {
On 2012/07/04 06:39:35, rlarocque wrote:
> I'm not sure it makes sense to share code between the two states in this way.
> 
> For example, the UMA_HISTOGRAM_COUNTS code will be executed twice, but only if
> the CLEANING_NIGORI step is executed.  We would be unable to differentiate
> between a count due to the partially synced type situation this code is
intended
> to handle, and one triggered by the failure of this code.  
> 
> I'd suggest breaking this up into two states.  If we need to share code (ie.
the
> bits about calculating the partially synced types) we can move that out into a
> function.  

Done.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:1197: // We apparently failed to
clean the nigori somehow. Attempting
On 2012/07/04 06:39:35, rlarocque wrote:
> Do you really expect this could happen?
> 
> If so, I think we need better handling than this.  We ought to delete the
> database file, which is something that backend initialize failure no longer
> does.  
> 
> Come to think of it, following the un-revert, we could start deleting the
> directory on backend initialize failure once again.  Let's just do that in a
> separate patch.  

Yeah, I was planning on having that land separately.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:28: #include
"sync/internal_api/public/sync_manager_factory.h"
On 2012/07/04 06:39:35, rlarocque wrote:
> Interesting... Is this part of the un-revert, or was it part of your original
> patch?

Neither, it's an addition to support testing via dependency injection.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:281: typedef
base::Callback<scoped_ptr<syncer::HttpPostProviderFactory>(void)>
On 2012/07/04 06:39:35, rlarocque wrote:
> I'm pretty sure our scoped_ptr class supports this, and it seems like a good
> idea.  I can't help but wonder if there's a reason why we don't see this
pattern
> more often.
> 
> I'm probably just being paranoid.

It's a relatively new (past 6 months I think) addition to scoped_ptr, for just
this kind of situation. We haven't transitioned all places we pass ownership of
pointers, but we should. In this case, it wasn't clear there were two pointers
whose ownership was being passed to the sync manager, and hence I was having
memory leaks in my fake. Switching to scoped ptr solved that.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:334: MessageLoop* sync_loop() const
{ return sync_thread_.message_loop(); }
On 2012/07/04 06:39:35, rlarocque wrote:
> This should be on several lines.
> 
> Also, are you sure it makes sense to make this function const?  It returns a
> non-const pointer.

SImple inlined methods like this are acceptable to have on a single line from
what I've seen, although I haven't seen any rule in style guide one way or the
other about it. That said, it's not the first time in this file.

Also, it's fine that it doesn't return a const pointer. The point is it doesn't
modify anything itself, and hence can be called from within a const method.
Getter's in general, whether they're for pointers or not, are typically const.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.h:348: // InitializationComplete
passes through the SyncBackendHost to forward
On 2012/07/04 06:39:35, rlarocque wrote:
> Was this move necessary?  If not, could you undo it to preserve 'git blame'
> history?

Ah, originally I was calling this from a child, but looks like not anymore.
Moved back.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:113: void PumpSyncLoop()
{
On 2012/07/04 06:39:35, rlarocque wrote:
> I'm amazed that there isn't a better way to do this.
> 
> Would calling MessageLoop::Quit() not work?  I know it won't help us if one of
> the tasks on that loop got stuck, but I think that's not really the test's
> responsibility.  We can handle that outside the tests, with a wrapper script
> that kills the test execution if it takes too long.

How would MessageLoop::Quit help? The issue is that we don't know how long the
posted tasks (and possibly any tasks they post) would take to complete.
MessageLoop quit would have the same effect as this really, except we wouldn't
know if we timed out or not.

More importantly, since this message loop is in fact part of a thread, we
shouldn't be calling quit on it anyways, but using the thread's stop method.
DCHECK's are hit otherwise.

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
File chrome/browser/sync/glue/sync_backend_host.h (right):

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.h:354: private:
FYI this fell out in my rebase.

[rlarocque, 2012-07-09 19:22:58]

More comments.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:374: if (!StartSyncThread())
On 2012/07/09 18:40:45, nzea wrote:
> On 2012/07/04 06:39:35, rlarocque wrote:
> > Since the function is only a few lines long, non-virtual and only used once,
> I'd
> > prefer to see it inlined.  
> 
> Chromium style guide generally discourages inlining unless you actually know
> it'll be useful (see
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).
> 
> As you mentioned, this method is rarely used, so there's not going to be much
> benefit to explicitly inlining it (and it's likely the compiler will inline
> anyways).

By inline I meant "copy and paste the function body here".  I should have been
more clear about that.

I don't care about the performance or code size effects.  I just find it easier
to read the code when I don't have to look for functions all the time.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:113: void PumpSyncLoop()
{
On 2012/07/09 18:40:45, nzea wrote:
> On 2012/07/04 06:39:35, rlarocque wrote:
> > I'm amazed that there isn't a better way to do this.
> > 
> > Would calling MessageLoop::Quit() not work?  I know it won't help us if one
of
> > the tasks on that loop got stuck, but I think that's not really the test's
> > responsibility.  We can handle that outside the tests, with a wrapper script
> > that kills the test execution if it takes too long.
> 
> How would MessageLoop::Quit help? The issue is that we don't know how long the
> posted tasks (and possibly any tasks they post) would take to complete.
> MessageLoop quit would have the same effect as this really, except we wouldn't
> know if we timed out or not.
> 

My point is that it shouldn't be the test's responsibility to ensure that it
terminates in a reasonable amount of time.  We don't bother implementing these
checks in single-threaded tests, why should multi-threaded tests be any
different?

> More importantly, since this message loop is in fact part of a thread, we
> shouldn't be calling quit on it anyways, but using the thread's stop method.
> DCHECK's are hit otherwise.

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

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1161: syncer::ModelTypeSet
partially_synced_types =
This indentation doesn't look right.

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1218: ConfigureDataTypes(
I don't like the nigori_state variable, or the fact that there are two ways to
call ConfigureDataTypes(WITH_NIGORI).  We should talk about refactoring this.

[rlarocque, 2012-07-09 19:36:04]

On 2012/07/09 18:40:45, nzea wrote:
> On 2012/07/04 06:39:34, rlarocque wrote:
> > General comment: Why is the code careful about purging datatypes only when
> > they're in a very specific invalid state (!empty_progress_markers &&
> > !initial_sync_ended && type == NIGORI)?  Why not just purge all data for
which
> > the initial_sync_ended bit is set?
> > 
> > I'm skeptical that the client would be able to make much use of the data it
> has
> > after being left in such a state.  I'd rather just purge all the data and
get
> it
> > over with.
> 
> 
> You mean unset? I special case nigori because we can handle partially synced
> non-nigori's. What benefit do we get from blowing them away? It's a valid
state
> to be in to have a progress marker without initial sync ended for the other
> types (we may have simply closed the browser before apply updates but after
> persisting to disk for example).
> 

I can't point to any clear benefit from this, aside from some reduction in code
complexity.  

The "soft" argument for not allowing non-initial sync ended types with progress
markers is that it reduces the number of states in our system, at a cost that
would be invisible to most users.  

It means that the an initial GetUpdates *always* starts from empty progress
markers and no data.  The initial download without progress markers code path is
well tested and exercised regularly through our testing of the 'normal' initial
sync process.

Bugs that occur only with data types that have !initial_sync_ended and some data
downloaded can remain hidden until our code is pushed to a wider audience.  We
could fix this by testing this scenario a bit better, but this scenario is very
hard to test because it involves the client, server and some amount of initial
data.  It would be easier to avoid eliminate this class of bugs altogether by
not allowing this state.

[Nicolas Zea, 2012-07-09 19:42:49]

PTAL.

Regarding blowing away all partial types, I'd like to hold off on making a
non-fix change like that until we have more information. I agree it's possible
we can just ignore those cases, but I'd like to get more information about how
prevalent they are first (hence the histogram).

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host.cc:374: if (!StartSyncThread())
On 2012/07/09 19:22:58, rlarocque wrote:
> On 2012/07/09 18:40:45, nzea wrote:
> > On 2012/07/04 06:39:35, rlarocque wrote:
> > > Since the function is only a few lines long, non-virtual and only used
once,
> > I'd
> > > prefer to see it inlined.  
> > 
> > Chromium style guide generally discourages inlining unless you actually know
> > it'll be useful (see
> > http://dev.chromium.org/developers/coding-style#TOC-Inline-functions).
> > 
> > As you mentioned, this method is rarely used, so there's not going to be
much
> > benefit to explicitly inlining it (and it's likely the compiler will inline
> > anyways).
> 
> By inline I meant "copy and paste the function body here".  I should have been
> more clear about that.
> 
> I don't care about the performance or code size effects.  I just find it
easier
> to read the code when I don't have to look for functions all the time.

Oh, I see. I pulled it into its own method for use by tests (so they can start
the thread before initialize is called). This is also why the method is
protected, not private.

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
File chrome/browser/sync/glue/sync_backend_host_unittest.cc (right):

http://codereview.chromium.org/10701085/diff/2004/chrome/browser/sync/glue/sy...
chrome/browser/sync/glue/sync_backend_host_unittest.cc:113: void PumpSyncLoop()
{
On 2012/07/09 19:22:58, rlarocque wrote:
> On 2012/07/09 18:40:45, nzea wrote:
> > On 2012/07/04 06:39:35, rlarocque wrote:
> > > I'm amazed that there isn't a better way to do this.
> > > 
> > > Would calling MessageLoop::Quit() not work?  I know it won't help us if
one
> of
> > > the tasks on that loop got stuck, but I think that's not really the test's
> > > responsibility.  We can handle that outside the tests, with a wrapper
script
> > > that kills the test execution if it takes too long.
> > 
> > How would MessageLoop::Quit help? The issue is that we don't know how long
the
> > posted tasks (and possibly any tasks they post) would take to complete.
> > MessageLoop quit would have the same effect as this really, except we
wouldn't
> > know if we timed out or not.
> > 
> 
> My point is that it shouldn't be the test's responsibility to ensure that it
> terminates in a reasonable amount of time.  We don't bother implementing these
> checks in single-threaded tests, why should multi-threaded tests be any
> different?
> 
> > More importantly, since this message loop is in fact part of a thread, we
> > shouldn't be calling quit on it anyways, but using the thread's stop method.
> > DCHECK's are hit otherwise.

We do these kind of checks anywhere a single action has the possibility of
failing without any noticeable side effect. The test framework does have timeout
functionality, but this is useful for limiting the amount of time a single
action takes.

The benefit is that we can fail faster in the cases where a bug is introduced,
and know that the failure comes from this particular sport.  The alternative is
test timeouts, which manifest as crashes and don't tell us where they came from.

It's not necessarily true we never do it in single threaded scenarios (I believe
some of the tests that rely on i/o do this too). But yeah, multi threaded
scenarios are more prone to this, so use this framework.

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

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1161: syncer::ModelTypeSet
partially_synced_types =
On 2012/07/09 19:22:58, rlarocque wrote:
> This indentation doesn't look right.

Done.

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1218: ConfigureDataTypes(
On 2012/07/09 19:22:58, rlarocque wrote:
> I don't like the nigori_state variable, or the fact that there are two ways to
> call ConfigureDataTypes(WITH_NIGORI).  We should talk about refactoring this.

Can you be more specific about what you dislike? There are two ways because
there are two states, which I think makes sense, although I suppose this way is
a bit more "simple state machine"-y.

[rlarocque, 2012-07-09 20:30:22]

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

http://codereview.chromium.org/10701085/diff/26001/chrome/browser/sync/glue/s...
chrome/browser/sync/glue/sync_backend_host.cc:1218: ConfigureDataTypes(
On 2012/07/09 19:42:50, nzea wrote:
> On 2012/07/09 19:22:58, rlarocque wrote:
> > I don't like the nigori_state variable, or the fact that there are two ways
to
> > call ConfigureDataTypes(WITH_NIGORI).  We should talk about refactoring
this.
> 
> Can you be more specific about what you dislike? There are two ways because
> there are two states, which I think makes sense, although I suppose this way
is
> a bit more "simple state machine"-y.

Resolved offline.

The summary is that I wanted to see Configure(WITH_NIGORI) and
Configure(WITHOUT_NIGORI) treated as if they were two separate functions.  In
other words, no using a boolean value to switch between them.

Making that change alone would make things look a bit odd, triggering more
refactoring.

We decided that this version is good enough as is.

http://codereview.chromium.org/10701085/diff/37001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10701085/diff/37001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:68: ConfigurationParams::ConfigurationParams()
I'm confused.  Are the diffs from the unrevert leaking into this patch, or is
this actually new code introduced after patch set 5?

[Nicolas Zea, 2012-07-09 20:47:06]

http://codereview.chromium.org/10701085/diff/37001/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10701085/diff/37001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:68: ConfigurationParams::ConfigurationParams()
On 2012/07/09 20:30:22, rlarocque wrote:
> I'm confused.  Are the diffs from the unrevert leaking into this patch, or is
> this actually new code introduced after patch set 5?

Crap, uploaded without diffing off the revert. See new patchset.

[rlarocque, 2012-07-09 21:15:51]

I still have some concerns about the test code, but I'll leave it to Fred to
continue that part of the review.  I'm hoping he'll be able to read more deeply
into that code than I did.  

Non-test code LGTM.

[Nicolas Zea, 2012-07-10 17:09:52]

ping Fred?

[Nicolas Zea, 2012-07-11 00:27:26]

Richard, the newest patchset includes the logic to cleanup all partial types to
avoid any migration issues (and I updated the unit test too). PTAL

[akalin, 2012-07-11 02:39:59]

took a look. not too crazy about the FakeSyncManager/SyncManager stuff, although
I can't seem to think of a better solution right now.  Let's talk it over
tomorrow...

[Nicolas Zea, 2012-07-11 22:26:37]

-Fred, since I'm splitting off the SBH test refactor into a separate patch.

Richard, PTAL. I've decided to have the purge be part of the Init call, so that
we can use the success flag instead of having to add _more_ state to SBH to
check that the purge succeeds.

[rlarocque, 2012-07-11 22:44:42]

That's much better!

LGTM with comment.

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

http://codereview.chromium.org/10701085/diff/39004/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:407: syncer::ModelTypeSet
GetTypesWithEmptyProgressMarkerToken(
Does this need to be public?  Does it need to be exposed as part of
SyncManager's interface, or could it exist on SyncInternal only?

[Nicolas Zea, 2012-07-11 23:04:16]

Landing once bots go green.

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

http://codereview.chromium.org/10701085/diff/39004/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:407: syncer::ModelTypeSet
GetTypesWithEmptyProgressMarkerToken(
On 2012/07/11 22:44:42, rlarocque wrote:
> Does this need to be public?  Does it need to be exposed as part of
> SyncManager's interface, or could it exist on SyncInternal only?

It's primarily public for testing. Both the SyncManagerTest, and the soon to be
upgraded SyncBackendHostTests will make use of it. That, and I'd rather have it
public than have to friend random tests everytime something wants to check the
directory state. The same applies to PurgePartiallySyncedTypes.

[commit-bot: I haz the power, 2012-07-11 23:45:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10701085/7030

[commit-bot: I haz the power, 2012-07-12 01:32:34]

Change committed as 146262