Sync new PasswordForm fields.

Sync new PasswordForm fields.

They are |display_name|, |avatar_url| and |federation_url|.

BUG=400674
TBR=isherman@chromium.org

Committed: https://crrev.com/dd57b1ab79066e73ff4624c61f594365d63656eb
Cr-Commit-Position: refs/heads/master@{#293489}

[vasilii, 2014-09-02 12:45:27]

vasilii@chromium.org changed reviewers:
+ gcasto@chromium.org, mkwst@chromium.org

[vasilii, 2014-09-02 12:45:27]

vasilii@chromium.org changed reviewers:
+ gcasto@chromium.org, mkwst@chromium.org

[vasilii, 2014-09-02 12:46:38]

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

[vasilii, 2014-09-02 12:47:16]

Hi guys,

please review.

[Mike West, 2014-09-02 12:58:48]

LGTM with one nit.

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

https://codereview.chromium.org/520583003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service_unittest.cc:467:
form1->times_used = 5;
Why did this change?

Nit: Since it's used a few times, perhaps it would make sense to change this
magic number to a constant?

[vasilii, 2014-09-02 14:56:22]

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

https://codereview.chromium.org/520583003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service_unittest.cc:467:
form1->times_used = 5;
On 2014/09/02 12:58:48, Mike West wrote:
> Why did this change?
> 
> Nit: Since it's used a few times, perhaps it would make sense to change this
> magic number to a constant?

The test had passed just because this constant was equal to the one in
CreateSyncData(). I changed the test to be correct. So we need another magic
number here to be sure.

[Mike West, 2014-09-02 15:27:25]

As discussed, at some point in the future it might make sense to extract these
variables. For the moment, this change fixes a bug in the test that was masked
by the previous value, so I'm perfectly happy for you to land it.

Thanks!

[Nicolas Zea, 2014-09-02 17:06:16]

LGTM with one question: are these fields already populated for existing
passwords? Will this cause all passwords to be recommitted with these fields
filled in on upgrade?

[Garrett Casto, 2014-09-03 06:54:19]

Do we also want to sync |is_zero_click|? Otherwise looks good.

[vasilii, 2014-09-03 08:36:21]

The fields aren't populated at the moment. I think they shouldn't be populated
for existing forms because the new API has nothing to do with form parsing.
|is_zero_click| isn't synced by design.

[Garrett Casto, 2014-09-03 20:51:34]

LGTM

If |is_zero_click| is not meant to be synced, do you want to leave a comment
somewhere (either in password_syncable_service or pasword_form.h) about why this
is? I can imagine someone reading this code a few months from now and wondering
if it's an oversight.

[vasilii, 2014-09-04 08:27:14]

On 2014/09/03 20:51:34, Garrett Casto wrote:
> LGTM
> 
> If |is_zero_click| is not meant to be synced, do you want to leave a comment
> somewhere (either in password_syncable_service or pasword_form.h) about why
this
> is? I can imagine someone reading this code a few months from now and
wondering
> if it's an oversight.

Done.

[vasilii, 2014-09-04 08:27:39]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-04 08:28:47]

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

[commit-bot: I haz the power, 2014-09-04 08:35:43]

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

[commit-bot: I haz the power, 2014-09-04 08:35:44]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[vasilii, 2014-09-04 09:48:14]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-04 09:49:13]

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

[commit-bot: I haz the power, 2014-09-04 10:00:23]

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

[commit-bot: I haz the power, 2014-09-04 10:00:25]

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

[vasilii, 2014-09-05 09:15:04]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-05 09:15:23]

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

[commit-bot: I haz the power, 2014-09-05 09:27:01]

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

[commit-bot: I haz the power, 2014-09-05 09:27:03]

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

[vasilii, 2014-09-05 09:41:25]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-09-05 09:42:12]

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

[commit-bot: I haz the power, 2014-09-05 10:14:30]

Committed patchset #4 (id:60001) as 2ccefea61c41ec6134cba4b0873ac07d2760b25a

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

Patchset 4 (id:??) landed as
https://crrev.com/dd57b1ab79066e73ff4624c61f594365d63656eb
Cr-Commit-Position: refs/heads/master@{#293489}