Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(67)

Issue 1927753003: [Password Manager] Add federations to sort key on password page (Closed)

Created:
4 years, 7 months ago by kolos1
Modified:
4 years, 7 months ago
Reviewers:
vabr (Chromium)
CC:
chromium-reviews, gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org, mkwst+watchlist-passwords_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Fixes addressed to reviewer comments #

Total comments: 2

Patch Set 3 : "enum" to "enum class" #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -59 lines) Patch
M chrome/browser/ui/passwords/password_manager_presenter.h View 1 2 2 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/passwords/password_manager_presenter.cc View 1 8 chunks +19 lines, -17 lines 0 comments Download
M chrome/browser/ui/passwords/password_manager_presenter_unittest.cc View 1 7 chunks +63 lines, -38 lines 0 comments Download

Messages

Total messages: 15 (6 generated)
kolos1
Hi Vaclav, Please review. Regards, Maxim
4 years, 7 months ago (2016-04-28 09:10:25 UTC) #4
vabr (Chromium)
Thanks, Maxim, this LGTM. https://codereview.chromium.org/1927753003/diff/40001/chrome/browser/ui/passwords/password_manager_presenter.cc File chrome/browser/ui/passwords/password_manager_presenter.cc (right): https://codereview.chromium.org/1927753003/diff/40001/chrome/browser/ui/passwords/password_manager_presenter.cc#newcode121 chrome/browser/ui/passwords/password_manager_presenter.cc:121: // from |duplicates|. nit: This ...
4 years, 7 months ago (2016-04-28 10:03:11 UTC) #5
kolos1
Thanks for your comments, Vaclav! I created the enum PasswordEntryType. Please review changes. Best regards, ...
4 years, 7 months ago (2016-04-28 14:26:45 UTC) #7
vabr (Chromium)
Thank you, Maxim. One more comment, but otherwise LGTM. Cheers, Vaclav https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passwords/password_manager_presenter.h File chrome/browser/ui/passwords/password_manager_presenter.h (right): ...
4 years, 7 months ago (2016-04-28 14:33:11 UTC) #8
kolos1
https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passwords/password_manager_presenter.h File chrome/browser/ui/passwords/password_manager_presenter.h (right): https://codereview.chromium.org/1927753003/diff/80001/chrome/browser/ui/passwords/password_manager_presenter.h#newcode30 chrome/browser/ui/passwords/password_manager_presenter.h:30: enum PasswordEntryType { SAVED, BLACKLISTED }; On 2016/04/28 14:33:11, ...
4 years, 7 months ago (2016-04-28 14:37:44 UTC) #9
vabr (Chromium)
lgtm
4 years, 7 months ago (2016-04-28 14:42:37 UTC) #10
commit-bot: I haz the power
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
4 years, 7 months ago (2016-04-28 19:17:49 UTC) #12
commit-bot: I haz the power
Committed patchset #3 (id:100001)
4 years, 7 months ago (2016-04-28 20:12:28 UTC) #13
commit-bot: I haz the power
4 years, 7 months ago (2016-04-30 17:20:32 UTC) #14
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/7c82fec6686c0ee3b38bb70d86d0f0c1837127c9
Cr-Commit-Position: refs/heads/master@{#390457}

Powered by Google App Engine
This is Rietveld 408576698