Fix an incorrect DCHECK caused by incorrect stop behavior.

When StartFailed for a datatype the fix is to call StopAssociation which would call DisassociateModel and clear off the change processor and model associator. 
When stop is called later on the same datatype we dont call StopAssociationAsync any more (which would be implemented in the datatypes' sub class) if the datatype never started succesfully. (The assertion was in this code because we were calling StopAssociationAsync on a datatype that failed to start).


BUG=100057
TEST=

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

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

[lipalani1, 2011-10-14 21:08:36]

Whoever is free please review.

[Nicolas Zea, 2011-10-14 21:13:50]

http://codereview.chromium.org/8305006/diff/1/chrome/browser/sync/glue/non_fr...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/8305006/diff/1/chrome/browser/sync/glue/non_fr...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:186: if (state_ ==
ASSOCIATING) {
I think it would be cleaner to have a switch statement here that covers all the
cases. See the implementation in new_non_frontend_data_type_controller.cc (you
may have to rebase to get it)

[lipalani1, 2011-10-14 22:02:49]

PTAL.

http://codereview.chromium.org/8305006/diff/1/chrome/browser/sync/glue/non_fr...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/8305006/diff/1/chrome/browser/sync/glue/non_fr...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:186: if (state_ ==
ASSOCIATING) {
Since it was not part of this CR I did not touch it initially. However since I
am in the general area of the code I cleaned it up :)
On 2011/10/14 21:13:50, nzea wrote:
> I think it would be cleaner to have a switch statement here that covers all
the
> cases. See the implementation in new_non_frontend_data_type_controller.cc (you
> may have to rebase to get it)

Done.

[Nicolas Zea, 2011-10-14 22:15:12]

LGTM with one change.

http://codereview.chromium.org/8305006/diff/4002/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/8305006/diff/4002/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:207: state_ =
STOPPING;
I think you can add a state here for DISABLED that calls StopModels and just
returns. (if we failed association, we wouldn't have activated the datatype).
Then, in the default state, you can DCHECK_EQ(state_, RUNNING) and get rid of
the if statement below.

[Nicolas Zea, 2011-10-15 00:13:37]

LGTM with nits

http://codereview.chromium.org/8305006/diff/4003/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller.cc (right):

http://codereview.chromium.org/8305006/diff/4003/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:207: state_ =
NOT_RUNNING;
Add comment that DISABLED state means we failed during association and hence
never activated, therefore we can return after stopping models.

http://codereview.chromium.org/8305006/diff/4003/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller.cc:210: default:
add DCHECK_EQ(state_, RUNNING) so that if we add another state we don't
accidentally fall into the default state again without considering it.

http://codereview.chromium.org/8305006/diff/4003/chrome/browser/sync/glue/non...
File chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/8305006/diff/4003/chrome/browser/sync/glue/non...
chrome/browser/sync/glue/non_frontend_data_type_controller_unittest.cc:195:
scoped_ptr<StrictMock<ProfileSyncFactoryMock>> profile_sync_factory_;
don't need to have this be a strictmock, just where you actually instantiate it.

[commit-bot: I haz the power, 2011-10-15 03:33:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8305006/6002

[commit-bot: I haz the power, 2011-10-15 08:06:01]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 105651, job name
was 8305006-6002 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-10-15 08:29:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lipalani@chromium.org/8305006/6002

[commit-bot: I haz the power, 2011-10-15 11:51:21]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 105656, job name
was 8305006-6002 (retry) (retry) (retry) (retry).