Filter duplicates in the account chooser.

Filter duplicates in the account chooser.

https://codereview.chromium.org/2110993003 implemented the filtering for empty usernames.

BUG=400674
Committed: https://crrev.com/cb207921aefb769585e8c4ddb47efcbb92f21a02
Cr-Commit-Position: refs/heads/master@{#414353}

[vasilii, 2016-08-24 15:46:15]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 15:46:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2016-08-24 15:46:47]

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

[vasilii, 2016-08-24 15:46:48]

Hi Vaclav,

please review.

[vabr (Chromium), 2016-08-24 15:56:18]

LGTM with some comments.
Thanks!
Vaclav

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:706:
duplicate.username_element = base::ASCIIToUTF16("username_element");
nit: If this is meant to be different from form_.username_element, it would be
good to point that out. You can change it to "different_username_element",
assign explicitly another string to form_, or write an ASSERT_NE.

(Also below.)

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:737:
.Times(testing::Exactly(1));
optional: This line is not necessary (omitting is equivalent), but if you want
it to emphasise that this should be just once, I'm fine with leaving it as is.

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

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:47:
if (!form1.preferred && form2.preferred)
Should this be the other way round? (I.e. preferred = better ?) If so, could you
write a test for that as well?

[vasilii, 2016-08-24 16:51:15]

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
File
components/password_manager/content/browser/credential_manager_impl_unittest.cc
(right):

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:706:
duplicate.username_element = base::ASCIIToUTF16("username_element");
On 2016/08/24 15:56:17, vabr (Chromium) wrote:
> nit: If this is meant to be different from form_.username_element, it would be
> good to point that out. You can change it to "different_username_element",
> assign explicitly another string to form_, or write an ASSERT_NE.
> 
> (Also below.)

Done.

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/content/browser/credential_manager_impl_unittest.cc:737:
.Times(testing::Exactly(1));
On 2016/08/24 15:56:17, vabr (Chromium) wrote:
> optional: This line is not necessary (omitting is equivalent), but if you want
> it to emphasise that this should be just once, I'm fine with leaving it as is.

It was copy-paste.

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

https://codereview.chromium.org/2277793002/diff/1/components/password_manager...
components/password_manager/core/browser/credential_manager_pending_request_task.cc:47:
if (!form1.preferred && form2.preferred)
On 2016/08/24 15:56:17, vabr (Chromium) wrote:
> Should this be the other way round? (I.e. preferred = better ?) If so, could
you
> write a test for that as well?

Done.

[vasilii, 2016-08-24 16:54:38]

The CQ bit was checked by vasilii@chromium.org

[vasilii, 2016-08-24 16:54:39]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/2277793002/#ps40001
(title: "missed one")

[commit-bot: I haz the power, 2016-08-24 16:55:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vabr (Chromium), 2016-08-24 16:56:53]

lgtm

[commit-bot: I haz the power, 2016-08-24 19:49:52]

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

[commit-bot: I haz the power, 2016-08-24 19:49:54]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[vasilii, 2016-08-25 07:21:01]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2016-08-25 07:21:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 07:56:12]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-25 07:58:41]

Description was changed from

==========
Filter duplicates in the account chooser.

https://codereview.chromium.org/2110993003 implemented the filtering for empty
usernames.

BUG=400674
==========

to

==========
Filter duplicates in the account chooser.

https://codereview.chromium.org/2110993003 implemented the filtering for empty
usernames.

BUG=400674

Committed: https://crrev.com/cb207921aefb769585e8c4ddb47efcbb92f21a02
Cr-Commit-Position: refs/heads/master@{#414353}
==========

[commit-bot: I haz the power, 2016-08-25 07:58:42]

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