Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1473733008:
[Autofill] Respect the autocomplete=off attribute. (Closed)
|
Created:
5 years ago by sebsg Modified:
5 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms.
BUG=468153
Committed: https://crrev.com/570de6587cd3a56da12d5b234e97ea1e31355f45
Cr-Commit-Position: refs/heads/master@{#363052}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Added password test #Patch Set 4 : Rebase #Patch Set 5 : Fixed mac error #Patch Set 6 : Fixed the fact that <select> always had should_autocomplete = false #Patch Set 7 : Rebase #Messages
Total messages: 59 (28 generated)
Patchset #1 (id:1) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Here is the small version. Thanks for reviewing!
https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:412: // fields that have the autocomplete attribute set to false. false -> off https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3958: // fields if the form has the autocomplete attribute set to off. fix comment. "if the initiating field has autocomplete set to off." https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3959: TEST_F(AutofillManagerTest, DisplaySuggestions_AutocompleteOffOnField) { Let's have this test be non-credit card only, and have a mix of autocomplete=off fields and not, and see that it matters. Let's have a separate test for credit card fields with autocomplete=off fields and not, and see that it doesn't matter.
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Thanks for your comments! https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:412: // fields that have the autocomplete attribute set to false. On 2015/11/27 16:14:33, Mathieu Perreault wrote: > false -> off Done. https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3958: // fields if the form has the autocomplete attribute set to off. On 2015/11/27 16:14:33, Mathieu Perreault wrote: > fix comment. "if the initiating field has autocomplete set to off." Done. https://codereview.chromium.org/1473733008/diff/20001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3959: TEST_F(AutofillManagerTest, DisplaySuggestions_AutocompleteOffOnField) { On 2015/11/27 16:14:33, Mathieu Perreault wrote: > Let's have this test be non-credit card only, and have a mix of autocomplete=off > fields and not, and see that it matters. > > Let's have a separate test for credit card fields with autocomplete=off fields > and not, and see that it doesn't matter. Done.
Description was changed from ========== [Autofill] Autocomplete WIP BUG= ========== to ========== [Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms. BUG=468153 ==========
sebsg@chromium.org changed reviewers: + vabr@chromium.org
Hi vabr, Could you please take a look at this CL? Thanks!
Hi sebsg@, The CL LGTM with one request and the comments below: Could you please add a test to chrome/browser/password_manager/password_manager_browsertest.cc checking that also passwords are ignoring autocomplete=off? Interestingly, we have the HTML page to test it (chrome/test/data/password/password_autocomplete_off_test.html) but the corresponding test disappeared at some point. :) Thanks! Vaclav https://codereview.chromium.org/1473733008/diff/80001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2096: for (FormFieldData field : form.fields) { nit: const FormFieldData& https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:133: // Field has autocomplete="off" set. It is still fillable. nit: Perhaps mention the reason. I expect this is because the autocomplete attribute only affects autocomplete and not autofill? https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3958: // fields if the initiating field field has the "autocomplete" attribute set to typo: field field https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:4020: for (size_t i = 0U; i < mixed_form.fields.size(); ++i) { nit: Could this be a range-based for-loop?
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/100001
Thanks for you comments! Here is the updated version. https://codereview.chromium.org/1473733008/diff/80001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:2096: for (FormFieldData field : form.fields) { On 2015/11/30 16:26:09, vabr (Chromium) wrote: > nit: const FormFieldData& Done. https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_field_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_field_unittest.cc:133: // Field has autocomplete="off" set. It is still fillable. On 2015/11/30 16:26:09, vabr (Chromium) wrote: > nit: Perhaps mention the reason. I expect this is because the autocomplete > attribute only affects autocomplete and not autofill? Done. https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3958: // fields if the initiating field field has the "autocomplete" attribute set to On 2015/11/30 16:26:09, vabr (Chromium) wrote: > typo: field field Done. https://codereview.chromium.org/1473733008/diff/80001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:4020: for (size_t i = 0U; i < mixed_form.fields.size(); ++i) { On 2015/11/30 16:26:09, vabr (Chromium) wrote: > nit: Could this be a range-based for-loop? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #6 (id:160001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/180001
sebsg@chromium.org changed reviewers: + pdr@chromium.org
Hi pdr@, Could you please review the change in WebFormControlElement.cpp? Thanks a lot!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/180001
On 2015/12/02 at 21:59:17, sebsg wrote: > Hi pdr@, > > Could you please review the change in WebFormControlElement.cpp? > > Thanks a lot! The WebFormControlElement.cpp change looks fine but I'm skeptical of these IsDesktopPlatform calls. Why are we introducing differences between desktop and non-desktop?
On 2015/12/02 at 22:23:09, pdr wrote: > On 2015/12/02 at 21:59:17, sebsg wrote: > > Hi pdr@, > > > > Could you please review the change in WebFormControlElement.cpp? > > > > Thanks a lot! > LGTM for WebFormControlElement > The WebFormControlElement.cpp change looks fine but I'm skeptical of these IsDesktopPlatform calls. Why are we introducing differences between desktop and non-desktop? sebsg forwarded me a discussion about this and it makes sense. Can we rename IsDesktopPlatform to "SupportsIME" (or similar) and add a comment about why IME solves our issue on mobile?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/12/02 22:33:34, pdr wrote: > On 2015/12/02 at 22:23:09, pdr wrote: > > On 2015/12/02 at 21:59:17, sebsg wrote: > > > Hi pdr@, > > > > > > Could you please review the change in WebFormControlElement.cpp? > > > > > > Thanks a lot! > > > > LGTM for WebFormControlElement > > > > The WebFormControlElement.cpp change looks fine but I'm skeptical of these > IsDesktopPlatform calls. Why are we introducing differences between desktop and > non-desktop? > > sebsg forwarded me a discussion about this and it makes sense. Can we rename > IsDesktopPlatform to "SupportsIME" (or similar) and add a comment about why IME > solves our issue on mobile? Since we still want to autofill for mobile users who have the keyboard accessory turned off for some reason, I would rather keep the name IsOnDesktop for the moment of that's ok with you.
On 2015/12/03 at 20:21:47, sebsg wrote: > On 2015/12/02 22:33:34, pdr wrote: > > On 2015/12/02 at 22:23:09, pdr wrote: > > > On 2015/12/02 at 21:59:17, sebsg wrote: > > > > Hi pdr@, > > > > > > > > Could you please review the change in WebFormControlElement.cpp? > > > > > > > > Thanks a lot! > > > > > > > LGTM for WebFormControlElement > > > > > > > The WebFormControlElement.cpp change looks fine but I'm skeptical of these > > IsDesktopPlatform calls. Why are we introducing differences between desktop and > > non-desktop? > > > > sebsg forwarded me a discussion about this and it makes sense. Can we rename > > IsDesktopPlatform to "SupportsIME" (or similar) and add a comment about why IME > > solves our issue on mobile? > > Since we still want to autofill for mobile users who have the keyboard accessory turned off for some reason, I would rather keep the name IsOnDesktop for the moment of that's ok with you. So you would return true for IsOnDesktop for mobile users with the keyboard accessory turned off? I still think it is best to name this function for what it's really doing--maybe even PlatformSupportsKeyboardAutofill. I'm now pretty far into bikeshed territory though, and the name here is not a huge issue either way. I'm okay with you landing this as-is.
On 2015/12/03 20:29:30, pdr wrote: > On 2015/12/03 at 20:21:47, sebsg wrote: > > On 2015/12/02 22:33:34, pdr wrote: > > > On 2015/12/02 at 22:23:09, pdr wrote: > > > > On 2015/12/02 at 21:59:17, sebsg wrote: > > > > > Hi pdr@, > > > > > > > > > > Could you please review the change in WebFormControlElement.cpp? > > > > > > > > > > Thanks a lot! > > > > > > > > > > LGTM for WebFormControlElement > > > > > > > > > > The WebFormControlElement.cpp change looks fine but I'm skeptical of these > > > IsDesktopPlatform calls. Why are we introducing differences between desktop > and > > > non-desktop? > > > > > > sebsg forwarded me a discussion about this and it makes sense. Can we rename > > > IsDesktopPlatform to "SupportsIME" (or similar) and add a comment about why > IME > > > solves our issue on mobile? > > > > Since we still want to autofill for mobile users who have the keyboard > accessory turned off for some reason, I would rather keep the name IsOnDesktop > for the moment of that's ok with you. > > So you would return true for IsOnDesktop for mobile users with the keyboard > accessory turned off? I still think it is best to name this function for what > it's really doing--maybe even PlatformSupportsKeyboardAutofill. I'm now pretty > far into bikeshed territory though, and the name here is not a huge issue either > way. I'm okay with you landing this as-is. Sorry, I think my last message was unclear... IsOnDesktop would always return false on mobile. That way we would always autofill forms on mobile regardless of the presence of the keyboard accessory. That's why I think IsOnDesktop is a good name: The behavior is different on desktop vs mobile (always autofill on mobile). The behavior is not always dependent on the presence of keyboard accessory. Does that make sense?
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1473733008/#ps200001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1473733008/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1473733008/200001
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms. BUG=468153 ========== to ========== [Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms. BUG=468153 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms. BUG=468153 ========== to ========== [Autofill] Respect the autocomplete=off attribute on desktop for non credit card related fields and forms. BUG=468153 Committed: https://crrev.com/570de6587cd3a56da12d5b234e97ea1e31355f45 Cr-Commit-Position: refs/heads/master@{#363052} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/570de6587cd3a56da12d5b234e97ea1e31355f45 Cr-Commit-Position: refs/heads/master@{#363052} |
