Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
256003003:
Enable Automatic Passwords Saving (Closed)
|
Created:
6 years, 7 months ago by rchtara Modified:
6 years, 7 months ago Reviewers:
vabr (Chromium) CC:
chromium-reviews, jar (doing other things), benquan, browser-components-watch_chromium.org, asvitkine+watch_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, stgao, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionEnable Automatic Passwords Saving
A new flag "enable-automatic-password-saving" is added in this cl to chrome to allow automatic passwords saving. This flag is intended for testing use, not for end users.
BUG=367690
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267214
Patch Set 1 #
Total comments: 32
Patch Set 2 : #Patch Set 3 : #Patch Set 4 : removing pref #Patch Set 5 : #Patch Set 6 : #
Total comments: 4
Patch Set 7 : #Patch Set 8 : tests #
Total comments: 9
Patch Set 9 : #
Total comments: 6
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #
Total comments: 2
Patch Set 12 : #Patch Set 13 : #Patch Set 14 : rebase #Patch Set 15 : custombuildonly #
Total comments: 8
Patch Set 16 : #Messages
Total messages: 54 (0 generated)
Hi Vaclav, Could you please review this cl, Thanks a lot Riadh
Hi Riadh. First of all: 1) Please create a bug for this, and explain what you are doing. You should emphasise that currently this is just a command-line flag for automatically saving the passwords, intended for testing use, not for end users. You can mention that transforming this into a user-facing setting is currently considered, and will get a separate launch bug if we go with that. 2) Add the bug number from above to the CL description, and also quickly explain what this CL is changing. Some more comments are below. Thanks! Vaclav https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:6342: + Enable automatic passwords saving. nit: Just "Save passwords automatically." ("enable" will be already the link text on the about:flags page to switch this on.) https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:6345: + Disable password prompt and save automatically password without notifying the user. Please avoid the word "disable", as the whole thing would read "Enable disable ...". What about: "Skip the password prompt and save passwords automatically," instead? https://codereview.chromium.org/256003003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/256003003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1118: "enable-automatic-passwords-saving", nit: passwords->password (see, e.g., the flag for password generation above, which also uses the singular form). If you change this, please also change the switch name definition. https://codereview.chromium.org/256003003/diff/1/chrome/browser/chrome_conten... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/256003003/diff/1/chrome/browser/chrome_conten... chrome/browser/chrome_content_browser_client.cc:1565: autofill::switches::kEnableAutomaticPasswordsSaving, If I understand it right, this code passes the switch to the renderer process. But it does not look like you need it there. Please explain why do you need it here, or remove the added line. https://codereview.chromium.org/256003003/diff/1/chrome/test/chromedriver/chr... File chrome/test/chromedriver/chrome/preferences.txt (right): https://codereview.chromium.org/256003003/diff/1/chrome/test/chromedriver/chr... chrome/test/chromedriver/chrome/preferences.txt:41: "password_manager_enable_automatic_passwords_saving": false As we just discussed, the setting is not currently needed, command line is enough for testing. We can add the setting once we also know which UI to create for it. https://codereview.chromium.org/256003003/diff/1/chrome/test/data/policy/poli... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/256003003/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:319: "PasswordManagerEnableAutomaticPasswordsSaving": { I don't understand what's the purpose of this change. If it's not needed, please remove it, otherwise please explain. https://codereview.chromium.org/256003003/diff/1/components/autofill/core/com... File components/autofill/core/common/autofill_switches.h (right): https://codereview.chromium.org/256003003/diff/1/components/autofill/core/com... components/autofill/core/common/autofill_switches.h:16: extern const char kEnableAutomaticPasswordsSaving[]; I think this (and the corresponding part in *.cc) should rather go to components/password_manager/core/common/password_manager_switches.*, unless you explicitly plan to use the switches in the autofill code as well. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:38: bool password_manager_enable_automatic_passwords_saving) { I don't think we need UMA metrics until we expose this to the users, so I suggest removing them. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:43: nit: Please remove this blank line. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:67: prefs::kPasswordManagerEnableAutomaticPasswordsSaving, As noted in one of the comments above, please remove the setting creation code for now. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:85: BooleanPrefMember enable_automatic_passwords_saving_pref; Ditto: As noted in one of the comments above, please remove the setting creation code for now. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.h:175: bool enable_automatic_passwords_saving_; nit: Drop this member variable, and just use client_->IsAutomaticPasswordsSavingEnabled() in code instead. Alternatively, if you have good reasons to cache the value, make this member variable a "const bool", and initialise it in the initialiser list of the PasswordManager constructor. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager_client.h:28: virtual bool IsAutomaticPasswordsSavingEnabled() const { Please document this method. For example: "For automated testing, the save password prompt should sometimes not be shown, and passwords immediately saved instead. That can be enforced by a command-line flag. If auto-saving is enforced, this method returns true. The default return value is false." https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager_client.h:29: return false; nit: Please only declare the method here, and define it in the *.cc file. Inline definitions should only be done for data member accessors (variable() and set_variable()). https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/common/password_manager_pref_names.h:22: extern const char kPasswordManagerEnableAutomaticPasswordsSaving[]; Please revert the changes to this *.h file and its corresponding *.cc file, because we don't introduce the preferences yet. https://codereview.chromium.org/256003003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/256003003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:16781: +<histogram name="PasswordManager.EnableAutomaticPasswordsSaving" enum="Boolean"> If we don't add the UMA metric now, then please drop this histogram definition as well.
Hi vaclav, Great! Thanks a lot. I updated the CL, and fixed every thing you asked me to fix. Riadh https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:6342: + Enable automatic passwords saving. On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: Just "Save passwords automatically." > ("enable" will be already the link text on the about:flags page to switch this > on.) Done. https://codereview.chromium.org/256003003/diff/1/chrome/app/generated_resourc... chrome/app/generated_resources.grd:6345: + Disable password prompt and save automatically password without notifying the user. On 2014/04/28 09:13:14, vabr (Chromium) wrote: > Please avoid the word "disable", as the whole thing would read "Enable disable > ...". > What about: "Skip the password prompt and save passwords automatically," > instead? Done. https://codereview.chromium.org/256003003/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/256003003/diff/1/chrome/browser/about_flags.c... chrome/browser/about_flags.cc:1118: "enable-automatic-passwords-saving", On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: passwords->password (see, e.g., the flag for password generation above, > which also uses the singular form). If you change this, please also change the > switch name definition. Done. https://codereview.chromium.org/256003003/diff/1/chrome/browser/chrome_conten... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/256003003/diff/1/chrome/browser/chrome_conten... chrome/browser/chrome_content_browser_client.cc:1565: autofill::switches::kEnableAutomaticPasswordsSaving, On 2014/04/28 09:13:14, vabr (Chromium) wrote: > If I understand it right, this code passes the switch to the renderer process. > But it does not look like you need it there. Please explain why do you need it > here, or remove the added line. Done. https://codereview.chromium.org/256003003/diff/1/chrome/test/chromedriver/chr... File chrome/test/chromedriver/chrome/preferences.txt (right): https://codereview.chromium.org/256003003/diff/1/chrome/test/chromedriver/chr... chrome/test/chromedriver/chrome/preferences.txt:41: "password_manager_enable_automatic_passwords_saving": false On 2014/04/28 09:13:14, vabr (Chromium) wrote: > As we just discussed, the setting is not currently needed, command line is > enough for testing. We can add the setting once we also know which UI to create > for it. Done. https://codereview.chromium.org/256003003/diff/1/chrome/test/data/policy/poli... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/256003003/diff/1/chrome/test/data/policy/poli... chrome/test/data/policy/policy_test_cases.json:319: "PasswordManagerEnableAutomaticPasswordsSaving": { On 2014/04/28 09:13:14, vabr (Chromium) wrote: > I don't understand what's the purpose of this change. > If it's not needed, please remove it, otherwise please explain. Done. https://codereview.chromium.org/256003003/diff/1/components/autofill/core/com... File components/autofill/core/common/autofill_switches.h (right): https://codereview.chromium.org/256003003/diff/1/components/autofill/core/com... components/autofill/core/common/autofill_switches.h:16: extern const char kEnableAutomaticPasswordsSaving[]; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > I think this (and the corresponding part in *.cc) should rather go to > components/password_manager/core/common/password_manager_switches.*, unless you > explicitly plan to use the switches in the autofill code as well. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:38: bool password_manager_enable_automatic_passwords_saving) { On 2014/04/28 09:13:14, vabr (Chromium) wrote: > I don't think we need UMA metrics until we expose this to the users, so I > suggest removing them. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:43: On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:67: prefs::kPasswordManagerEnableAutomaticPasswordsSaving, On 2014/04/28 09:13:14, vabr (Chromium) wrote: > As noted in one of the comments above, please remove the setting creation code > for now. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.cc:85: BooleanPrefMember enable_automatic_passwords_saving_pref; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > Ditto: As noted in one of the comments above, please remove the setting creation > code for now. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager.h:175: bool enable_automatic_passwords_saving_; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: Drop this member variable, and just use > client_->IsAutomaticPasswordsSavingEnabled() in code instead. > > Alternatively, if you have good reasons to cache the value, make this member > variable a "const bool", and initialise it in the initialiser list of the > PasswordManager constructor. Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/browser/password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager_client.h:29: return false; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: Please only declare the method here, and define it in the *.cc file. Inline > definitions should only be done for data member accessors (variable() and > set_variable()). Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/browser/password_manager_client.h:29: return false; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > nit: Please only declare the method here, and define it in the *.cc file. Inline > definitions should only be done for data member accessors (variable() and > set_variable()). Done. https://codereview.chromium.org/256003003/diff/1/components/password_manager/... File components/password_manager/core/common/password_manager_pref_names.h (right): https://codereview.chromium.org/256003003/diff/1/components/password_manager/... components/password_manager/core/common/password_manager_pref_names.h:22: extern const char kPasswordManagerEnableAutomaticPasswordsSaving[]; On 2014/04/28 09:13:14, vabr (Chromium) wrote: > Please revert the changes to this *.h file and its corresponding *.cc file, > because we don't introduce the preferences yet. Done. https://codereview.chromium.org/256003003/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/256003003/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:16781: +<histogram name="PasswordManager.EnableAutomaticPasswordsSaving" enum="Boolean"> On 2014/04/28 09:13:14, vabr (Chromium) wrote: > If we don't add the UMA metric now, then please drop this histogram definition > as well. Done.
Thanks, Riadh, the changes so far look good. However, before I can approve the CL, there need to be tests: 1) A unit test for the ChromePasswordManagerClient to ensure that A) IsAutomaticPasswordSavingEnabled() returns false when the command line is not changed, and that B) IsAutomaticPasswordSavingEnabled() returns true when the correct command-line flag is given. 2) Similarly, a browser test for PasswordManager to verify that A) New passwords which are not generated or PSL-matched still don't get saved automatically without changes to the command line. B) Such passwords get saved automatically with the right command-line flag. Obviously, 1A and 2A are much more important to test than 1B and 2B, because the As being broken were a privacy/security disaster, whereas Bs only break the automatic testing. Thanks for you good work so far! :) Vaclav https://codereview.chromium.org/256003003/diff/100001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/100001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:38: virtual ~ChromePasswordManagerClient(); nit: Keep the blank line after the destructor declaration. https://codereview.chromium.org/256003003/diff/100001/components/password_man... File components/password_manager/core/common/password_manager_switches.cc (right): https://codereview.chromium.org/256003003/diff/100001/components/password_man... components/password_manager/core/common/password_manager_switches.cc:11: // Disables password prompt and saves automatically password without notifying nit: Suggested reformulation: "Disables the save-password prompt. Passwords are then saved automatically, without asking the user." (Note in particular the difference between "asking" and "notifying" the user. If we make this a user-facing feature, there will be a notification, but not a question. The suggested change will keep the flag comment up to date.)
Hi vaclav, I have just fixed the small issues. I will start working on the tests right now. Thanks a lot for your review Riadh https://codereview.chromium.org/256003003/diff/100001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/100001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:38: virtual ~ChromePasswordManagerClient(); On 2014/04/28 13:43:42, vabr (Chromium) wrote: > nit: Keep the blank line after the destructor declaration. Done. https://codereview.chromium.org/256003003/diff/100001/components/password_man... File components/password_manager/core/common/password_manager_switches.cc (right): https://codereview.chromium.org/256003003/diff/100001/components/password_man... components/password_manager/core/common/password_manager_switches.cc:11: // Disables password prompt and saves automatically password without notifying On 2014/04/28 13:43:42, vabr (Chromium) wrote: > nit: Suggested reformulation: > > "Disables the save-password prompt. Passwords are then saved automatically, > without asking the user." > > (Note in particular the difference between "asking" and "notifying" the user. If > we make this a user-facing feature, there will be a notification, but not a > question. The suggested change will keep the flag comment up to date.) Done.
Hi vaclav, I added the tests. Could you please check them ? Thanks a lot Riadh
Thanks, Riadh, I like the tests you added. However, there is one piece missing in the browser test -- checking that the password indeed got saved. Please see my comments below. Cheers, Vaclav https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:10: #include "chrome/common/chrome_switches.h" Please do not include files which you don't need: 1) You don't need this in the header file at all. 2) Even in the .cc file you need just #include "components/password_manager/core/common/password_manager_switches.h" instead. Note: once you remove this #include from the header, don't forget to include components/password_manager/core/common/password_manager_switches.h also in the *_unittest.cc, as well as in the *.cc. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:82: TEST_F(ChromePasswordManagerClientTest, IsAutomaticPasswordSavingEnabledTest) { nit: please split this test into two: one testing the default behaviour, and the other setting the command line flag and testing that. Having smaller independent tests helps both to understand better a potential failure, and also avoid flakiness. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:166: // Checkes that enable-automatic-password-saving is not enabled nit: This comment is not really necessary, please consider removing it. If you want to keep it, then please fix the typo Checkes->Checks and also add a full-stop '.' at the end of the sentence. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:659: // Fill a form and submit through a <input type="submit"> button. Nothing nit: "Nothing special" is just clutter, please remove from here and also below. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:689: EXPECT_FALSE(observer.infobar_shown()); Please also check that the password indeed got saved. That means, before submitting the form, check that there credentials ('temp', 'random') are not known to the PasswordStore, and before the EXPECT_FALSE check that they are there.
Hi vaclav, I updated the cl. Thanks a lot for the review Riadh https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.h (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.h:10: #include "chrome/common/chrome_switches.h" On 2014/04/29 09:08:26, vabr (Chromium) wrote: > Please do not include files which you don't need: > 1) You don't need this in the header file at all. > 2) Even in the .cc file you need just > #include "components/password_manager/core/common/password_manager_switches.h" > instead. > > Note: once you remove this #include from the header, don't forget to include > components/password_manager/core/common/password_manager_switches.h also in the > *_unittest.cc, as well as in the *.cc. Done. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:82: TEST_F(ChromePasswordManagerClientTest, IsAutomaticPasswordSavingEnabledTest) { On 2014/04/29 09:08:26, vabr (Chromium) wrote: > nit: please split this test into two: one testing the default behaviour, and the > other setting the command line flag and testing that. > Having smaller independent tests helps both to understand better a potential > failure, and also avoid flakiness. Done. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:166: // Checkes that enable-automatic-password-saving is not enabled On 2014/04/29 09:08:26, vabr (Chromium) wrote: > nit: This comment is not really necessary, please consider removing it. > > If you want to keep it, then please fix the typo Checkes->Checks and also add a > full-stop '.' at the end of the sentence. Done. https://codereview.chromium.org/256003003/diff/150001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:659: // Fill a form and submit through a <input type="submit"> button. Nothing On 2014/04/29 09:08:26, vabr (Chromium) wrote: > nit: "Nothing special" is just clutter, please remove from here and also below. Done.
Awesome, thanks a lot, Riadh. LGTM: after you address my final comments, feel free to send to the commit queue. Thanks for your work! Vaclav https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87: EXPECT_FALSE(client->IsAutomaticPasswordSavingEnabled()); nit: get rid of the |client| variable and just call GetClient()->IsAutomaticPasswordSavingEnabled(). That will make the code shorter and easier to read. The same below. https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:672: (password_manager::TestPasswordStore*)PasswordStoreFactory::GetForProfile( Please don't use C-style casts. Use static_cast<> instead. Also, please correct the formatting of this line. https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:675: ASSERT_TRUE(password_store->stored_passwords().size() == 0); Please use EXPECT_EQ(0u, password_store->stored_passwords().size()); instead. Reasons: 1) EXPECT instead of ASSERT, because the test is still valuable to run after this fails (it checks for the infobar not shown), and ASSERT would just kill it. 2) _EQ instead of _TRUE, because _EQ prints out the size on error, whereas _TRUE would just say that the expected and actual values were unequal. Please also correct this at the end of the test.
The CQ bit was checked by rchtara@chromium.org
The CQ bit was unchecked by rchtara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/password_manager/chrome_password_manager_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/password_manager/chrome_password_manager_client.cc
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 77 (offset 14 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/password_manager/chrome_password_manager_client.cc.rej
Patch: chrome/browser/password_manager/chrome_password_manager_client.cc
Index: chrome/browser/password_manager/chrome_password_manager_client.cc
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc
b/chrome/browser/password_manager/chrome_password_manager_client.cc
index
ed721657823aa0ce0f0d422e91e29c365c4f0267..f17cf0fb3b27e232841c2a4f8552c9e8962e8a8d
100644
--- a/chrome/browser/password_manager/chrome_password_manager_client.cc
+++ b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -25,6 +25,7 @@
#include "components/password_manager/core/browser/password_manager.h"
#include "components/password_manager/core/browser/password_manager_logger.h"
#include
"components/password_manager/core/browser/password_manager_metrics_util.h"
+#include "components/password_manager/core/common/password_manager_switches.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_view.h"
#include "ipc/ipc_message_macros.h"
@@ -63,6 +64,11 @@ ChromePasswordManagerClient::ChromePasswordManagerClient(
ChromePasswordManagerClient::~ChromePasswordManagerClient() {}
+bool ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
+ return CommandLine::ForCurrentProcess()->HasSwitch(
+ password_manager::switches::kEnableAutomaticPasswordSaving);
+}
+
void ChromePasswordManagerClient::PromptUserToSavePassword(
password_manager::PasswordFormManager* form_to_save) {
if (IsTheHotNewBubbleUIEnabled()) {
Hi, Great!! It's time to commit. Thanks a lot for you help vaclav Riadh https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:87: EXPECT_FALSE(client->IsAutomaticPasswordSavingEnabled()); On 2014/04/29 13:04:44, vabr (Chromium) wrote: > nit: get rid of the |client| variable and just call > GetClient()->IsAutomaticPasswordSavingEnabled(). > That will make the code shorter and easier to read. > > The same below. Done. https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:672: (password_manager::TestPasswordStore*)PasswordStoreFactory::GetForProfile( On 2014/04/29 13:04:44, vabr (Chromium) wrote: > Please don't use C-style casts. Use static_cast<> instead. > > Also, please correct the formatting of this line. Done. https://codereview.chromium.org/256003003/diff/170001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:675: ASSERT_TRUE(password_store->stored_passwords().size() == 0); On 2014/04/29 13:04:44, vabr (Chromium) wrote: > Please use > EXPECT_EQ(0u, password_store->stored_passwords().size()); > instead. Reasons: > 1) EXPECT instead of ASSERT, because the test is still valuable to run after > this fails (it checks for the infobar not shown), and ASSERT would just kill it. > 2) _EQ instead of _TRUE, because _EQ prints out the size on error, whereas _TRUE > would just say that the expected and actual values were unequal. > > Please also correct this at the end of the test. Done.
The CQ bit was checked by rchtara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/password_manager/chrome_password_manager_client.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/password_manager/chrome_password_manager_client.cc
Hunk #1 FAILED at 25.
Hunk #2 succeeded at 77 (offset 14 lines).
1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/password_manager/chrome_password_manager_client.cc.rej
Patch: chrome/browser/password_manager/chrome_password_manager_client.cc
Index: chrome/browser/password_manager/chrome_password_manager_client.cc
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc
b/chrome/browser/password_manager/chrome_password_manager_client.cc
index
ed721657823aa0ce0f0d422e91e29c365c4f0267..f17cf0fb3b27e232841c2a4f8552c9e8962e8a8d
100644
--- a/chrome/browser/password_manager/chrome_password_manager_client.cc
+++ b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -25,6 +25,7 @@
#include "components/password_manager/core/browser/password_manager.h"
#include "components/password_manager/core/browser/password_manager_logger.h"
#include
"components/password_manager/core/browser/password_manager_metrics_util.h"
+#include "components/password_manager/core/common/password_manager_switches.h"
#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_view.h"
#include "ipc/ipc_message_macros.h"
@@ -63,6 +64,11 @@ ChromePasswordManagerClient::ChromePasswordManagerClient(
ChromePasswordManagerClient::~ChromePasswordManagerClient() {}
+bool ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled() const {
+ return CommandLine::ForCurrentProcess()->HasSwitch(
+ password_manager::switches::kEnableAutomaticPasswordSaving);
+}
+
void ChromePasswordManagerClient::PromptUserToSavePassword(
password_manager::PasswordFormManager* form_to_save) {
if (IsTheHotNewBubbleUIEnabled()) {
Hi Riadh, Still LGTM, but please address the new comments below. Cheers, Vaclav https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:676: EXPECT_EQ(password_store->stored_passwords().size(), 0); nit: Please revert the order: 0u, password_store->stored_passwords().size() It's a Chromium-wide convention that the first operand is the expected one, and the second the actual one for _EQ macros. https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:693: EXPECT_EQ(password_store->stored_passwords().size(), 1); ditto: please revert the order: 1u, password_store->stored_passwords().size() https://codereview.chromium.org/256003003/diff/200001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/200001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:673: (PasswordStoreFactory::GetForProfile(browser()->profile(), nit: Please format this correctly (even better, run "git cl format").
Hi, Ok, I fixed them, I hope every thing is fine. Thanks a lot Vaclav Riadh https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:676: EXPECT_EQ(password_store->stored_passwords().size(), 0); On 2014/04/29 13:58:46, vabr (Chromium) wrote: > nit: Please revert the order: > 0u, password_store->stored_passwords().size() > > It's a Chromium-wide convention that the first operand is the expected one, and > the second the actual one for _EQ macros. Done. https://codereview.chromium.org/256003003/diff/170002/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:693: EXPECT_EQ(password_store->stored_passwords().size(), 1); On 2014/04/29 13:58:46, vabr (Chromium) wrote: > ditto: please revert the order: > 1u, password_store->stored_passwords().size() Done. https://codereview.chromium.org/256003003/diff/200001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/200001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:673: (PasswordStoreFactory::GetForProfile(browser()->profile(), On 2014/04/29 13:58:46, vabr (Chromium) wrote: > nit: Please format this correctly (even better, run "git cl format"). Done.
Thanks, Riadh. LGTM, no further comments. Feel free to send to the Commit Queue again. Cheers, Vaclav
Great, but there was same conflict. So, I'm now merging before sending to the commit queue. Thanks a lot Riadh
Hi Riadh, I chatted with our PM, and because of the sensitive nature of this, we need to make sure that this feature is not available on the distribution channels (Stable, Beta, Dev, Canary) yet. Because you did not send your CL to the commit queue yet, I believe it's best to incorporate the following addition into it: in ChromePasswordManagerClient::IsAutomaticPasswordSavingEnabled, return true only if the command-line flag is there _and_ chrome::VersionInfo::GetChannel() == chrome::VersionInfo::CHANNEL_UNKNOWN. Don't forget to make also the tests aware of this (so that the tests don't fail in official builds. After that, the feature will work (behind the flag) in custom builds of Chromium, but not in the official builds. I hope we'll get approval to also allow this on Canary soon. And of course, once we add the proper user notification and launch the feature, these restrictions will be removed. If you have any questions, please feel free to ask. I'm really sorry to delay you so last-minute. Cheers, Vaclav
Hi vaclav, Could you please have a look to the new patch. Thanks a lot Cheers Riadh
Thanks, Riadh. Almost there! One important comment and a couple of nits below. Thanks, Vaclav https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:11: nit: Please remove this blank line. https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:86: chrome::VersionInfo::CHANNEL_UNKNOWN; nit: This indenting looks wrong. It should be +2 more spaces. Please make sure you run "git cl format". https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:137: nit: Please remove this blank line. https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:693: EXPECT_FALSE(observer.infobar_shown()); For official builds this needs to be EXPECT_TRUE. Please move inside the if-else statement with appropriate modifications.
Hi, Could you please check the new patch Thanks a lot Riadh
LGTM, woohoo! :)
The CQ bit was checked by rchtara@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
AWESOME !!!! :)
https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:11: On 2014/04/30 07:46:31, vabr (Chromium) wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client.cc:86: chrome::VersionInfo::CHANNEL_UNKNOWN; On 2014/04/30 07:46:31, vabr (Chromium) wrote: > nit: This indenting looks wrong. It should be +2 more spaces. Please make sure > you run "git cl format". Done. https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/chrome_password_manager_client_unittest.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/chrome_password_manager_client_unittest.cc:137: On 2014/04/30 07:46:31, vabr (Chromium) wrote: > nit: Please remove this blank line. Done. https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/256003003/diff/280001/chrome/browser/password... chrome/browser/password_manager/password_manager_browsertest.cc:693: EXPECT_FALSE(observer.infobar_shown()); On 2014/04/30 07:46:31, vabr (Chromium) wrote: > For official builds this needs to be EXPECT_TRUE. Please move inside the if-else > statement with appropriate modifications. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
On 2014/04/30 09:25:18, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_chromium_rel on tryserver.chromium Seems like a trybot flake.
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium
The CQ bit was checked by vabr@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchtara@chromium.org/256003003/300001
Message was sent while issue was closed.
Change committed as 267214 |
