Add StartSyncFlare to PasswordSyncableService

Add StartSyncFlare to PasswordSyncableService.

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

[vasilii, 2014-03-04 15:56:04]

Hi Ilya,

please review the CL.

[Ilya Sherman, 2014-03-04 22:35:52]

Could you add test coverage for this change?

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.cc:79: bool
PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare) {
Should |flare| be used somewhere in this method?

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_syncable_service.cc:285:
flare_.Reset();
Optional nit: IMO it would be a little clearer to move this line into the nested
if-stmt, but I don't feel strongly about this.

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_syncable_service.cc:287: }
nit: Please add a blank line after this one.

[vasilii, 2014-03-05 12:46:21]

I added the test.

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_store.cc:79: bool
PasswordStore::Init(const syncer::SyncableService::StartSyncFlare& flare) {
On 2014/03/04 22:35:53, Ilya Sherman wrote:
> Should |flare| be used somewhere in this method?

Sure!

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_syncable_service.cc
(right):

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_syncable_service.cc:285:
flare_.Reset();
On 2014/03/04 22:35:53, Ilya Sherman wrote:
> Optional nit: IMO it would be a little clearer to move this line into the
nested
> if-stmt, but I don't feel strongly about this.

Done.

https://codereview.chromium.org/183223003/diff/60001/components/password_mana...
components/password_manager/core/browser/password_syncable_service.cc:287: }
On 2014/03/04 22:35:53, Ilya Sherman wrote:
> nit: Please add a blank line after this one.

Done.

[Ilya Sherman, 2014-03-06 00:04:06]

LGTM, though there's an opportunity to make the test coverage even better.

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store.cc:82:
ScheduleTask(base::Bind(&PasswordStore::InitSyncableService, this, flare));
It would be nice to have a test exercising this code path, i.e. one that would
have caught the bug of forgetting to include the (correct) flare here.

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

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:623:
// No flare and no SyncChangeProcessor, the call shouldn't crash.
nit: It currently looks like this comment is meant to apply to all four lines
below, whereas I think it's really intended to just apply to the first one. 
Please add a separate comment for the remaining lines, or at least split things
up with vertical whitespace, to clarify.

[vasilii, 2014-03-06 12:35:39]

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
File components/password_manager/core/browser/password_store.cc (right):

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
components/password_manager/core/browser/password_store.cc:82:
ScheduleTask(base::Bind(&PasswordStore::InitSyncableService, this, flare));
On 2014/03/06 00:04:07, Ilya Sherman wrote:
> It would be nice to have a test exercising this code path, i.e. one that would
> have caught the bug of forgetting to include the (correct) flare here.

Done.

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

https://codereview.chromium.org/183223003/diff/80001/components/password_mana...
components/password_manager/core/browser/password_syncable_service_unittest.cc:623:
// No flare and no SyncChangeProcessor, the call shouldn't crash.
On 2014/03/06 00:04:07, Ilya Sherman wrote:
> nit: It currently looks like this comment is meant to apply to all four lines
> below, whereas I think it's really intended to just apply to the first one. 
> Please add a separate comment for the remaining lines, or at least split
things
> up with vertical whitespace, to clarify.

Done.

[Ilya Sherman, 2014-03-07 00:06:20]

LGTM, thanks!

[vasilii, 2014-03-07 08:42:04]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-07 08:47:31]

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

[vasilii, 2014-03-07 12:45:40]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-07 12:47:08]

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

[commit-bot: I haz the power, 2014-03-07 16:27:40]

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

[commit-bot: I haz the power, 2014-03-07 16:27:41]

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

[vasilii, 2014-03-07 16:31:12]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:31:55]

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

[commit-bot: I haz the power, 2014-03-07 19:51:16]

Change committed as 255690