PasswordSyncableService shouldn't push any sync changes if MergeDataAndStartSyncing() failed.

PasswordSyncableService shouldn't push any sync changes if MergeDataAndStartSyncing() failed.

BUG=411521
Committed: https://crrev.com/7ae486083e4fafc0f774efae16c1d326973798f4
Cr-Commit-Position: refs/heads/master@{#294152}

[vasilii, 2014-09-08 09:27:11]

vasilii@chromium.org changed reviewers:
+ isherman@chromium.org, zea@chromium.org

[vasilii, 2014-09-08 09:27:11]

Hi guys,

please review.

[Nicolas Zea, 2014-09-08 18:01:37]

LGTM from a sync perspective.

https://codereview.chromium.org/552713002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/552713002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:151:
sync_error_factory_ = sync_error_factory.Pass();
nit: comment why we set these here, rather than above?

[vasilii, 2014-09-09 17:01:29]

https://codereview.chromium.org/552713002/diff/1/components/password_manager/...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/552713002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:151:
sync_error_factory_ = sync_error_factory.Pass();
On 2014/09/08 18:01:37, Nicolas Zea wrote:
> nit: comment why we set these here, rather than above?

Done.

[Ilya Sherman, 2014-09-09 22:33:17]

LGTM % nits.

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
File
components/password_manager/core/browser/password_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:483:
// Start syncing with an error. Consecutive password store updates shouldn't be
nit: "Consecutive" -> "Subsequent"

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:492:
EXPECT_CALL(*processor_, ProcessSyncChanges(_, _)).Times(0);
I assume this is meant to correspond to the call to ActOnPasswordStoreChanges()
below, correct?  If so, please move it down to be just before that call.  As
written, it's not clear (when skimming) what the lower block of code is doing --
it looks like it is just a bunch of code that couldn't possibly affect the test
result.

[vasilii, 2014-09-10 08:32:18]

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
File
components/password_manager/core/browser/password_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:483:
// Start syncing with an error. Consecutive password store updates shouldn't be
On 2014/09/09 22:33:17, Ilya Sherman wrote:
> nit: "Consecutive" -> "Subsequent"

Done.

https://codereview.chromium.org/552713002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:492:
EXPECT_CALL(*processor_, ProcessSyncChanges(_, _)).Times(0);
On 2014/09/09 22:33:17, Ilya Sherman wrote:
> I assume this is meant to correspond to the call to
ActOnPasswordStoreChanges()
> below, correct?  If so, please move it down to be just before that call.  As
> written, it's not clear (when skimming) what the lower block of code is doing
--
> it looks like it is just a bunch of code that couldn't possibly affect the
test
> result.

You are right but I cannot fix it. The service takes ownership over
|processor_|. In the current implementation the object is destroyed after
MergeDataAndStartSyncing(). Therefore, there is nothing I can set the
expectation for. Instead I added a comment.

[vasilii, 2014-09-10 08:32:33]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-10 08:33:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/552713002/40001

[commit-bot: I haz the power, 2014-09-10 08:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 08:43:07]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[vasilii, 2014-09-10 08:59:09]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-10 08:59:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/552713002/40001

[commit-bot: I haz the power, 2014-09-10 10:03:19]

Committed patchset #3 (id:40001) as 0210184216adc97700d6fd4d5aeeeaf9cb556ea0

[commit-bot: I haz the power, 2014-09-10 10:56:22]

Patchset 3 (id:??) landed as
https://crrev.com/7ae486083e4fafc0f774efae16c1d326973798f4
Cr-Commit-Position: refs/heads/master@{#294152}