Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2478043002:
HTTP Bad: Add warning message to autofill dropdown for http sites (Closed)
|
Created:
4 years, 1 month ago by lshang Modified:
4 years, 1 month ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHTTP Bad: Add warning message to autofill dropdown for http sites
In HTTP Bad project, chrome will generally warn users for http websites which
asks for credit cards and password info.
Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a
warning message "Login not secure" on top of password suggestions in autofill
popup dropdown. Will polish its styles and add icons in the following CLs.
BUG=662298, 662297
Committed: https://crrev.com/479fa58cbbb3b9234de6e53c05037afdfcf928d2
Cr-Commit-Position: refs/heads/master@{#430858}
Patch Set 1 #
Total comments: 14
Patch Set 2 : add test to AutofillManagerTest #
Total comments: 4
Patch Set 3 : reuse IsContextSecure() #Patch Set 4 : remove an unused header file #
Total comments: 21
Patch Set 5 : address review comments #Patch Set 6 : advise test #
Total comments: 6
Patch Set 7 : nit change #Messages
Total messages: 59 (35 generated)
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites include order remove unused import format change BUG= ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. This CL add a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ==========
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. This CL add a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChip, this CL add a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ==========
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChip, this CL add a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChip, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ==========
lshang@chromium.org changed reviewers: + estade@chromium.org
estade@, PTAL thanks! This CL adds a warning message on top of autofill popup. For more details, please see https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6....
vabr@chromium.org changed reviewers: + vabr@chromium.org
Hello, I left some comments, the most important being the request for a test of the new code. To spare you waiting because of the timezones, I give you my LGTM, providing the tests are added and comments addressed. If you have any questions, please ask, I'll do my best to respond quickly. Cheers, Vaclav https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:23: #include "content/public/browser/web_contents.h" Is this still needed? https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:495: if (!IsTopLevelURLSecure() && Could you please add a test for the new functionality in chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc ? It should test at least these things (ideally one per test): * There is a warning as long as the warning feature is on and the current URL is not secure. * There no warning if the warning feature is off even if the current URL is not secure. * There is no warning if the warning feature is on but the current URL is secure. * The warning is always the first among suggestions. * If there are, say, 3 real suggestions, and the warning is added, the total number of suggestions shown is 4. https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:501: suggestions_.insert(suggestions_.begin(), autofill_http_warning_suggestion); optional: Currently we copy |suggestions| into |suggestions_| and then insert one more to the front. That means that all the data from |suggestions| needs to be shifted in |suggestions_|. An alternative would be to delete line 490, first only push_back the warning suggestion, and then use std::copy(suggestions.begin(), suggestions.end(), std::inserter(suggestions_, suggestions_.end())); This comment is optional, because it is an unclear trade-off: your current code may be easier to read, the proposed change will be slightly efficient. If you considered both options and prefer the current code for its readability, then I'm fine with your choice, because this is unlikely to be a performance hotspot. https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... components/autofill_strings.grdp:177: <message name="IDS_AUTOFILL_HTTP_WARNING_MESSAGE" desc="Text shown as the top of the suggestion drop down when the form is in http sites."> nit: This might not be enough context for the translator. Let's not assume that they connect "AUTOFILL" in the message ID to what "suggestion drop down" is meant. I acknowledge that this matches the other descriptions, but let's improve at least the new one. Suggested description (feel free to reword, as long as you keep the context): "Chrome can help the user fill web forms bu showing a pop-up with text suggestions next to a focused text field. If the form is on an insecure site (e.g., http://), this text is shown on top of those suggestions."
Hah, I over-estimated my ownership over the autofill code, which does not include UI. :) In that case please see my comments only as optional suggestions, as Evan's (or Ilya's) review is both necessary and sufficient for you to land this. Sorry for the noise! Vaclav
We fill credit cards into http sites? When did that change?
mathp@chromium.org changed reviewers: + mathp@chromium.org
Hi Liu, Thanks for the contribution. However, the description does not match what is being done here. The autofill popup controller is not used for password dropdowns (it is used for credit card and address suggestions though). With your current patch, here is the result that I get when visiting http://rsolomakhin.github.io/autofill/ ** https://drive.google.com/file/d/0B3xzZ-vFr2LRWjZYaE5HbzhSN00/view?usp=sharing https://drive.google.com/file/d/0B3xzZ-vFr2LRUlk0bkhsSEU0YW8/view?usp=sharing I suggest looking at how the credit card on insecure page warning (shown in the first link) is being added in AutofillManager, and changing the logic so it adds two rows as in the mocks (and have "Payment not secure", not "Login not secure"). I also agree with Vaclav that we will need tests for this. See AutofillManagerTest, AutofillExternalDelegateTest. Thanks! ** Note that there is an https version of this site at https://rsolomakhin.github.io/autofill/ https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:404: return content::IsOriginSecure( BTW in AutofillManager, we check for the secure context in a slightly different way [1], and allow mixed passive content. Just making sure we actually want different behavior for this warning. [1] https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:488: void AutofillPopupControllerImpl::SetValues( This function doesn't seem like the ideal place to be adding entries to the popup. See the code in AutofillManager that adds the existing Credit card insecure warning here: https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... components/autofill_strings.grdp:177: <message name="IDS_AUTOFILL_HTTP_WARNING_MESSAGE" desc="Text shown as the top of the suggestion drop down when the form is in http sites."> if this is for the password popup, I would suggest making the name password specific such as IDS_AUTOFILL_PASSWORD_HTTP_WARNING_MESSAGE. The mock for credit cards is "Payment not secure"
where is the mock? Why is the current warning insufficient on its own?
Hi Evan, Liu sent these mocks to me: go/fns-ui-spec <https://goto.google.com/fns-ui-spec> On Sat, Nov 5, 2016 at 2:27 AM, <estade@chromium.org> wrote: > where is the mock? Why is the current warning insufficient on its own? > > https://codereview.chromium.org/2478043002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thanks for the link. Those mocks don't look unreasonable. I might suggest
eliminating "payment" from the second line ("autofilling disabled") since
"payment" is already present in the first string, but in any case I'm glad to
see this is on your radar and I can bow out of this review.
Hi all, I've updated the patch to show different warning messages for credit card form and password form. PTAL thanks! Tests are added as well. I'll confirm with HTTP Bad people about the message-shown condition, to see if we need to make it the same as showing the credit card autofill disabled message(i.e. use IsContextSecure()). On 2016/11/05 03:26:59, Mathieu Perreault wrote: > Hi Liu, > > Thanks for the contribution. However, the description does not match what is > being done here. The autofill popup controller is not used for password > dropdowns (it is used for credit card and address suggestions though). > > With your current patch, here is the result that I get when visiting > http://rsolomakhin.github.io/autofill/ ** > > https://drive.google.com/file/d/0B3xzZ-vFr2LRWjZYaE5HbzhSN00/view?usp=sharing > https://drive.google.com/file/d/0B3xzZ-vFr2LRUlk0bkhsSEU0YW8/view?usp=sharing > > I suggest looking at how the credit card on insecure page warning (shown in the > first link) is being added in AutofillManager, and changing the logic so it adds > two rows as in the mocks (and have "Payment not secure", not "Login not > secure"). > > I also agree with Vaclav that we will need tests for this. See > AutofillManagerTest, AutofillExternalDelegateTest. > > Thanks! > In the old patch, both credit card and password forms are displaying the warning message, since AutofillPopupControllerImpl gets called for both of them. In the updated patch, I split the "payment not secure" message and "login not secure" message, added them in two different places(AutofillManager's credit card check, and PasswordAutofillManager). Now the popup in http sites are like: https://screenshot.googleplex.com/rdfrepva1Zm https://screenshot.googleplex.com/MpyUtoC0EU1 And thanks for the test page links! Those are really helpful! https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:23: #include "content/public/browser/web_contents.h" On 2016/11/04 08:55:29, vabr (Chromium) wrote: > Is this still needed? Removed. https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:404: return content::IsOriginSecure( On 2016/11/05 03:26:59, Mathieu Perreault wrote: > BTW in AutofillManager, we check for the secure context in a slightly different > way [1], and allow mixed passive content. Just making sure we actually want > different behavior for this warning. > > [1] > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... > Good question. In my update patch, for password form, I use IsOriginSecure() as the HTTP Bad design doc suggested (https://docs.google.com/document/d/1xno6g6OnA7strcyzE-o_drevW8L0Mb6ZBEkjsiwa6...), and for credit card form, since there is already a IsContextSecure() check to show the autofill disabled message, so I added the warning message there. I'll ask Enamel people about these different check methods to confirm. https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:488: void AutofillPopupControllerImpl::SetValues( On 2016/11/05 03:26:59, Mathieu Perreault wrote: > This function doesn't seem like the ideal place to be adding entries to the > popup. See the code in AutofillManager that adds the existing Credit card > insecure warning here: > > https://cs.chromium.org/chromium/src/components/autofill/core/browser/autofil... Yeah, I add password entry in password_autofill_manager and credit card entry in autofill_manager's credit card check. Please see the updated patch:-) https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:495: if (!IsTopLevelURLSecure() && On 2016/11/04 08:55:29, vabr (Chromium) wrote: > Could you please add a test for the new functionality in > chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc ? > > It should test at least these things (ideally one per test): > * There is a warning as long as the warning feature is on and the current URL is > not secure. > * There no warning if the warning feature is off even if the current URL is not > secure. > * There is no warning if the warning feature is on but the current URL is > secure. > * The warning is always the first among suggestions. > * If there are, say, 3 real suggestions, and the warning is added, the total > number of suggestions shown is 4. Done. Tests added in autofill_manager_unittest and password_autofill_manager_unittest. https://codereview.chromium.org/2478043002/diff/1/chrome/browser/ui/autofill/... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:501: suggestions_.insert(suggestions_.begin(), autofill_http_warning_suggestion); On 2016/11/04 08:55:29, vabr (Chromium) wrote: > optional: > > Currently we copy |suggestions| into |suggestions_| and then insert one more to > the front. That means that all the data from |suggestions| needs to be shifted > in |suggestions_|. > > An alternative would be to delete line 490, first only push_back the warning > suggestion, and then use > > std::copy(suggestions.begin(), suggestions.end(), std::inserter(suggestions_, > suggestions_.end())); > > > This comment is optional, because it is an unclear trade-off: your current code > may be easier to read, the proposed change will be slightly efficient. If you > considered both options and prefer the current code for its readability, then > I'm fine with your choice, because this is unlikely to be a performance hotspot. I split the password and credit card logic into two different upper-level classes, so that warning message is added when generating suggestions, prior to calling into this AotufillPopupControllerImpl class. https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... components/autofill_strings.grdp:177: <message name="IDS_AUTOFILL_HTTP_WARNING_MESSAGE" desc="Text shown as the top of the suggestion drop down when the form is in http sites."> On 2016/11/04 08:55:29, vabr (Chromium) wrote: > nit: This might not be enough context for the translator. Let's not assume that > they connect "AUTOFILL" in the message ID to what "suggestion drop down" is > meant. > > I acknowledge that this matches the other descriptions, but let's improve at > least the new one. > > Suggested description (feel free to reword, as long as you keep the context): > > "Chrome can help the user fill web forms bu showing a pop-up with text > suggestions next to a focused text field. If the form is on an insecure site > (e.g., http://), this text is shown on top of those suggestions." Done. https://codereview.chromium.org/2478043002/diff/1/components/autofill_strings... components/autofill_strings.grdp:177: <message name="IDS_AUTOFILL_HTTP_WARNING_MESSAGE" desc="Text shown as the top of the suggestion drop down when the form is in http sites."> On 2016/11/05 03:26:59, Mathieu Perreault wrote: > if this is for the password popup, I would suggest making the name password > specific such as IDS_AUTOFILL_PASSWORD_HTTP_WARNING_MESSAGE. The mock for credit > cards is "Payment not secure" Done. Added two warning messages, one is "Login not secure", the other one is "Payment not secure".
estark@chromium.org changed reviewers: + estark@chromium.org
https://codereview.chromium.org/2478043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:578: security_state::switches::kMarkHttpWithPasswordsOrCcWithChip) { Drive-by: this should be the new switch, right? (kMarkHttpWithPasswordsOrCcWithChipAndFormWarning) https://codereview.chromium.org/2478043002/diff/40001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/40001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:215: choice == security_state::switches::kMarkHttpWithPasswordsOrCcWithChip) { Same here
https://codereview.chromium.org/2478043002/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:578: security_state::switches::kMarkHttpWithPasswordsOrCcWithChip) { On 2016/11/07 06:06:57, estark wrote: > Drive-by: this should be the new switch, right? > (kMarkHttpWithPasswordsOrCcWithChipAndFormWarning) Done. Yeah I've rebased and changed to use the new switch. https://codereview.chromium.org/2478043002/diff/40001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/40001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:215: choice == security_state::switches::kMarkHttpWithPasswordsOrCcWithChip) { On 2016/11/07 06:06:57, estark wrote: > Same here Done.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChip, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ==========
Here are more Autofill comments. This is looking much better :) Vaclav, would you review the triggering rules for Password Manager? I wouldn't want to miss a specific case. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.cc:302: void AutofillExternalDelegate::ApplyAutofillWarnings( Let's change the name and documentation of this function so that it's clearer to the reader what's going on. Let's also unit test it because the logic is becoming non trivial. in autofill_external_delegate.h // Will remove Autofill warnings from |suggestions| if there are also autocomplete entries in the vector. Note: at this point, it is assumed that if there are Autofill warnings, they will be at the head of the vector and any entry that is not an Autofill warning is considered an Autocomplete entry. void PossiblyRemoveAutofillWarnings(...) https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.cc:304: while (suggestions->size() > 1 && This works as long as POPUP_ITEM_ID_WARNING_MESSAGE is the only Autofill warning type. Consider writing a small IsAutofillWarningEntry(int frontend_id) function https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:569: Suggestion warning_suggestion(l10n_util::GetStringUTF16( Should this block be *after* your new code, to follow the order of the warnings in the dropdown? https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:574: std::string choice = Add a comment on both this block and the previous block to explain what each of the warnings are. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:577: if (choice == security_state::switches:: Please put the whole check for the flags in autofill_experiments.cc https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:579: autofill::Suggestion cc_field_http_warning_suggestion( nit: remove autofill:: https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1609: base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( Make this a function in the AutofillManagerTest class https://codereview.chromium.org/2478043002/diff/120001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill_st... components/autofill_strings.grdp:180: <message name="IDS_AUTOFILL_PASSWORD_HTTP_WARNING_MESSAGE" desc="Chrome can help the user fill password web forms by showing a pop-up with text suggestions next to a focused password text field. If the form is on an insecure site(e.g., http://), this text is shown on top of those suggestions."> nit: missing a space in "site(e.g.,"
Thanks for notifying me. password_manager code LGTM, and the triggering condition in PasswordManager seems to match the mocks from go/fns-ui-spec. Cheers, Vaclav https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:582: std::unique_ptr<TestPasswordManagerClient> client( nit: Please use auto client = base::MakeUnique<TestPasswordManagerClient>() because it does not duplicate the type name. Also with unique_ptrs below. https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:674: // Http warning message won't show for secure context, even with switch flag Just for completeness, could we also have a test for when the context is not secure but the flag is off? At that point you might want to merge all the common work of the tests into a single preparation method and call that, followed by the correct expectation verification, from individual tests.
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.cc:302: void AutofillExternalDelegate::ApplyAutofillWarnings( On 2016/11/07 14:32:27, Mathieu Perreault wrote: > Let's change the name and documentation of this function so that it's clearer to > the reader what's going on. Let's also unit test it because the logic is > becoming non trivial. > > in autofill_external_delegate.h > // Will remove Autofill warnings from |suggestions| if there are also > autocomplete entries in the vector. Note: at this point, it is assumed that if > there are Autofill warnings, they will be at the head of the vector and any > entry that is not an Autofill warning is considered an Autocomplete entry. > void PossiblyRemoveAutofillWarnings(...) Done. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.cc:304: while (suggestions->size() > 1 && On 2016/11/07 14:32:27, Mathieu Perreault wrote: > This works as long as POPUP_ITEM_ID_WARNING_MESSAGE is the only Autofill warning > type. Consider writing a small IsAutofillWarningEntry(int frontend_id) function > Done. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:569: Suggestion warning_suggestion(l10n_util::GetStringUTF16( On 2016/11/07 14:32:27, Mathieu Perreault wrote: > Should this block be *after* your new code, to follow the order of the warnings > in the dropdown? This insecure connection warning message will first replace the original suggestion content and modify the vector size to 1, then "Payment not secure" messages below will be inserted on top of it. We could change their order to let "Payment not secure" do the replacement and make insecure connection warning message pushed back to the vector, which might looks clearer? Or we can make it clearer in the comments. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:574: std::string choice = On 2016/11/07 14:32:27, Mathieu Perreault wrote: > Add a comment on both this block and the previous block to explain what each of > the warnings are. Done. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:577: if (choice == security_state::switches:: On 2016/11/07 14:32:27, Mathieu Perreault wrote: > Please put the whole check for the flags in autofill_experiments.cc Done. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:579: autofill::Suggestion cc_field_http_warning_suggestion( On 2016/11/07 14:32:27, Mathieu Perreault wrote: > nit: remove autofill:: Done. Also with the autofill::POPUP_ITEM_ID_WARNING_MESSAGE below. https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1609: base::CommandLine::ForCurrentProcess()->AppendSwitchASCII( On 2016/11/07 14:32:27, Mathieu Perreault wrote: > Make this a function in the AutofillManagerTest class Done. https://codereview.chromium.org/2478043002/diff/120001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2478043002/diff/120001/components/autofill_st... components/autofill_strings.grdp:180: <message name="IDS_AUTOFILL_PASSWORD_HTTP_WARNING_MESSAGE" desc="Chrome can help the user fill password web forms by showing a pop-up with text suggestions next to a focused password text field. If the form is on an insecure site(e.g., http://), this text is shown on top of those suggestions."> On 2016/11/07 14:32:27, Mathieu Perreault wrote: > nit: missing a space in "site(e.g.," Done. https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:582: std::unique_ptr<TestPasswordManagerClient> client( On 2016/11/07 14:57:27, vabr (Chromium) wrote: > nit: Please use > > auto client = base::MakeUnique<TestPasswordManagerClient>() > > because it does not duplicate the type name. > > Also with unique_ptrs below. Done. https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:674: // Http warning message won't show for secure context, even with switch flag On 2016/11/07 14:57:27, vabr (Chromium) wrote: > Just for completeness, could we also have a test for when the context is not > secure but the flag is off? > > At that point you might want to merge all the common work of the tests into a > single preparation method and call that, followed by the correct expectation > verification, from individual tests. We have that test in the first test I added, when context is not secure and flag is off, only shows title and username, and when flag is on, warning message shows up.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM. https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:674: // Http warning message won't show for secure context, even with switch flag On 2016/11/08 06:00:58, lshang wrote: > On 2016/11/07 14:57:27, vabr (Chromium) wrote: > > Just for completeness, could we also have a test for when the context is not > > secure but the flag is off? > > > > At that point you might want to merge all the common work of the tests into a > > single preparation method and call that, followed by the correct expectation > > verification, from individual tests. > > We have that test in the first test I added, when context is not secure and flag > is off, only shows title and username, and when flag is on, warning message > shows up. Ah, right, I missed that without-flag part in both tests. Sorry for the noise!
lgtm with nits https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_external_delegate.h (right): https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.h:123: bool IsAutofillWarningEntry(int frontend_id); this can be in the anonymous namespace of the cc file. https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1014: void set_http_warning_enabled() { Use CamelCaseNaming() https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1618: // Set up our form data. indent is off here
Thanks all! https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_external_delegate.h (right): https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_external_delegate.h:123: bool IsAutofillWarningEntry(int frontend_id); On 2016/11/08 17:26:17, Mathieu Perreault wrote: > this can be in the anonymous namespace of the cc file. Done. https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1014: void set_http_warning_enabled() { On 2016/11/08 17:26:17, Mathieu Perreault wrote: > Use CamelCaseNaming() Done. https://codereview.chromium.org/2478043002/diff/160001/components/autofill/co... components/autofill/core/browser/autofill_manager_unittest.cc:1618: // Set up our form data. On 2016/11/08 17:26:17, Mathieu Perreault wrote: > indent is off here Done. Don't know why this weird indent happens, probably formatting got wrong:P
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2478043002/#ps180001 (title: "nit change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Hi estark@, there's presubmit errors saying:
You need LGTM from owners of depends-on paths in DEPS that were modified in this
CL:
'+components/security_state',
Could you take a look?
added DEPs lgtm
The CQ bit was checked by lshang@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 ========== to ========== HTTP Bad: Add warning message to autofill dropdown for http sites In HTTP Bad project, chrome will generally warn users for http websites which asks for credit cards and password info. Guarded by a switch flag kMarkHttpWithPasswordsOrCcWithChipAndFormWarning, this CL adds a warning message "Login not secure" on top of password suggestions in autofill popup dropdown. Will polish its styles and add icons in the following CLs. BUG=662298, 662297 Committed: https://crrev.com/479fa58cbbb3b9234de6e53c05037afdfcf928d2 Cr-Commit-Position: refs/heads/master@{#430858} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/479fa58cbbb3b9234de6e53c05037afdfcf928d2 Cr-Commit-Position: refs/heads/master@{#430858} |
