Password sync refactoring: implemented |MergeDataAndStartSyncing| and unit_tests

The patch is based on https://codereview.chromium.org/27233003/. The unit tests were modified, one has been added.

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

[vasilii, 2014-01-28 15:52:57]

Hi guys,

Tim: I added you as an owner. Ilya is on vacation. Will you have time to review
this?

Nicolas: You reviewed the original. Can you review that one too?

[Nicolas Zea, 2014-01-31 07:56:00]

mostly LG

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_syncable_service_unittest.cc:184:
.WillRepeatedly(Invoke(this, &PasswordStoreDataVerifier::VerifyUpdate));
Same comment as below (here and elsewhere). If possible, I prefer to avoid mock
methods, and just have them automatically invoke some checker method. So in this
case just invoke VerifyUpdate, which itself will know whether to expect anything
or not.

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_syncable_service_unittest.cc:192:
EXPECT_CALL(*sync_processor, ProcessSyncChanges(_,_))
Why not make the sync processor's ProcessSyncChanges always invoke the
PasswordStoreDataVerifier::TestSyncChanges, that way you don't need to the mock
logic?

[vasilii, 2014-01-31 13:23:22]

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/password_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_syncable_service_unittest.cc:184:
.WillRepeatedly(Invoke(this, &PasswordStoreDataVerifier::VerifyUpdate));
On 2014/01/31 07:56:01, Nicolas Zea wrote:
> Same comment as below (here and elsewhere). If possible, I prefer to avoid
mock
> methods, and just have them automatically invoke some checker method. So in
this
> case just invoke VerifyUpdate, which itself will know whether to expect
anything
> or not.

UpdateLoginImpl is a mock method from MockPasswordStore class that isn't mine.

https://codereview.chromium.org/131003006/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/password_syncable_service_unittest.cc:192:
EXPECT_CALL(*sync_processor, ProcessSyncChanges(_,_))
On 2014/01/31 07:56:01, Nicolas Zea wrote:
> Why not make the sync processor's ProcessSyncChanges always invoke the
> PasswordStoreDataVerifier::TestSyncChanges, that way you don't need to the
mock
> logic?

Done.

[Nicolas Zea, 2014-01-31 13:30:21]

lgtm

[vasilii, 2014-02-03 10:41:06]

Ilya, can you review the patch? You reviewed the original.

[vasilii, 2014-02-04 09:07:54]

Ilya, Tim: friendly ping

[tim (not reviewing), 2014-02-05 14:31:23]

LGTM

[vasilii, 2014-02-05 14:36:47]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-02-05 14:42:55]

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

[commit-bot: I haz the power, 2014-02-05 15:15:36]

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

[commit-bot: I haz the power, 2014-02-05 15:15:37]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[vasilii, 2014-02-05 17:38:12]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-02-05 17:43:29]

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

[commit-bot: I haz the power, 2014-02-05 20:09:18]

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

[commit-bot: I haz the power, 2014-02-05 20:09:18]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[vasilii, 2014-02-06 08:48:46]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-02-06 08:51:53]

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

[commit-bot: I haz the power, 2014-02-06 08:51:55]

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

[commit-bot: I haz the power, 2014-02-06 08:51:56]

Failed to apply patch for chrome/chrome_tests_unit.gypi:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/chrome_tests_unit.gypi
  Hunk #1 FAILED at 1103.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_tests_unit.gypi.rej

Patch:       chrome/chrome_tests_unit.gypi
Index: chrome/chrome_tests_unit.gypi
diff --git a/chrome/chrome_tests_unit.gypi b/chrome/chrome_tests_unit.gypi
index
6f05e3750b5b4c6f252f23c5c23452b6832248a9..6ba31277df231443fc5550d688b671bc346aeb4b
100644
--- a/chrome/chrome_tests_unit.gypi
+++ b/chrome/chrome_tests_unit.gypi
@@ -1103,6 +1103,7 @@
         'browser/password_manager/password_store_mac_unittest.cc',
         'browser/password_manager/password_store_win_unittest.cc',
         'browser/password_manager/password_store_x_unittest.cc',
+        'browser/password_manager/password_syncable_service_unittest.cc',
         'browser/password_manager/psl_matching_helper_unittest.cc',
         'browser/performance_monitor/database_unittest.cc',
         'browser/plugins/plugin_finder_unittest.cc',

[vasilii, 2014-02-06 09:17:00]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-02-06 09:21:24]

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

[commit-bot: I haz the power, 2014-02-06 09:59:56]

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

[commit-bot: I haz the power, 2014-02-06 09:59:57]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[vasilii, 2014-02-06 13:06:19]

Nicolas, can you review again as I did a non-trivial merge.

[Nicolas Zea, 2014-02-06 20:45:15]

lgtm

[Ilya Sherman, 2014-02-07 01:59:24]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2014-02-07 02:11:40]

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

[commit-bot: I haz the power, 2014-02-07 08:09:20]

Change committed as 249615