[Sync] - Implement isolated model association.

Implement isolated model association. if a type takes more than 2 minutes to load we move on to the next type.

When the type eventually does load we could be in one of 2 states:
1. Configuring - if we are currently configuring we will associate this type the next chance we get.
2. IDLE - we will initialize a configuration cycle and start associating this type.

BUG=
TEST=

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

[lipalani1, 2012-05-18 00:07:13]

Ping!

[tim (not reviewing), 2012-05-21 23:18:57]

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:76: // cycle.
Wording is a bit unclear, perhaps "List of types that failed to start
association within our allotted time period (see kAssociationDeadline [or
whatever]).  We move forward here and allow these types to continue loading, but
association will not start until another configure cycle begins [what kicks this
off]?

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:91: DCHECK(state_ ==
IDLE);
DCHECK_EQ

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:338: // If the type that
got loaded was the type that was started
'If the type that just finished loading was the last type to start, we can
safely stop the timer.'

is that what you're trying to say?  And this is true because the types run
serially and not in parallel -- I'd mention that as a final sentence on your
comment.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:359: // We got a callback
on type that we started loading during the
"This data type finished loading after the deadline imposed by the originating
configuration cycle.  Inform the DataTypeManager that the type has loaded, so
that association may begin."

- don't use DTM / abbreviations in comments, helps for grep etc.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:414: void
ModelAssociationManager::ModelLoadTimedOut() {
Is this method actually needed?

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:429: void
ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {
I think there is a way to do this that doesn't require adding test only
functionality like this and the model_association_manager() accessor, explained
below.  Note otherwise however the style guide stipulates that this method and
the accessor should be suffixed with "ForTesting", to ensure compatibility with
presubmit script.

One better option to remove the accessor by injecting ModelAssociationManager
into the DataTypeManager. From there, I would suggest one of the following

- Have ModelAssociationManager take a Timer on construction, since this is what
you ultimately want control over in your test (common enough). You can use
timer.user_task and timer.stop to get what you want.
- Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
inject from your test.

Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one test
needs this special behavior, but this is less ideal (especially given that you
have to dig through  a class to get here! I think injection is the right
answer.)

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.h (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:42: // has been called back
from this class to the caller.
I still don't really grok this comment.  "OnModelAssociationDone has been called
back from this class to the caller".  I think it should say 'after the
ModelAssociationManager has invoked OnModelAssociationDone on the
ModelAssociationResultProcessor.'

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1440:
DCHECK(result->failed_data_types.size() == 1);
DCHECK_EQ

[lipalani1, 2012-05-22 01:23:58]

please review.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/da...
File chrome/browser/sync/glue/data_type_manager.h (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/da...
chrome/browser/sync/glue/data_type_manager.h:76: // cycle.
On 2012/05/21 23:18:57, timsteele wrote:
> Wording is a bit unclear, perhaps "List of types that failed to start
> association within our allotted time period (see kAssociationDeadline [or
> whatever]).  We move forward here and allow these types to continue loading,
but
> association will not start until another configure cycle begins [what kicks
this
> off]?

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:91: DCHECK(state_ ==
IDLE);
On 2012/05/21 23:18:57, timsteele wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:338: // If the type that
got loaded was the type that was started
On 2012/05/21 23:18:57, timsteele wrote:
> 'If the type that just finished loading was the last type to start, we can
> safely stop the timer.'
> 
> is that what you're trying to say?  And this is true because the types run
> serially and not in parallel -- I'd mention that as a final sentence on your
> comment.

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:359: // We got a callback
on type that we started loading during the
On 2012/05/21 23:18:57, timsteele wrote:
> "This data type finished loading after the deadline imposed by the originating
> configuration cycle.  Inform the DataTypeManager that the type has loaded, so
> that association may begin."
> 
> - don't use DTM / abbreviations in comments, helps for grep etc.

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:414: void
ModelAssociationManager::ModelLoadTimedOut() {
Just for clarity purpose... Compiler probably optimizes this any way.
On 2012/05/21 23:18:57, timsteele wrote:
> Is this method actually needed?

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:429: void
ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {
I think the eventual direction you are advocating is moving towards dependancy
injection. Which I think is cool and all of our code should adhere to.

We will have to change few components including PSS to inject the MAM class into
DTM. (and this is true of most components we have). I think modifying those
components is better as a different change list. and should be done with DI as
the goal. 

The other option is to make the timer injectable from DTM. But the problem with
that is test cases in DTM would still need an accessor.

For now I have taken the suggestion of user_task_ and created a test only
accessor for timer. (The reason this is better is: Accessors  gets us half way
to DI. Instead of injecting we are extracting. However the code that manipulates
the object that is being extracted(or injected) would remain the same even after
the move to DI. Another advantage of this is that we are only using the public
methods of the object extracted(injected) which is slightly preferable than
writing derived classes.)
On 2012/05/21 23:18:57, timsteele wrote:
> I think there is a way to do this that doesn't require adding test only
> functionality like this and the model_association_manager() accessor,
explained
> below.  Note otherwise however the style guide stipulates that this method and
> the accessor should be suffixed with "ForTesting", to ensure compatibility
with
> presubmit script.
> 
> One better option to remove the accessor by injecting ModelAssociationManager
> into the DataTypeManager. From there, I would suggest one of the following
> 
> - Have ModelAssociationManager take a Timer on construction, since this is
what
> you ultimately want control over in your test (common enough). You can use
> timer.user_task and timer.stop to get what you want.
> - Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
> inject from your test.
> 
> Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one test
> needs this special behavior, but this is less ideal (especially given that you
> have to dig through  a class to get here! I think injection is the right
> answer.)

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.h (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:42: // has been called back
from this class to the caller.
On 2012/05/21 23:18:57, timsteele wrote:
> I still don't really grok this comment.  "OnModelAssociationDone has been
called
> back from this class to the caller".  I think it should say 'after the
> ModelAssociationManager has invoked OnModelAssociationDone on the
> ModelAssociationResultProcessor.'

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_service.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_service.cc:1440:
DCHECK(result->failed_data_types.size() == 1);
On 2012/05/21 23:18:57, timsteele wrote:
> DCHECK_EQ

Done.

[tim (not reviewing), 2012-05-22 16:50:05]

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:414: void
ModelAssociationManager::ModelLoadTimedOut() {
On 2012/05/22 01:23:58, lipalani1 wrote:
> Just for clarity purpose... Compiler probably optimizes this any way.
> On 2012/05/21 23:18:57, timsteele wrote:
> > Is this method actually needed?
> 

Hmm, to me the extra indirection makes it less clear.  It's also easier for
someone to sneak in code that does something different in the timeout case, and
I'd like that to be a Big Event / called out in a future code review.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:429: void
ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {
On 2012/05/22 01:23:58, lipalani1 wrote:
> I think the eventual direction you are advocating is moving towards dependancy
> injection. Which I think is cool and all of our code should adhere to.
> 
> We will have to change few components including PSS to inject the MAM class
into
> DTM. (and this is true of most components we have). I think modifying those
> components is better as a different change list. and should be done with DI as
> the goal. 
> 
> The other option is to make the timer injectable from DTM. But the problem
with
> that is test cases in DTM would still need an accessor.
> 
> For now I have taken the suggestion of user_task_ and created a test only
> accessor for timer. (The reason this is better is: Accessors  gets us half way
> to DI. Instead of injecting we are extracting. However the code that
manipulates
> the object that is being extracted(or injected) would remain the same even
after
> the move to DI. Another advantage of this is that we are only using the public
> methods of the object extracted(injected) which is slightly preferable than
> writing derived classes.)
> On 2012/05/21 23:18:57, timsteele wrote:
> > I think there is a way to do this that doesn't require adding test only
> > functionality like this and the model_association_manager() accessor,
> explained
> > below.  Note otherwise however the style guide stipulates that this method
and
> > the accessor should be suffixed with "ForTesting", to ensure compatibility
> with
> > presubmit script.
> > 
> > One better option to remove the accessor by injecting
ModelAssociationManager
> > into the DataTypeManager. From there, I would suggest one of the following
> > 
> > - Have ModelAssociationManager take a Timer on construction, since this is
> what
> > you ultimately want control over in your test (common enough). You can use
> > timer.user_task and timer.stop to get what you want.
> > - Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
> > inject from your test.
> > 
> > Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one test
> > needs this special behavior, but this is less ideal (especially given that
you
> > have to dig through  a class to get here! I think injection is the right
> > answer.)
> 

It sounds like we're in agreement that it would be better to inject the
parameters, so I guess if you're not doing it here this new code should be
accompanied by a TODO and a bug on file tracking the fact that it is incomplete.

In the meantime, the accessor for model_association_manager() should be called
GetModelAssociationManagerForTesting().  My main concern is other components
enforcing an unnecessary dependency / requirement that the DataTypeManager
exposes ModelAssociationManager.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:31: const int64
kDataTypeLoadWaitTimeInSeconds = 120;
Comment.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:259: void
ModelAssociationManager::HandleFailedTypes(
Is this called for failures other than associations?  If not
HandleFailedAssociations(...) would be a better name, since it's unclear what
steps "failed types" refers to.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:383: // serially.(Except
when the type loading takes more than the alloted
nit- Space after period, and 'alloted' -> 'allotted'. 

Actually your edit was a case I hadn't thought of. I think this might be clearer
worded as:

"... because the types are loaded serially while the timer is running.  If the
timer expires (because at least one type took longer than
kDataTypeLoadWaitTimeSeconds to load), we can have multiple types loading in
parallel, but they are no longer timed."

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:462: configure_status =
DataTypeManager::PARTIAL_SUCCESS;
Explain why this is only a partial success.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.h (right):

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:33: // when types (that did
not complete loading, during the previous
nit - Comments start with capital letters.

Seeing DataTypeManager in this comment is a bit odd -- I think you mean 'let the
ModelAssociationResultProcessor know...".

editorial suggestion - "Called to let the ModelAssociationResultProcessor know
that "delayed" types have finished loading and association should take place. (A
delayed type here is a type did not finish loading during the previous
configuration cycle.) 

Is this *only* for types that are 'delayed'?  If so, the method name should
incorporate that.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:113: void
HandleFailedTypes(DataTypeController::StartResult result,
Comment.

[lipalani1, 2012-05-22 20:20:46]

PTAL.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:414: void
ModelAssociationManager::ModelLoadTimedOut() {
On 2012/05/22 16:50:05, timsteele wrote:
> On 2012/05/22 01:23:58, lipalani1 wrote:
> > Just for clarity purpose... Compiler probably optimizes this any way.
> > On 2012/05/21 23:18:57, timsteele wrote:
> > > Is this method actually needed?
> > 
> 
> Hmm, to me the extra indirection makes it less clear.  It's also easier for
> someone to sneak in code that does something different in the timeout case,
and
> I'd like that to be a Big Event / called out in a future code review.

Done.

http://codereview.chromium.org/10387144/diff/2001/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:429: void
ModelAssociationManager::TestSimulateDataTypeLoadTimeout() {
I have addressed this. Let me know what you think.
On 2012/05/22 16:50:05, timsteele wrote:
> On 2012/05/22 01:23:58, lipalani1 wrote:
> > I think the eventual direction you are advocating is moving towards
dependancy
> > injection. Which I think is cool and all of our code should adhere to.
> > 
> > We will have to change few components including PSS to inject the MAM class
> into
> > DTM. (and this is true of most components we have). I think modifying those
> > components is better as a different change list. and should be done with DI
as
> > the goal. 
> > 
> > The other option is to make the timer injectable from DTM. But the problem
> with
> > that is test cases in DTM would still need an accessor.
> > 
> > For now I have taken the suggestion of user_task_ and created a test only
> > accessor for timer. (The reason this is better is: Accessors  gets us half
way
> > to DI. Instead of injecting we are extracting. However the code that
> manipulates
> > the object that is being extracted(or injected) would remain the same even
> after
> > the move to DI. Another advantage of this is that we are only using the
public
> > methods of the object extracted(injected) which is slightly preferable than
> > writing derived classes.)
> > On 2012/05/21 23:18:57, timsteele wrote:
> > > I think there is a way to do this that doesn't require adding test only
> > > functionality like this and the model_association_manager() accessor,
> > explained
> > > below.  Note otherwise however the style guide stipulates that this method
> and
> > > the accessor should be suffixed with "ForTesting", to ensure compatibility
> > with
> > > presubmit script.
> > > 
> > > One better option to remove the accessor by injecting
> ModelAssociationManager
> > > into the DataTypeManager. From there, I would suggest one of the following
> > > 
> > > - Have ModelAssociationManager take a Timer on construction, since this is
> > what
> > > you ultimately want control over in your test (common enough). You can use
> > > timer.user_task and timer.stop to get what you want.
> > > - Stick SimulateDataTypeLoadTimeout on a test-only subclass that you would
> > > inject from your test.
> > > 
> > > Another option is to use FRIEND_TEST_ALL_PREFIXES.  It appears only one
test
> > > needs this special behavior, but this is less ideal (especially given that
> you
> > > have to dig through  a class to get here! I think injection is the right
> > > answer.)
> > 
> 
> It sounds like we're in agreement that it would be better to inject the
> parameters, so I guess if you're not doing it here this new code should be
> accompanied by a TODO and a bug on file tracking the fact that it is
incomplete.
> 
> In the meantime, the accessor for model_association_manager() should be called
> GetModelAssociationManagerForTesting().  My main concern is other components
> enforcing an unnecessary dependency / requirement that the DataTypeManager
> exposes ModelAssociationManager.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:31: const int64
kDataTypeLoadWaitTimeInSeconds = 120;
On 2012/05/22 16:50:05, timsteele wrote:
> Comment. 

Done.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:259: void
ModelAssociationManager::HandleFailedTypes(
It is refactored so it could be called to handle load failure and association
failure. I dont see any reason for differentiating them. The SyncError struct
would have the detailed information which gets displayed in about:sync(that code
is already there).
On 2012/05/22 16:50:05, timsteele wrote:
> Is this called for failures other than associations?  If not
> HandleFailedAssociations(...) would be a better name, since it's unclear what
> steps "failed types" refers to.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:383: // serially.(Except
when the type loading takes more than the alloted
I think I have reworded this now. Please take a look. I want to make sure you
are not confused by this. As this timer is relatively insignificant.
On 2012/05/22 16:50:05, timsteele wrote:
> nit- Space after period, and 'alloted' -> 'allotted'. 
> 
> Actually your edit was a case I hadn't thought of. I think this might be
clearer
> worded as:
> 
> "... because the types are loaded serially while the timer is running.  If the
> timer expires (because at least one type took longer than
> kDataTypeLoadWaitTimeSeconds to load), we can have multiple types loading in
> parallel, but they are no longer timed."

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:462: configure_status =
DataTypeManager::PARTIAL_SUCCESS;
On 2012/05/22 16:50:05, timsteele wrote:
> Explain why this is only a partial success.

Done.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.h (right):

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:33: // when types (that did
not complete loading, during the previous
On 2012/05/22 16:50:05, timsteele wrote:
> nit - Comments start with capital letters.
> 
> Seeing DataTypeManager in this comment is a bit odd -- I think you mean 'let
the
> ModelAssociationResultProcessor know...".
> 
> editorial suggestion - "Called to let the ModelAssociationResultProcessor know
> that "delayed" types have finished loading and association should take place.
(A
> delayed type here is a type did not finish loading during the previous
> configuration cycle.) 
> 
> Is this *only* for types that are 'delayed'?  If so, the method name should
> incorporate that.

Done.

http://codereview.chromium.org/10387144/diff/8003/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.h:113: void
HandleFailedTypes(DataTypeController::StartResult result,
On 2012/05/22 16:50:05, timsteele wrote:
> Comment.

Done.

[tim (not reviewing), 2012-05-23 03:18:52]

Few more comments, almost there!

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fa...
File chrome/browser/sync/glue/fake_data_type_controller.h (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fa...
chrome/browser/sync/glue/fake_data_type_controller.h:60: virtual void
FinishModelLoad();
Call this 'SimulateModelLoadFinishing' or something, helps to differentiate it
in the test case from real vs fake code.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fr...
File chrome/browser/sync/glue/frontend_data_type_controller.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fr...
chrome/browser/sync/glue/frontend_data_type_controller.cc:92: 
extra newlines

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:128: 
extra newline

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:175: VLOG(0) <<
"ModelAssociationManager: Stopping disabled types.";
DVLOG(1) (here and elsewhere)?

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:263: void
ModelAssociationManager::HandleFailedTypes(
Actually, since this doesn't totally "handle" failed types (I found myself
wondering what handled the failed_datatypes_info_ after this was called and how
callsites would recover / handle things after HandleFailedTypes returned.  

Maybe AppendToFailedDatatypesAndLogError is more clear.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager_unittest.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:242: 
extra newline

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:252:
model_association_manager.GetTimerForTesting();
indent

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:261:
GetController(controllers_, syncable::BOOKMARKS)->FinishModelLoad();
Comment like "Simulate delayed loading of bookmarks."

Also, at first glance it seems like this could be flaky / dependent on the
timeout value in ModelAssociationManager, but it isn't because the timer posts a
task and we never give up control of the message loop.  A comment explaining why
this test is *not* flaky here might go a ways in the future of someone changes
the code and this test starts failing randomly.

[lipalani1, 2012-05-23 17:47:20]

PTAL.

If possible a quick review would be good. I want to check all this in tonight
and give it a couple of days to bake in canary.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fa...
File chrome/browser/sync/glue/fake_data_type_controller.h (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fa...
chrome/browser/sync/glue/fake_data_type_controller.h:60: virtual void
FinishModelLoad();
On 2012/05/23 03:18:52, timsteele wrote:
> Call this 'SimulateModelLoadFinishing' or something, helps to differentiate it
> in the test case from real vs fake code.

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fr...
File chrome/browser/sync/glue/frontend_data_type_controller.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/fr...
chrome/browser/sync/glue/frontend_data_type_controller.cc:92: 
On 2012/05/23 03:18:52, timsteele wrote:
> extra newlines

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:128: 
On 2012/05/23 03:18:52, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:175: VLOG(0) <<
"ModelAssociationManager: Stopping disabled types.";
On 2012/05/23 03:18:52, timsteele wrote:
> DVLOG(1) (here and elsewhere)?

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager.cc:263: void
ModelAssociationManager::HandleFailedTypes(
On 2012/05/23 03:18:52, timsteele wrote:
> Actually, since this doesn't totally "handle" failed types (I found myself
> wondering what handled the failed_datatypes_info_ after this was called and
how
> callsites would recover / handle things after HandleFailedTypes returned.  
> 
> Maybe AppendToFailedDatatypesAndLogError is more clear.

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
File chrome/browser/sync/glue/model_association_manager_unittest.cc (right):

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:242: 
On 2012/05/23 03:18:52, timsteele wrote:
> extra newline

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:252:
model_association_manager.GetTimerForTesting();
On 2012/05/23 03:18:52, timsteele wrote:
> indent

Done.

http://codereview.chromium.org/10387144/diff/8005/chrome/browser/sync/glue/mo...
chrome/browser/sync/glue/model_association_manager_unittest.cc:261:
GetController(controllers_, syncable::BOOKMARKS)->FinishModelLoad();
On 2012/05/23 03:18:52, timsteele wrote:
> Comment like "Simulate delayed loading of bookmarks."
> 
> Also, at first glance it seems like this could be flaky / dependent on the
> timeout value in ModelAssociationManager, but it isn't because the timer posts
a
> task and we never give up control of the message loop.  A comment explaining
why
> this test is *not* flaky here might go a ways in the future of someone changes
> the code and this test starts failing randomly.

Done.

[tim (not reviewing), 2012-05-23 21:33:27]

LGTM!

[lipalani, 2012-05-23 21:34:57]

woohoo thanks alot!
Should be present in tomorrow's canary!

On Wed, May 23, 2012 at 2:33 PM, <tim@chromium.org> wrote:

> LGTM!
>
>
http://codereview.chromium.**org/10387144/<http://codereview.chromium.org/103...
>