[Password Manager] Add federations to sort key on password page

[Password Manager] Add federations to sort key on password page

If federations are not included into sort key, entries with the same origin, username and password, but different federation will be collapsed into one entry. That is not correct.

BUG=604750
Committed: https://crrev.com/7c82fec6686c0ee3b38bb70d86d0f0c1837127c9
Cr-Commit-Position: refs/heads/master@{#390457}

[kolos1, 2016-04-28 09:05:52]

Patchset #1 (id:1) has been deleted

[kolos1, 2016-04-28 09:05:58]

Patchset #1 (id:20001) has been deleted

[kolos1, 2016-04-28 09:10:25]

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

[kolos1, 2016-04-28 09:10:25]

Hi Vaclav,

Please review.

Regards,
Maxim

[vabr (Chromium), 2016-04-28 10:03:11]

Thanks, Maxim, this LGTM.

https://codereview.chromium.org/1927753003/diff/40001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.cc (right):

https://codereview.chromium.org/1927753003/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:121: // from
|duplicates|.
nit: This comment should also explain what is |is_password_exception|. It might
be helpful to note that it should be true for the list of blacklisted passwords,
and false for the list of saved passwords.

https://codereview.chromium.org/1927753003/diff/40001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.cc:195: false /* not
password exception */);
For this line and lines 214, 380, 405 also:

Please write the name of the argument variable in the comment, i.e.,
/* is_password_exception */

When you write "false /* not password_exception */", it is not clear if the
false already includes the "not".

An even better way to make such calls easy to read would be to change the type
of the argument from
bool is_password_exception
to
PasswordEntryType entry_type

with the new type declared as
enum class PasswordEntryType {SAVED, BLACKLISTED};

Then the call would be
RemoveDuplicates(..., PasswordEntryType::SAVED);
and would not require any clarifying comments.

[kolos1, 2016-04-28 14:23:51]

Patchset #2 (id:60001) has been deleted

[kolos1, 2016-04-28 14:26:45]

Thanks for your comments, Vaclav! I created the enum PasswordEntryType. Please
review changes.

Best regards,
Maxim

[vabr (Chromium), 2016-04-28 14:33:11]

Thank you, Maxim.
One more comment, but otherwise LGTM.

Cheers,
Vaclav

https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.h (right):

https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.h:30: enum
PasswordEntryType { SAVED, BLACKLISTED };
Please make this an "enum class", not just enum. That will disallow implicit
conversions to int and also enclose the value names in the PasswordEntryType
namespace.

[kolos1, 2016-04-28 14:37:44]

https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passw...
File chrome/browser/ui/passwords/password_manager_presenter.h (right):

https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passw...
chrome/browser/ui/passwords/password_manager_presenter.h:30: enum
PasswordEntryType { SAVED, BLACKLISTED };
On 2016/04/28 14:33:11, vabr (Chromium) wrote:
> Please make this an "enum class", not just enum. That will disallow implicit
> conversions to int and also enclose the value names in the PasswordEntryType
> namespace.

Done.

[vabr (Chromium), 2016-04-28 14:42:37]

lgtm

[kolos1, 2016-04-28 19:17:25]

The CQ bit was checked by kolos@chromium.org

[commit-bot: I haz the power, 2016-04-28 19:17:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1927753003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1927753003/100001

[commit-bot: I haz the power, 2016-04-28 20:12:28]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:20:32]

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

[commit-bot: I haz the power, 2016-04-30 17:57:06]

Description was changed from

==========
[Password Manager] Add federations to sort key on password page

If federations are not included into sort key, entries with the same origin,
username and password, but different federation will be collapsed into one
entry. That is not correct.

BUG=604750
==========

to

==========
[Password Manager] Add federations to sort key on password page

If federations are not included into sort key, entries with the same origin,
username and password, but different federation will be collapsed into one
entry. That is not correct.

BUG=604750

Committed: https://crrev.com/7c82fec6686c0ee3b38bb70d86d0f0c1837127c9
Cr-Commit-Position: refs/heads/master@{#390457}
==========