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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 3 weeks ago by kolos1
Modified:
1 day, 13 hours ago
Reviewers:
vasilii
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 user start typing password BUG=725883

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: 25

Patch Set 18 : fix enum.xml #

Unified diffs Side-by-side diffs Delta from patch set Stats (+470 lines, -14 lines) Patch
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/flag_descriptions.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/flag_descriptions.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +7 lines, -0 lines 0 comments Download
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 1 chunk +5 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 1 chunk +25 lines, -0 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 1 chunk +4 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 1 chunk +25 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 1 chunk +68 lines, -0 lines 0 comments Download
M chrome/browser/ui/passwords/passwords_client_ui_delegate.h View 1 2 3 4 1 chunk +10 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 3 chunks +11 lines, -0 lines 0 comments Download
M chrome/renderer/autofill/fake_content_password_manager_driver.cc View 1 2 3 4 5 6 7 8 9 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 1 chunk +83 lines, -0 lines 0 comments Download
M components/autofill/content/common/autofill_driver.mojom View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/content/renderer/DEPS View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -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 2 chunks +15 lines, -4 lines 0 comments Download
M components/autofill/content/renderer/renderer_save_password_progress_logger_unittest.cc View 1 2 3 4 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 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 1 chunk +13 lines, -0 lines 0 comments Download
M components/password_manager/core/browser/password_manager.h View 1 2 3 4 1 chunk +8 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 3 chunks +29 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 1 chunk +11 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 1 chunk +7 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 4 chunks +69 lines, -0 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 1 chunk +3 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 1 chunk +6 lines, -0 lines 0 comments Download
M components/password_manager/core/common/experiments.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +3 lines, -0 lines 0 comments Download
M components/password_manager/core/common/experiments.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
M components/password_manager/core/common/password_manager_features.h View 1 2 3 4 5 6 7 8 9 10 11 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/passwords/ios_chrome_password_manager_client.h View 1 2 3 4 5 1 chunk +4 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 1 chunk +10 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/input/KeyboardEventManager.cpp View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 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 3 chunks +5 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -1 line 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 26 (24 generated)
kolos1
Hi Vasilii, Please review this CL. Regards, Maxim
2 days, 11 hours ago (2017-07-20 15:23:35 UTC) #24
vasilii
1 day, 13 hours ago (2017-07-21 12:48:21 UTC) #26
Please next time upload smaller patches. That one could easily be split into
three well-defined pieces.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller.cc:102:
passwords_data_.OnAutomaticPasswordSave(std::move(form_manager));
That I don't understand. What scenario do you keep in mind and why is UI
confused with OnShowManualFallback in that case?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:818:
controller()->OnShowManualFallback(std::move(test_form_manager), is_update);
You can also check opened_bubble()

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:819:
ExpectIconStateIs(is_update
Why not ExpectIconAndControllerStateIs?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:824:
EXPECT_CALL(*controller(), OnUpdateBubbleAndIconVisibility());
Why do we expect it only here?

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/manage_passwords_ui_controller_unittest.cc:872:
controller()->OnHideManualFallback();
I need more info what's being tested here.
OnBubbleHidden() is a bug because you don't pop up the bubble.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
File chrome/browser/ui/passwords/passwords_client_ui_delegate.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:42: // save/update
prompt anchored to the omnibox icon but keeps it hidden.
It doesn't create any prompt. Only the icon.

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:43: virtual void
OnShowManualFallback(
ForSaving

https://codereview.chromium.org/2915763003/diff/340001/chrome/browser/ui/pass...
chrome/browser/ui/passwords/passwords_client_ui_delegate.h:48: // save/update
prompt and switch UI to |MANAGE_STATE|.
There is no a hidden prompt at any point in time. It just switches the state.
Also it's interesting to know what happens if the bubble is currently open.

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/fake_content_password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:133: int
called_show_manual_fallback_cnt() {
const

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/fake_content_password_manager_driver.h:222: int
called_show_manual_fallback_cnt_ = 0;
What is "cnt"? Probably should be renamed to "count"

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3086: //
available to save the entered value. It's a subject to re-consider.
Why do we need to reconsider it?

https://codereview.chromium.org/2915763003/diff/340001/chrome/renderer/autofi...
chrome/renderer/autofill/password_autofill_agent_browsertest.cc:3107: // Clear
all password fields. The fallback should be disabled.
What happens if you have two password fields. The user
- edits the first password
- edits the second password
- removes the second password

Is the manual fallback shown?

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/common/autofill_driver.mojom (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:82: // be available.
Can we get a bit lower level here. Describe what triggers it. Same below.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/common/autofill_driver.mojom:83:
ShowManualFallback(PasswordForm password_form);
Emphasize that it's for saving. Same below.

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/autofill/co...
components/autofill/content/renderer/password_autofill_agent.cc:1782: if
(password_manager::ManualFallbackForSavingEnabled()) {
Are you sure we need this check here? It introduces a dependency. We are going
to launch this feature anyway. Why not filtering those calls somewhere in the
browser?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.cc (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:140: bool
IsPasswordUpdate(const PasswordFormManager& provisional_save_manager) {
Comment?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.cc:489: if
(provisional_save_manager_->form_fetcher()->GetState() !=
== WAITING?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager.h:158: void
ShowManualFallback(password_manager::PasswordManagerDriver* driver,
ForSaving

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_driver.h (right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_driver.h:81: // Tells
the driver to show the manual fallback for password saving, i.e. to
Who tells whom? I think PasswordManagerDriver doesn't need this events.

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/browser/password_manager_unittest.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:87:
ShowManualFallbackPtr(manager.release(), is_update);
Memory leak here?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1828:
.WillRepeatedly(WithArg<1>(InvokeConsumer(stored_form)));
I'm concerned if it happens more than once. Is it true?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1834:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, true));
Why don't we check anything about the form being passed

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/browser/password_manager_unittest.cc:1868:
EXPECT_CALL(client_, ShowManualFallbackPtr(_, false));
Move down to ShowManualFallback. Do I understand right that the fallback isn't
shown automatically upon receiving the forms?

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
File components/password_manager/core/common/password_manager_features.cc
(right):

https://codereview.chromium.org/2915763003/diff/340001/components/password_ma...
components/password_manager/core/common/password_manager_features.cc:23:
"enable-manual-fallback-for-password-saving",
The documentation asks to use CamelCase for the name

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/input/KeyboardEventManager.cpp (right):

https://codereview.chromium.org/2915763003/diff/340001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/input/KeyboardEventManager.cpp:203:
(!is_browser_shortcut || initial_key_event.text[0] == '\b')) {
Can you clarify this?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973