Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1411363003:
[Autofill] Always show available data when encountering autocomplete attributes (Closed)
|
Created:
5 years, 2 months ago by sebsg Modified:
5 years ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jam, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_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] Always show available data when encountering autocomplete attributes
This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled.
As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled.
Changes General Description:
Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions.
Form_structure: ShouldBeParsed will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute.
Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form.
In OnQueryFormFieldAutofill use ShouldBeParsed instead of Autofillable to make sure to send suggestions for small forms with autocomplete attribute(s)
BUG=541681
Committed: https://crrev.com/6b4ba4ea43e17b16e59ef8f8b416226515ac14f1
Cr-Commit-Position: refs/heads/master@{#361460}
Patch Set 1 #Patch Set 2 : Added test #
Total comments: 12
Patch Set 3 : Addressed mathp's comments #
Total comments: 12
Patch Set 4 : Rebase #Patch Set 5 : Addressed comments and added tests #Patch Set 6 : Addressed comments #Patch Set 7 : Rebase #
Total comments: 20
Patch Set 8 : Addressed comments #
Total comments: 10
Patch Set 9 : Addressed comments #
Total comments: 4
Patch Set 10 : Rebase #Patch Set 11 : Addressed comments #Patch Set 12 : Rebase #Patch Set 13 : Fixed bug #Messages
Total messages: 65 (32 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Can you take a look when you get the chance? Thanks!
Description was changed from ========== [Autofill] Always show available data when encountering autocomplete attributes BUG=541681 ========== to ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. BUG=541681 ==========
https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... components/autofill/content/renderer/form_cache.cc:67: return (num_editable_elements < 1 && num_control_elements > 0); !num_editable_elements && num_control_elements https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... components/autofill/content/renderer/form_cache.cc:119: if (form.fields.size() >= 1 && !ContainsKey(parsed_forms_, form)) { !form.fields.empty() same with change below https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1528: form.active_field_count() >= kRequiredAutofillFields) reverse and add comment on why it's redundant https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure.cc:1196: was_parsed_for_autocomplete_attributes_ = true; should this go one line down, outside the loop? https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure.h (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure.h:275: // author, via the |autocompletetype| attribute. nit: "via the autocomplete attribute" https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure_unittest.cc:313: // The form has two fields but one has the autocomplete attribute Please make a separate test case
Patchset #3 (id:80001) has been deleted
Description was changed from ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. BUG=541681 ========== to ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. Changes Description: Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions. Form_structure: ShouldParse will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute. Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form. BUG=541681 ==========
Description was changed from ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. Changes Description: Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions. Form_structure: ShouldParse will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute. Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form. BUG=541681 ========== to ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. Changes General Description: Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions. Form_structure: ShouldParse will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute. Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form. BUG=541681 ==========
Description was changed from ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. Changes General Description: Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions. Form_structure: ShouldParse will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute. Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form. BUG=541681 ========== to ========== [Autofill] Always show available data when encountering autocomplete attributes This will specifically alter behavior for forms that have 1-2 fields and which specify autocomplete attributes. Those will now be autofilled. As per existing behavior, if a form is partially marked up with autocomplete attributes, non-marked fields are not parsed or autofilled. Changes General Description: Form_cache: Change the limit of fields / editable elements from 3 to 1. Otherwise the manager will not receive the form at all and won't be able to make suggestions. Form_structure: ShouldBeParsed will now return true for forms with less than 3 fields if there is at least one field with an autocomplete attribute. Autofill_manager: In ParseForms, the ParseFieldTypesFromAutocompleteAttributes is called before the check to ShouldParse to detect autocomplete attributes in the form. In OnQueryFormFieldAutofill use ShouldBeParsed instead of Autofillable to make sure to send suggestions for small forms with autocomplete attribute(s) BUG=541681 ==========
https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... components/autofill/content/renderer/form_cache.cc:67: return (num_editable_elements < 1 && num_control_elements > 0); On 2015/10/20 21:03:10, Mathieu Perreault wrote: > !num_editable_elements && num_control_elements > Done. https://codereview.chromium.org/1411363003/diff/60001/components/autofill/con... components/autofill/content/renderer/form_cache.cc:119: if (form.fields.size() >= 1 && !ContainsKey(parsed_forms_, form)) { On 2015/10/20 21:03:10, Mathieu Perreault wrote: > !form.fields.empty() > > > same with change below Done. https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:1528: form.active_field_count() >= kRequiredAutofillFields) On 2015/10/20 21:03:10, Mathieu Perreault wrote: > reverse and add comment on why it's redundant Done. https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure.cc:1196: was_parsed_for_autocomplete_attributes_ = true; On 2015/10/20 21:03:11, Mathieu Perreault wrote: > should this go one line down, outside the loop? Done. https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure.h (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure.h:275: // author, via the |autocompletetype| attribute. On 2015/10/20 21:03:11, Mathieu Perreault wrote: > nit: "via the autocomplete attribute" Done. https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... File components/autofill/core/browser/form_structure_unittest.cc (right): https://codereview.chromium.org/1411363003/diff/60001/components/autofill/cor... components/autofill/core/browser/form_structure_unittest.cc:313: // The form has two fields but one has the autocomplete attribute On 2015/10/20 21:03:11, Mathieu Perreault wrote: > Please make a separate test case Done.
The CQ bit was checked by mathp@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/1411363003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411363003/100001
mathp@chromium.org changed reviewers: + estade@chromium.org
lgtm Evan please have a look.
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...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/10/21 19:02:12, commit-bot: I haz the power wrote: > 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...) > mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) actually I would like to see more tests for this, namely that autofill suggestions are now shown for 1-2 field forms that contain autocomplete.
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && Why are we changing the point at which forms might get filtered out based on size? Seems like you should be using ShouldBeParsed to filter things out here. And ShouldBeParsed is a weird name given that you have to parse the form to answer the question... https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1530: if (!form.ShouldBeParsed() && shouldn't this be an or not an and?
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/21 19:12:31, Evan Stade wrote: > Why are we changing the point at which forms might get filtered out based on > size? Seems like you should be using ShouldBeParsed to filter things out here. > > And ShouldBeParsed is a weird name given that you have to parse the form to > answer the question... This is the point where we filtered out forms with less than 3 fields before they reached the browser. These fields didn't get to the autofill_manager before, but now we need to allow them to reach it so they can be parsed for autocomplete attribute and so we can make some suggestions. So I'm not sure I understand what you mean. Do you mean converting to a form_structure, calling ParseFieldTypesFromAutocompleteAttributes then calling ShouldBeParsed here? https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1530: if (!form.ShouldBeParsed() && On 2015/10/21 19:12:31, Evan Stade wrote: > shouldn't this be an or not an and? Done.
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/26 15:57:55, sebsg wrote: > On 2015/10/21 19:12:31, Evan Stade wrote: > > Why are we changing the point at which forms might get filtered out based on > > size? Seems like you should be using ShouldBeParsed to filter things out here. > > > > And ShouldBeParsed is a weird name given that you have to parse the form to > > answer the question... > > This is the point where we filtered out forms with less than 3 fields before > they reached the browser. These fields didn't get to the autofill_manager > before, but now we need to allow them to reach it so they can be parsed for > autocomplete attribute and so we can make some suggestions. only if they actually have that attribute > So I'm not sure I > understand what you mean. Do you mean converting to a form_structure, calling > ParseFieldTypesFromAutocompleteAttributes then calling ShouldBeParsed here? something like that https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1530: if (!form.ShouldBeParsed() && On 2015/10/26 15:57:56, sebsg wrote: > On 2015/10/21 19:12:31, Evan Stade wrote: > > shouldn't this be an or not an and? > > Done. does one of the tests you added verify the correct behavior of this conditional?
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/27 17:08:37, Evan Stade wrote: > On 2015/10/26 15:57:55, sebsg wrote: > > On 2015/10/21 19:12:31, Evan Stade wrote: > > > Why are we changing the point at which forms might get filtered out based on > > > size? Seems like you should be using ShouldBeParsed to filter things out > here. > > > > > > And ShouldBeParsed is a weird name given that you have to parse the form to > > > answer the question... > > > > This is the point where we filtered out forms with less than 3 fields before > > they reached the browser. These fields didn't get to the autofill_manager > > before, but now we need to allow them to reach it so they can be parsed for > > autocomplete attribute and so we can make some suggestions. > > only if they actually have that attribute > > > So I'm not sure I > > understand what you mean. Do you mean converting to a form_structure, calling > > ParseFieldTypesFromAutocompleteAttributes then calling ShouldBeParsed here? > > something like that I don't think it makes sense to move FormStructure and its associated parsing to the renderer, at least for this trivial change. If anything I think the password team is working towards moving their business logic to the browser, with the renderer only handling <form>-to-FormData/PasswordForm. It makes sense for the renderer code to be the link between WebKit and the browser, but not much more. Here, you can have a single function that looks at the FormData and determines if it's interesting enough to send to the browser. I'm bad with naming, but it does sounds like it should be called something like ShouldBeParsed, with the conditions that it has at least 3 fields or fields with autocomplete attributes.
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && > if it's interesting enough to send to the browser. I'm bad with naming, IsFormInteresting?
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/27 17:08:37, Evan Stade (ooo) wrote: > On 2015/10/26 15:57:55, sebsg wrote: > > On 2015/10/21 19:12:31, Evan Stade wrote: > > > Why are we changing the point at which forms might get filtered out based on > > > size? Seems like you should be using ShouldBeParsed to filter things out > here. > > > > > > And ShouldBeParsed is a weird name given that you have to parse the form to > > > answer the question... > > > > This is the point where we filtered out forms with less than 3 fields before > > they reached the browser. These fields didn't get to the autofill_manager > > before, but now we need to allow them to reach it so they can be parsed for > > autocomplete attribute and so we can make some suggestions. > > only if they actually have that attribute > > > So I'm not sure I > > understand what you mean. Do you mean converting to a form_structure, calling > > ParseFieldTypesFromAutocompleteAttributes then calling ShouldBeParsed here? > > something like that Acknowledged. https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/28 12:50:48, Mathieu Perreault wrote: > On 2015/10/27 17:08:37, Evan Stade wrote: > > On 2015/10/26 15:57:55, sebsg wrote: > > > On 2015/10/21 19:12:31, Evan Stade wrote: > > > > Why are we changing the point at which forms might get filtered out based > on > > > > size? Seems like you should be using ShouldBeParsed to filter things out > > here. > > > > > > > > And ShouldBeParsed is a weird name given that you have to parse the form > to > > > > answer the question... > > > > > > This is the point where we filtered out forms with less than 3 fields before > > > they reached the browser. These fields didn't get to the autofill_manager > > > before, but now we need to allow them to reach it so they can be parsed for > > > autocomplete attribute and so we can make some suggestions. > > > > only if they actually have that attribute > > > > > So I'm not sure I > > > understand what you mean. Do you mean converting to a form_structure, > calling > > > ParseFieldTypesFromAutocompleteAttributes then calling ShouldBeParsed here? > > > > something like that > > I don't think it makes sense to move FormStructure and its associated parsing to > the renderer, at least for this trivial change. If anything I think the password > team is working towards moving their business logic to the browser, with the > renderer only handling <form>-to-FormData/PasswordForm. It makes sense for the > renderer code to be the link between WebKit and the browser, but not much more. > > Here, you can have a single function that looks at the FormData and determines > if it's interesting enough to send to the browser. I'm bad with naming, but it > does sounds like it should be called something like ShouldBeParsed, with the > conditions that it has at least 3 fields or fields with autocomplete attributes. > Acknowledged. https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:120: if (form.fields.size() >= kRequiredAutofillFields && On 2015/10/28 18:51:53, Evan Stade (ooo) wrote: > > if it's interesting enough to send to the browser. I'm bad with naming, > > IsFormInteresting? Done.
https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:68: num_control_elements > 0); prefer this way of checking against 0 rather than implicit cast to bool https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:63: // allowable fields. The corresponding maximum number of allowable fields that minimum number is now one, so this comment needs updating https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:64: // is imposed by WebFormElementToFormData(). I think this comment needs further updating because I can't grasp the logic of why you require there to be control elements in order to ignore the form https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:67: return (!num_editable_elements && num_control_elements); nit: remove excess parens https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:312: // autocomplete suggestions will be shown. this comment is misleading, we don't know if suggestions will be shown or not https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.h (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.h:53: static bool IsFormInteresting(FormData form); this doesn't belong here because it's not actually used anywhere besides form_Cache.cc. Put it in the .cc file in an anonymous namespace. You should still be able to exercise it in unit tests. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredAutofillFields) kRequiredAutofillFields needs updating (its comment as well as its name) https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredAutofillFields) nit: Curlies https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/form_structure.cc:747: !has_author_specified_types_) curlies https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/form_structure.cc:1083: it != fields_.end(); ++it) { why did you change teh format of this loop
Patchset #8 (id:240001) has been deleted
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #8 (id:300001) has been deleted
Thanks for your comments. Could you take another look when you get the chance? Thanks! https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1530: if (!form.ShouldBeParsed() && On 2015/10/27 17:08:37, Evan Stade wrote: > On 2015/10/26 15:57:56, sebsg wrote: > > On 2015/10/21 19:12:31, Evan Stade wrote: > > > shouldn't this be an or not an and? > > > > Done. > > does one of the tests you added verify the correct behavior of this conditional? Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (left): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:68: num_control_elements > 0); On 2015/11/04 17:15:18, Evan Stade wrote: > prefer this way of checking against 0 rather than implicit cast to bool Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:63: // allowable fields. The corresponding maximum number of allowable fields On 2015/11/04 17:15:18, Evan Stade (ooo) wrote: > that minimum number is now one, so this comment needs updating Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:64: // is imposed by WebFormElementToFormData(). On 2015/11/04 17:15:18, Evan Stade (ooo) wrote: > I think this comment needs further updating because I can't grasp the logic of > why you require there to be control elements in order to ignore the form As I read the code more I realized that num_editable_elements is a subset of num_control_elements (the editable ones) which is really the number of autofillable fields. Talked to isherman@ and it was used with has_skipped_forms https://chromiumcodereview.appspot.com/13831013 which is no longer there. So I removed it. DO you know of a reason why it would still be useful? https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:67: return (!num_editable_elements && num_control_elements); On 2015/11/04 17:15:18, Evan Stade wrote: > nit: remove excess parens Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:312: // autocomplete suggestions will be shown. On 2015/11/04 17:15:18, Evan Stade wrote: > this comment is misleading, we don't know if suggestions will be shown or not Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/content/renderer/form_cache.h (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/content/renderer/form_cache.h:53: static bool IsFormInteresting(FormData form); On 2015/11/04 17:15:18, Evan Stade wrote: > this doesn't belong here because it's not actually used anywhere besides > form_Cache.cc. Put it in the .cc file in an anonymous namespace. You should > still be able to exercise it in unit tests. Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredAutofillFields) On 2015/11/04 17:15:19, Evan Stade wrote: > nit: Curlies Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredAutofillFields) On 2015/11/04 17:15:19, Evan Stade wrote: > kRequiredAutofillFields needs updating (its comment as well as its name) Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... File components/autofill/core/browser/form_structure.cc (right): https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/form_structure.cc:747: !has_author_specified_types_) On 2015/11/04 17:15:19, Evan Stade wrote: > curlies Done. https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... components/autofill/core/browser/form_structure.cc:1083: it != fields_.end(); ++it) { On 2015/11/04 17:15:19, Evan Stade wrote: > why did you change teh format of this loop Bad merge on PS#4, good catch!
On 2015/11/10 19:04:51, sebsg wrote: > Thanks for your comments. Could you take another look when you get the chance? > Thanks! > > https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/1411363003/diff/100001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:1530: if > (!form.ShouldBeParsed() && > On 2015/10/27 17:08:37, Evan Stade wrote: > > On 2015/10/26 15:57:56, sebsg wrote: > > > On 2015/10/21 19:12:31, Evan Stade wrote: > > > > shouldn't this be an or not an and? > > > > > > Done. > > > > does one of the tests you added verify the correct behavior of this > conditional? > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > File components/autofill/content/renderer/form_cache.cc (left): > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.cc:68: num_control_elements > > 0); > On 2015/11/04 17:15:18, Evan Stade wrote: > > prefer this way of checking against 0 rather than implicit cast to bool > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > File components/autofill/content/renderer/form_cache.cc (right): > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.cc:63: // allowable fields. The > corresponding maximum number of allowable fields > On 2015/11/04 17:15:18, Evan Stade (ooo) wrote: > > that minimum number is now one, so this comment needs updating > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.cc:64: // is imposed by > WebFormElementToFormData(). > On 2015/11/04 17:15:18, Evan Stade (ooo) wrote: > > I think this comment needs further updating because I can't grasp the logic of > > why you require there to be control elements in order to ignore the form > > As I read the code more I realized that num_editable_elements is a subset of > num_control_elements (the editable ones) which is really the number of > autofillable fields. Talked to isherman@ and it was used with has_skipped_forms > https://chromiumcodereview.appspot.com/13831013 which is no longer there. So I > removed it. DO you know of a reason why it would still be useful? > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.cc:67: return > (!num_editable_elements && num_control_elements); > On 2015/11/04 17:15:18, Evan Stade wrote: > > nit: remove excess parens > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.cc:312: // autocomplete > suggestions will be shown. > On 2015/11/04 17:15:18, Evan Stade wrote: > > this comment is misleading, we don't know if suggestions will be shown or not > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > File components/autofill/content/renderer/form_cache.h (right): > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/content/renderer/form_cache.h:53: static bool > IsFormInteresting(FormData form); > On 2015/11/04 17:15:18, Evan Stade wrote: > > this doesn't belong here because it's not actually used anywhere besides > > form_Cache.cc. Put it in the .cc file in an anonymous namespace. You should > > still be able to exercise it in unit tests. > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > File components/autofill/core/browser/autofill_manager.cc (right): > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:818: > form.active_field_count() < kRequiredAutofillFields) > On 2015/11/04 17:15:19, Evan Stade wrote: > > nit: Curlies > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/core/browser/autofill_manager.cc:818: > form.active_field_count() < kRequiredAutofillFields) > On 2015/11/04 17:15:19, Evan Stade wrote: > > kRequiredAutofillFields needs updating (its comment as well as its name) > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > File components/autofill/core/browser/form_structure.cc (right): > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/core/browser/form_structure.cc:747: > !has_author_specified_types_) > On 2015/11/04 17:15:19, Evan Stade wrote: > > curlies > > Done. > > https://codereview.chromium.org/1411363003/diff/220001/components/autofill/co... > components/autofill/core/browser/form_structure.cc:1083: it != fields_.end(); > ++it) { > On 2015/11/04 17:15:19, Evan Stade wrote: > > why did you change teh format of this loop > > Bad merge on PS#4, good catch! Friendly ping.
https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:64: bool IsFormInteresting(FormData form) { const FormData& ? https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/core/common/autofill_constants.h (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/core/common/autofill_constants.h:17: // The number of fields required by Autofill to execute it's heuristic and nit: *its
https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:70: for (auto field : form.fields) { const auto& field ?
https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:70: for (auto field : form.fields) { On 2015/11/17 18:27:11, Mathieu Perreault wrote: > const auto& field ? please don't use auto for simple types like this, it just reduces readability. You can and should use auto for complicated iterator types. https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredFieldsForPredictionRoutines) { nit: this function now looks like if (a) return false; if (b) return false; if (c || d) return false; which is oddly inconsistent. Why not return IsAutofillEnabled() && !driver_->IsOffTheRecord() && form.ShouldBeParsed() && form.active_field_count() >= kRequiredFieldsForPredictionRoutines; Also I don't get why you would want to autofill a small form with autocomplete attributes, but wouldn't want to upload that form. But if that does make sense, your reuse of kRequiredFieldsForPredictionRoutines is the opposite of what you'd expect because you're only using it to filter out some small non-prediction forms. You should create a different constant called kRequiredFieldsForUpload or something (which might just happen to have the same value as kRequiredFieldForPredictionRoutines, or might not).
Thanks for your comments https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/content/renderer/form_cache.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:64: bool IsFormInteresting(FormData form) { On 2015/11/17 15:45:06, Mathieu Perreault wrote: > const FormData& ? Done. https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:70: for (auto field : form.fields) { On 2015/11/17 19:13:36, Evan Stade wrote: > On 2015/11/17 18:27:11, Mathieu Perreault wrote: > > const auto& field ? > > please don't use auto for simple types like this, it just reduces readability. > You can and should use auto for complicated iterator types. Done. https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/content/renderer/form_cache.cc:70: for (auto field : form.fields) { On 2015/11/17 18:27:11, Mathieu Perreault wrote: > const auto& field ? Acknowledged. https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:818: form.active_field_count() < kRequiredFieldsForPredictionRoutines) { On 2015/11/17 19:13:36, Evan Stade wrote: > nit: this function now looks like > > if (a) > return false; > > if (b) > return false; > > if (c || d) > return false; > > which is oddly inconsistent. Why not > > return IsAutofillEnabled() && !driver_->IsOffTheRecord() && > form.ShouldBeParsed() && form.active_field_count() >= > kRequiredFieldsForPredictionRoutines; > > Also I don't get why you would want to autofill a small form with autocomplete > attributes, but wouldn't want to upload that form. But if that does make sense, > your reuse of kRequiredFieldsForPredictionRoutines is the opposite of what you'd > expect because you're only using it to filter out some small non-prediction > forms. You should create a different constant called kRequiredFieldsForUpload or > something (which might just happen to have the same value as > kRequiredFieldForPredictionRoutines, or might not). Done. https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... File components/autofill/core/common/autofill_constants.h (right): https://codereview.chromium.org/1411363003/diff/320001/components/autofill/co... components/autofill/core/common/autofill_constants.h:17: // The number of fields required by Autofill to execute it's heuristic and On 2015/11/17 15:45:06, Mathieu Perreault wrote: > nit: *its Done.
lgtm https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1355: ^H https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... File components/autofill/core/common/autofill_constants.h (right): https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... components/autofill/core/common/autofill_constants.h:22: extern const size_t kRequiredFieldsForPredictionRoutines; not sure there's a point in making this extern instead of inlining
Patchset #10 (id:360001) has been deleted
Thanks! https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:1355: On 2015/11/18 22:59:15, Evan Stade wrote: > ^H Done. https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... File components/autofill/core/common/autofill_constants.h (right): https://codereview.chromium.org/1411363003/diff/340001/components/autofill/co... components/autofill/core/common/autofill_constants.h:22: extern const size_t kRequiredFieldsForPredictionRoutines; On 2015/11/18 22:59:15, Evan Stade wrote: > not sure there's a point in making this extern instead of inlining Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1411363003/#ps400001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411363003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411363003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...) linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1411363003/#ps420001 (title: "Fix for android")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411363003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411363003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, no build URL)
Patchset #12 (id:420001) has been deleted
Patchset #12 (id:440001) has been deleted
Patchset #12 (id:460001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
sebsg@chromium.org changed reviewers: + thestig@chromium.org
Hi Lei, could you please take a quick look at chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc please? Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411363003/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411363003/500001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1411363003/#ps500001 (title: "Fixed bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1411363003/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1411363003/500001
Message was sent while issue was closed.
Committed patchset #13 (id:500001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/6b4ba4ea43e17b16e59ef8f8b416226515ac14f1 Cr-Commit-Position: refs/heads/master@{#361460} |
