Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(3)

Issue 2915763003: [Password Manager] Show omnibox icon and anchored prompt once user start typing password (Closed)

Created:
5 months, 3 weeks ago by kolos1
Modified:
3 months, 2 weeks ago
Reviewers:
vasilii, Mike West
CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, qsr+mojo_chromium.org, Aaron Boodman, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, viettrungluu+watch_chromium.org, browser-components-watch_chromium.org, yzshen+watch_chromium.org, abarth-chromium, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, darin (slow to review), gcasto+watchlist_chromium.org, vabr+watchlistpasswordmanager_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Password Manager] Show omnibox icon and anchored prompt once users start typing password If the password to be save is a generated by Chrome password, the confirmation bubble will be shown (the bubble that a user sees when she submits a form with generated password). If the password is not a generated one, a save or update bubble will be shown. BUG=725883 Review-Url: https://codereview.chromium.org/2915763003 Cr-Commit-Position: refs/heads/master@{#493337} Committed: https://chromium.googlesource.com/chromium/src/+/7cf7408af869f5b605a6f35cdd458380c56fb73d

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Patch Set 14 : #

Patch Set 15 : #

Patch Set 16 : #

Patch Set 17 : Sent For Review #

Total comments: 58

Patch Set 18 : Changes addressed to vasilii@ comments #

Total comments: 4

Patch Set 19 : Changes address to vasilii@ comments (second iteration) #

Total comments: 2

Patch Set 20 : ui tests #

Total comments: 10

Patch Set 21 : CheckThatCredentialsStored fixed #

Patch Set 22 : Introduced |has_generated_password| argument #

Total comments: 18

Patch Set 23 : ShouldBlockPasswordForSameOriginButDifferentScheme added #

Patch Set 24 : Changes addressed to reveiwer comments #

Total comments: 24

Patch Set 25 : Changes addressed to reviewer comments #

Patch Set 26 : '|web_contents|' comment is back #

Total comments: 8

Patch Set 27 : Changes addressed to revewer comments #

Total comments: 2

Patch Set 28 : Changes comments for IsSave/UpdatePromptShownAutomatically #

Patch Set 29 : #

Patch Set 30 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+815 lines, -206 lines) Patch
M chrome/browser/password_manager/chrome_password_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +6 lines, -1 line 0 comments Download
M chrome/browser/password_manager/chrome_password_manager_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/password_manager/credential_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 7 chunks +11 lines, -11 lines 0 comments Download
M chrome/browser/password_manager/password_manager_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 80 chunks +93 lines, -139 lines 0 comments Download
M chrome/browser/password_manager/password_manager_interactive_uitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 2 chunks +21 lines, -1 line 0 comments Download
M chrome/browser/password_manager/password_manager_test_base.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 3 chunks +29 lines, -7 lines 0 comments Download
M chrome/browser/password_manager/password_manager_test_base.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 8 chunks +144 lines, -33 lines 0 comments Download
M chrome/browser/ui/passwords/manage_passwords_ui_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/passwords/manage_passwords_ui_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +83 lines, -0 lines 0 comments Download
M chrome/browser/ui/passwords/passwords_client_ui_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/renderer/autofill/fake_content_password_manager_driver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 3 chunks +13 lines, -1 line 0 comments Download
M chrome/renderer/autofill/fake_content_password_manager_driver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +9 lines, -0 lines 0 comments Download
M chrome/renderer/autofill/password_autofill_agent_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +76 lines, -0 lines 0 comments Download
M components/autofill/content/common/autofill_driver.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +8 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +12 lines, -4 lines 0 comments Download
M components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +5 lines, -0 lines 0 comments Download
M components/password_manager/content/browser/bad_message.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +2 lines, -0 lines 0 comments Download
M components/password_manager/content/browser/content_password_manager_driver.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +13 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +9 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +42 lines, -6 lines 0 comments Download
M components/password_manager/core/browser/password_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +12 lines, -1 line 0 comments Download
M components/password_manager/core/browser/password_manager_driver.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +8 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +116 lines, -2 lines 0 comments Download
M components/password_manager/core/browser/stub_password_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +5 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/stub_password_manager_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 1 chunk +7 lines, -0 lines 0 comments Download
M ios/chrome/browser/passwords/ios_chrome_password_manager_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +5 lines, -0 lines 0 comments Download
M ios/chrome/browser/passwords/ios_chrome_password_manager_client.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +11 lines, -0 lines 0 comments Download
M tools/metrics/histograms/enums.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 118 (86 generated)
kolos1
Hi Vasilii, Please review this CL. Regards, Maxim
4 months, 1 week ago (2017-07-20 15:23:35 UTC) #24
vasilii
Please next time upload smaller patches. That one could easily be split into three well-defined ...
4 months ago (2017-07-21 12:48:21 UTC) #26
kolos1
https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc#newcode102 chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102: passwords_data_.OnAutomaticPasswordSave(std::move(form_manager)); On 2017/07/21 12:48:19, vasilii wrote: > That I ...
4 months ago (2017-07-24 15:33:31 UTC) #31
vasilii
https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right): https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/passwords/manage_passwords_ui_controller.cc#newcode102 chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102: passwords_data_.OnAutomaticPasswordSave(std::move(form_manager)); On 2017/07/24 15:33:29, kolos1 wrote: > On 2017/07/21 ...
4 months ago (2017-07-25 11:17:47 UTC) #32
kolos1
Hi Vasilii, PTAL. Please skip the generation related issues. We will discuss them tomorrow. Regards, ...
4 months ago (2017-07-26 13:40:58 UTC) #33
vasilii
https://codereview.chromium.org/2915763003/diff/460001/components/password_manager/core/browser/password_manager_unittest.cc File components/password_manager/core/browser/password_manager_unittest.cc (right): https://codereview.chromium.org/2915763003/diff/460001/components/password_manager/core/browser/password_manager_unittest.cc#newcode1875 components/password_manager/core/browser/password_manager_unittest.cc:1875: // credentials too. I'm confused about the comment. The ...
4 months ago (2017-07-26 14:59:33 UTC) #34
kolos1
Hi Vasilii, PTAL. I added a test to password_manager/password_manager_interactive_uitest.cc and changes some functions for browser/interactive ...
3 months, 3 weeks ago (2017-08-01 11:04:35 UTC) #48
vasilii
I'll keep reviewing it tomorrow. https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode179 chrome/browser/password_manager/password_manager_test_base.h:179: // password store contains ...
3 months, 3 weeks ago (2017-08-02 17:41:11 UTC) #61
kolos1
https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode179 chrome/browser/password_manager/password_manager_test_base.h:179: // password store contains only 1 credential. On 2017/08/02 ...
3 months, 3 weeks ago (2017-08-03 07:58:02 UTC) #62
vasilii
https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode181 chrome/browser/password_manager/password_manager_test_base.h:181: password_manager::TestPasswordStore* password_store, On 2017/08/03 07:58:01, kolos1 wrote: > On ...
3 months, 3 weeks ago (2017-08-03 08:59:15 UTC) #63
kolos1
https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode181 chrome/browser/password_manager/password_manager_test_base.h:181: password_manager::TestPasswordStore* password_store, On 2017/08/03 08:59:15, vasilii wrote: > On ...
3 months, 3 weeks ago (2017-08-03 09:08:03 UTC) #64
vasilii
More comments are coming. https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode181 chrome/browser/password_manager/password_manager_test_base.h:181: password_manager::TestPasswordStore* password_store, On 2017/08/03 09:08:03, ...
3 months, 3 weeks ago (2017-08-03 09:55:39 UTC) #65
vasilii
https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/password_manager/password_manager_interactive_uitest.cc File chrome/browser/password_manager/password_manager_interactive_uitest.cc (right): https://codereview.chromium.org/2915763003/diff/660002/chrome/browser/password_manager/password_manager_interactive_uitest.cc#newcode112 chrome/browser/password_manager/password_manager_interactive_uitest.cc:112: prompt_observer.WaitForSavePrompt(); I think that is the only place which ...
3 months, 3 weeks ago (2017-08-03 10:46:37 UTC) #66
kolos1
Hi Vasilii, PTAL. Thanks, Maxim https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/660001/chrome/browser/password_manager/password_manager_test_base.h#newcode181 chrome/browser/password_manager/password_manager_test_base.h:181: password_manager::TestPasswordStore* password_store, On 2017/08/03 ...
3 months, 3 weeks ago (2017-08-04 16:36:18 UTC) #75
vasilii
https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_browsertest.cc#newcode211 chrome/browser/password_manager/password_manager_browsertest.cc:211: scoped_refptr<password_manager::TestPasswordStore> password_store = unused https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_browsertest.cc#newcode1021 chrome/browser/password_manager/password_manager_browsertest.cc:1021: EXPECT_FALSE(password_store->IsEmpty()); excessive, also ...
3 months, 2 weeks ago (2017-08-07 17:13:24 UTC) #78
kolos1
Hi Vasilii, Thanks for comments. PTAL. Regards, Maxim https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_browsertest.cc File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_browsertest.cc#newcode211 chrome/browser/password_manager/password_manager_browsertest.cc:211: scoped_refptr<password_manager::TestPasswordStore> ...
3 months, 2 weeks ago (2017-08-08 12:37:17 UTC) #82
vasilii
https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (left): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h#oldcode88 chrome/browser/password_manager/password_manager_test_base.h:88: // PasswordManagerBrowserTestBase. On 2017/08/08 12:37:17, kolos1 wrote: > On ...
3 months, 2 weeks ago (2017-08-08 13:26:31 UTC) #85
kolos1
https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (left): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h#oldcode88 chrome/browser/password_manager/password_manager_test_base.h:88: // PasswordManagerBrowserTestBase. On 2017/08/08 13:26:31, vasilii wrote: > On ...
3 months, 2 weeks ago (2017-08-08 13:41:58 UTC) #89
kolos1
MojoConnectionRecreatedAfterNavigation fixed.
3 months, 2 weeks ago (2017-08-08 13:54:13 UTC) #90
vasilii
https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (left): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h#oldcode88 chrome/browser/password_manager/password_manager_test_base.h:88: // PasswordManagerBrowserTestBase. On 2017/08/08 13:41:57, kolos1 wrote: > On ...
3 months, 2 weeks ago (2017-08-08 13:57:42 UTC) #91
kolos1
https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (left): https://codereview.chromium.org/2915763003/diff/850001/chrome/browser/password_manager/password_manager_test_base.h#oldcode88 chrome/browser/password_manager/password_manager_test_base.h:88: // PasswordManagerBrowserTestBase. On 2017/08/08 13:57:42, vasilii wrote: > On ...
3 months, 2 weeks ago (2017-08-08 16:20:07 UTC) #94
vasilii
https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/password_manager/password_manager_test_base.cc File chrome/browser/password_manager/password_manager_test_base.cc (right): https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/password_manager/password_manager_test_base.cc#newcode99 chrome/browser/password_manager/password_manager_test_base.cc:99: bool CheckIfTargetStateIsObserved( IsTargetStateObserved https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/password_manager/password_manager_test_base.cc#newcode104 chrome/browser/password_manager/password_manager_test_base.cc:104: const password_manager::ui::State target_state, I ...
3 months, 2 weeks ago (2017-08-09 09:32:15 UTC) #95
kolos1
Hi Vasilii Your comments have been addressed. PTAL. Regards, Maxim https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/password_manager/password_manager_test_base.cc File chrome/browser/password_manager/password_manager_test_base.cc (right): https://codereview.chromium.org/2915763003/diff/930001/chrome/browser/password_manager/password_manager_test_base.cc#newcode99 ...
3 months, 2 weeks ago (2017-08-09 09:55:06 UTC) #98
vasilii
lgtm https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/password_manager/password_manager_test_base.h#newcode76 chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if the save prompt was shown ...
3 months, 2 weeks ago (2017-08-09 10:31:32 UTC) #99
kolos1
Thanks Vasilii for this durable review. https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/password_manager/password_manager_test_base.h File chrome/browser/password_manager/password_manager_test_base.h (right): https://codereview.chromium.org/2915763003/diff/950001/chrome/browser/password_manager/password_manager_test_base.h#newcode76 chrome/browser/password_manager/password_manager_test_base.h:76: // Checks if ...
3 months, 2 weeks ago (2017-08-09 11:37:22 UTC) #100
kolos1
Hi Mike! Please review changes in components/autofill/content/common/autofill_driver.mojom Thanks, Maxim
3 months, 2 weeks ago (2017-08-09 11:39:10 UTC) #102
kolos1
mkwst: Hi Mike! Friendly ping. Please mojo changes in autofill_driver.mojom. Thx.
3 months, 2 weeks ago (2017-08-10 06:31:39 UTC) #107
Mike West
On 2017/08/10 at 06:31:39, kolos wrote: > mkwst: Hi Mike! Friendly ping. Please mojo changes ...
3 months, 2 weeks ago (2017-08-10 06:52:14 UTC) #108
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/2915763003/1010001
3 months, 2 weeks ago (2017-08-10 07:06:46 UTC) #111
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_compile_dbg_ng/builds/475253)
3 months, 2 weeks ago (2017-08-10 08:00:16 UTC) #113
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/2915763003/1010001
3 months, 2 weeks ago (2017-08-10 08:31:15 UTC) #115
commit-bot: I haz the power
3 months, 2 weeks ago (2017-08-10 09:00:32 UTC) #118
Message was sent while issue was closed.
Committed patchset #30 (id:1010001) as
https://chromium.googlesource.com/chromium/src/+/7cf7408af869f5b605a6f35cdd45...

Powered by Google App Engine
This is Rietveld efc10ee0f