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

Issue 2971783002: Skeleton for showing "Show all saved passwords row" for Linux/CrOs/Windows platforms (Closed)

Created:
3 years, 5 months ago by melandory
Modified:
3 years, 4 months ago
CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. TBR=mahmadi@chromium.org BUG=739343 Review-Url: https://codereview.chromium.org/2971783002 Cr-Commit-Position: refs/heads/master@{#490365} Committed: https://chromium.googlesource.com/chromium/src/+/04fc429cf5d4c16316402d43c5435c36d09c480f

Patch Set 1 : more tests #

Total comments: 15

Patch Set 2 : move vector icons include #

Patch Set 3 : fix ios compilation and adress some vasilii@ comments #

Patch Set 4 : more comments adaressed #

Total comments: 16

Patch Set 5 : . #

Total comments: 1

Patch Set 6 : Fix ios compilation #

Total comments: 13

Patch Set 7 : comments #

Total comments: 31

Patch Set 8 : adressed some comments of estade@ #

Patch Set 9 : . #

Patch Set 10 : . #

Total comments: 1

Patch Set 11 : . #

Total comments: 27

Patch Set 12 : remove android guard from autofill_popup_layout_model_unittest #

Patch Set 13 : . #

Total comments: 12

Patch Set 14 : . #

Total comments: 4

Patch Set 15 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+315 lines, -118 lines) Patch
M android_webview/browser/aw_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -2 lines 0 comments Download
M android_webview/browser/aw_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -3 lines 0 comments Download
M chrome/app/vector_icons/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
A chrome/app/vector_icons/open_in_new.icon View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +31 lines, -0 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_layout_model.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -7 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_layout_model.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +27 lines, -27 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -6 lines 0 comments Download
M chrome/browser/ui/autofill/chrome_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/autofill/chrome_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +21 lines, -6 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_view_views.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +10 lines, -7 lines 0 comments Download
M components/autofill/core/browser/autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -7 lines 0 comments Download
M components/autofill/core/browser/autofill_external_delegate.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -5 lines 0 comments Download
M components/autofill/core/browser/autofill_external_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +8 lines, -6 lines 0 comments Download
M components/autofill/core/browser/autofill_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +9 lines, -4 lines 0 comments Download
M components/autofill/core/browser/popup_item_ids.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/core/browser/test_autofill_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -2 lines 0 comments Download
M components/autofill/core/browser/test_autofill_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -3 lines 0 comments Download
M components/autofill_strings.grdp View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 5 chunks +22 lines, -5 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 9 chunks +145 lines, -19 lines 0 comments Download
M components/password_manager/core/common/password_manager_features.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/core/common/password_manager_features.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M ios/chrome/browser/ui/autofill/autofill_client_ios.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -2 lines 0 comments Download
M ios/chrome/browser/ui/autofill/autofill_client_ios.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +1 line, -5 lines 0 comments Download

Messages

Total messages: 192 (155 generated)
melandory
PTAL. Screenshot is attached to the bug. https://codereview.chromium.org/2971783002/diff/60001/components/password_manager/core/browser/password_autofill_manager_unittest.cc File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_manager/core/browser/password_autofill_manager_unittest.cc#newcode906 components/password_manager/core/browser/password_autofill_manager_unittest.cc:906: // message ...
3 years, 5 months ago (2017-07-05 14:57:58 UTC) #19
vasilii
I spent most of my time on autofill_popup_view_views.cc. I hope you'll make it easier to ...
3 years, 5 months ago (2017-07-05 17:12:15 UTC) #26
melandory
https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons/show_all_saved_passwords.1x.icon File chrome/app/vector_icons/show_all_saved_passwords.1x.icon (right): https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons/show_all_saved_passwords.1x.icon#newcode1 chrome/app/vector_icons/show_all_saved_passwords.1x.icon:1: // Copyright 2017 The Chromium Authors. All rights reserved. ...
3 years, 5 months ago (2017-07-06 13:50:11 UTC) #33
vasilii
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode131 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:131: row_size += is_warning_message ? kHttpWarningNamePadding : kNamePadding; It's strange ...
3 years, 5 months ago (2017-07-06 15:15:22 UTC) #34
melandory
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode137 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:137: (is_warning_message ? kRightHandSideIconPapping : kIconPadding); On 2017/07/06 15:15:21, vasilii ...
3 years, 5 months ago (2017-07-07 12:26:32 UTC) #42
melandory
3 years, 5 months ago (2017-07-07 12:26:34 UTC) #43
vasilii
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode184 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:184: ? AutofillPopupLayoutModel::kRightHandSideIconPapping On 2017/07/07 12:26:31, melandory wrote: > On ...
3 years, 5 months ago (2017-07-07 12:51:27 UTC) #44
melandory
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode200 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:200: is_rtl ? value_rect.right() - value_width : value_rect.x(); On 2017/07/07 ...
3 years, 5 months ago (2017-07-10 14:15:14 UTC) #52
melandory
estade@chromium.org: Please review changes in *autofill*
3 years, 5 months ago (2017-07-10 14:15:52 UTC) #54
vasilii
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode125 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_right_handside_icon = it is left side https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode133 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:133: ...
3 years, 5 months ago (2017-07-10 17:17:58 UTC) #57
melandory
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode125 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_right_handside_icon = On 2017/07/10 17:17:57, vasilii wrote: > ...
3 years, 5 months ago (2017-07-11 16:00:02 UTC) #60
melandory
boliu@chromium.org: Please review changes in *android_webview*
3 years, 5 months ago (2017-07-11 16:00:18 UTC) #62
boliu
On 2017/07/11 16:00:18, melandory wrote: > mailto:boliu@chromium.org: Please review changes in > > > *android_webview* ...
3 years, 5 months ago (2017-07-11 16:47:01 UTC) #63
melandory
estade@, friendly ping :)
3 years, 5 months ago (2017-07-12 11:02:21 UTC) #66
vasilii
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode166 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:166: int x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x(); On ...
3 years, 5 months ago (2017-07-12 13:20:14 UTC) #67
Evan Stade
sorry I missed this due to some combination of skimming through the post-vacation email backlog, ...
3 years, 5 months ago (2017-07-12 17:53:53 UTC) #68
melandory
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode125 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_left_handside_icon = On 2017/07/12 17:53:52, Evan Stade wrote: ...
3 years, 5 months ago (2017-07-12 23:13:47 UTC) #71
melandory
https://codereview.chromium.org/2971783002/diff/220001/components/autofill/core/browser/autofill_client.h File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/220001/components/autofill/core/browser/autofill_client.h#newcode205 components/autofill/core/browser/autofill_client.h:205: virtual void ShowPasswordsSettingsPage() = 0; On 2017/07/12 17:53:52, Evan ...
3 years, 5 months ago (2017-07-13 10:38:10 UTC) #86
melandory
https://codereview.chromium.org/2971783002/diff/220001/components/autofill/core/browser/autofill_client.h File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/220001/components/autofill/core/browser/autofill_client.h#newcode205 components/autofill/core/browser/autofill_client.h:205: virtual void ShowPasswordsSettingsPage() = 0; On 2017/07/12 17:53:52, Evan ...
3 years, 5 months ago (2017-07-13 10:38:11 UTC) #87
melandory
PTAL at changes in: ios/chrome/browser/ui/autofill/autofill_client_ios.*
3 years, 5 months ago (2017-07-13 11:13:59 UTC) #96
melandory
To estade@ and vasilii@, so it does very minimal modification to the chrome/browser/ui/views/autofill/autofill_popup_view_views.cc. I'd be ...
3 years, 5 months ago (2017-07-13 13:06:17 UTC) #102
melandory
On 2017/07/13 13:06:17, melandory wrote: > To estade@ and vasilii@, so it does very minimal ...
3 years, 5 months ago (2017-07-13 13:06:42 UTC) #103
Mathieu
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode145 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, On 2017/07/12 23:13:47, melandory wrote: > ...
3 years, 5 months ago (2017-07-13 14:16:04 UTC) #111
melandory
On 2017/07/13 14:16:04, Mathieu wrote: > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode145 > ...
3 years, 5 months ago (2017-07-13 14:18:46 UTC) #112
Mathieu
On 2017/07/13 14:18:46, melandory wrote: > On 2017/07/13 14:16:04, Mathieu wrote: > > > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc ...
3 years, 5 months ago (2017-07-13 14:26:46 UTC) #113
vasilii
https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode47 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:47: const int kRowWithLeftHandsideIcon = 16; Confusing. The name shouldn't ...
3 years, 5 months ago (2017-07-13 16:11:51 UTC) #118
Evan Stade
fine with me to refactor DrawAutofillEntry later. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode225 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t ...
3 years, 5 months ago (2017-07-13 16:48:43 UTC) #119
melandory
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode225 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const { On 2017/07/12 23:13:47, melandory ...
3 years, 5 months ago (2017-07-25 15:08:50 UTC) #141
vasilii
https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode216 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:216: const int kIconSize = 16; constexpr https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/autofill/chrome_autofill_client.cc File chrome/browser/ui/autofill/chrome_autofill_client.cc ...
3 years, 4 months ago (2017-07-26 12:17:37 UTC) #147
melandory
https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode216 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:216: const int kIconSize = 16; On 2017/07/26 12:17:36, vasilii ...
3 years, 4 months ago (2017-07-26 15:01:30 UTC) #158
vasilii
lgtm
3 years, 4 months ago (2017-07-26 15:04:07 UTC) #159
melandory
mahmadi@ PTAL at ios files.
3 years, 4 months ago (2017-07-27 09:50:03 UTC) #180
melandory
estade@ please take another look
3 years, 4 months ago (2017-07-27 15:26:13 UTC) #183
Evan Stade
lgtm https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode122 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:122: bool has_substext) const { nit: s/substext/subtext/g https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode126 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:126: ...
3 years, 4 months ago (2017-07-27 18:17:57 UTC) #184
melandory
https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/autofill/autofill_popup_layout_model.cc#newcode122 chrome/browser/ui/autofill/autofill_popup_layout_model.cc:122: bool has_substext) const { On 2017/07/27 18:17:57, Evan Stade ...
3 years, 4 months ago (2017-07-28 10:24:50 UTC) #186
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2971783002/710001
3 years, 4 months ago (2017-07-28 10:26:25 UTC) #189
commit-bot: I haz the power
3 years, 4 months ago (2017-07-28 11:52:07 UTC) #192
Message was sent while issue was closed.
Committed patchset #15 (id:710001) as
https://chromium.googlesource.com/chromium/src/+/04fc429cf5d4c16316402d43c543...

Powered by Google App Engine
This is Rietveld 408576698