Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
614023002:
[Password manager] Relplace the FormFieldData vector from autofill::FormData with named fields… (Closed)
|
Created:
6 years, 2 months ago by Pritam Nikam Modified:
6 years, 1 month ago CC:
chromium-reviews, mkwst+watchlist_chromium.org, benquan, browser-components-watch_chromium.org, jam, darin-cc_chromium.org, Dane Wallinga, dyu1, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Password manager] Replace the FormFieldData vector from autofill::FormData with named-fields (Clean-up).
BUG=417295
Committed: https://crrev.com/0acc7f6e27371b58110c5a273de2784c068d1ad8
Cr-Commit-Position: refs/heads/master@{#303603}
Patch Set 1 : #
Total comments: 13
Patch Set 2 : Incorporated review inputs. #
Total comments: 38
Patch Set 3 : Incorporated review comments. #
Total comments: 6
Patch Set 4 : Removed FormData from PasswordFormFillData. #
Total comments: 34
Patch Set 5 : Incorporated review inputs. #
Total comments: 2
Patch Set 6 : Incorporated Vaclav's review comments. #
Total comments: 3
Patch Set 7 : After rebase. #
Total comments: 2
Patch Set 8 : Incorporated nit. #Patch Set 9 : Fixed browser_tests. #
Total comments: 2
Patch Set 10 : Incorporated nit from Vaclav's review. #Messages
Total messages: 42 (9 generated)
Patchset #1 (id:1) has been deleted
pritam.nikam@samsung.com changed reviewers: + vabr@chromium.org
On 2014/09/30 13:16:36, Pritam Nikam wrote: > mailto:pritam.nikam@samsung.com changed reviewers: > + mailto:vabr@chromium.org Hi Vaclav, Please let me know whether my interpretation of the issue/bug is correct. Hope, I've not mistaken here & can go ahead for rest of the auto-fillable input fields. Please have a look. Thanks!
Hi Pritam Nikam, Thanks for looping me in. I think your direction is correct, and the password manager part looks like what I meant. I see no reason why you should not carry on and add the non-password manager autofill changes. Ilya is auto-Cc-ed on this CL, so hopefully he can speak up if the plans uncoiled here don't match his expectation. Cheers, Vaclav https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:101: found_input = false; At this point you throw out the previous value of |found_input|. That's a problem: consider a page with a duplicated input field. In the old code, that would result in exiting the function and returning false. Here it will happily continue and potentially return true. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:103: for (size_t i = 0; i < temp_elements.size(); ++i) { Before you prepare the final version of this CL, please consider de-duplicating the for-loop, e.g., by making a helper function in the anonymous namespace for that. https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... File components/autofill/core/common/form_data.h (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... components/autofill/core/common/form_data.h:43: // TODO (pritam.nikam): Make sure we remove |fields|, and add form fields The action described in TODO should be part of this CL. In other words, please do not land without getting rid of |fields|.
isherman@chromium.org changed reviewers: + isherman@chromium.org
Yes, this is definitely moving in the correct direction. Thanks for working on this cleanup task! As you work on it, please remember to be careful to think through all of the subtleties present in the existing code, and make sure to preserve them in the updated code. If you believe that some are obsolete, please double-check with a code reviewer -- it's, unfortunately, easy to miss a corner case and thus create a small bug that will not be quickly spotted. If you find that you are able to remove a check and still have all the tests pass, please add test coverage for the behavior. This might feel like extra work in the short term, but it is very high leverage: By writing the tests once, you are able to keep all future developers from accidentally introducing a subtle regression. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:85: // text fields. It looks like you removed the code that addresses this comment. Please restore it, both here and below. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:85: // text fields. Also, if there aren't any existing test cases that test this behavior, please add them. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:103: for (size_t i = 0; i < temp_elements.size(); ++i) { On 2014/09/30 14:19:48, vabr (Chromium) wrote: > Before you prepare the final version of this CL, please consider de-duplicating > the for-loop, e.g., by making a helper function in the anonymous namespace for > that. I will emphasize this comment by phrasing it more strongly: You should definitely avoid duplicating this code. The loop is non-trivial, and the logic is bound to be updated periodically in the future. It's therefore important that the logic only be written once, so that it's not possible for someone to fix a bug by updating one copy of the logic, but fail to fix a sister bug by not noticing the second copy.
On 2014/09/30 20:02:32, Ilya Sherman wrote: > Yes, this is definitely moving in the correct direction. Thanks for working on > this cleanup task! > > As you work on it, please remember to be careful to think through all of the > subtleties present in the existing code, and make sure to preserve them in the > updated code. If you believe that some are obsolete, please double-check with a > code reviewer -- it's, unfortunately, easy to miss a corner case and thus create > a small bug that will not be quickly spotted. If you find that you are able to > remove a check and still have all the tests pass, please add test coverage for > the behavior. This might feel like extra work in the short term, but it is > very high leverage: By writing the tests once, you are able to keep all future > developers from accidentally introducing a subtle regression. > > https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:85: // text > fields. > It looks like you removed the code that addresses this comment. Please restore > it, both here and below. > > https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:85: // text > fields. > Also, if there aren't any existing test cases that test this behavior, please > add them. > > https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... > components/autofill/content/renderer/password_autofill_agent.cc:103: for (size_t > i = 0; i < temp_elements.size(); ++i) { > On 2014/09/30 14:19:48, vabr (Chromium) wrote: > > Before you prepare the final version of this CL, please consider > de-duplicating > > the for-loop, e.g., by making a helper function in the anonymous namespace > for > > that. > > I will emphasize this comment by phrasing it more strongly: You should > definitely avoid duplicating this code. The loop is non-trivial, and the logic > is bound to be updated periodically in the future. It's therefore important > that the logic only be written once, so that it's not possible for someone to > fix a bug by updating one copy of the logic, but fail to fix a sister bug by not > noticing the second copy. Thanks Vaclav & Ilya for inputs. I'll continue doing this cleanup and as you've mentioned will add test-cases if required. Thanks!
Thanks Vaclav and Ilya for review inputs. Sorry for delay on posting updates on this CL. I've incorporated inputs from your last review. PTAL! However, there are few quires related to catheterizing rest of the fields. Broadly, I could see 2 possible approaches that we can adopt: 1. Perform heuristic match on renderer side inside WebFormElementToFormData() and fill the fields. 2. Pass FormFieldData array instead of bundling it in FormData, till it gets parse on browser side and categorize based on heuristic match (as per current implementation). If I'm missing something or other way(s) to go about performing this cleanup, please let me know. Thanks! https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:85: // text fields. On 2014/09/30 20:02:32, Ilya Sherman wrote: > It looks like you removed the code that addresses this comment. Please restore > it, both here and below. Done. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:85: // text fields. On 2014/09/30 20:02:32, Ilya Sherman wrote: > Also, if there aren't any existing test cases that test this behavior, please > add them. Test already exists. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:101: found_input = false; On 2014/09/30 14:19:49, vabr (Chromium) wrote: > At this point you throw out the previous value of |found_input|. That's a > problem: consider a page with a duplicated input field. In the old code, that > would result in exiting the function and returning false. Here it will happily > continue and potentially return true. Done. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:103: for (size_t i = 0; i < temp_elements.size(); ++i) { On 2014/09/30 14:19:48, vabr (Chromium) wrote: > Before you prepare the final version of this CL, please consider de-duplicating > the for-loop, e.g., by making a helper function in the anonymous namespace for > that. Done. https://codereview.chromium.org/614023002/diff/20001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:103: for (size_t i = 0; i < temp_elements.size(); ++i) { On 2014/09/30 20:02:32, Ilya Sherman wrote: > On 2014/09/30 14:19:48, vabr (Chromium) wrote: > > Before you prepare the final version of this CL, please consider > de-duplicating > > the for-loop, e.g., by making a helper function in the anonymous namespace > for > > that. > > I will emphasize this comment by phrasing it more strongly: You should > definitely avoid duplicating this code. The loop is non-trivial, and the logic > is bound to be updated periodically in the future. It's therefore important > that the logic only be written once, so that it's not possible for someone to > fix a bug by updating one copy of the logic, but fail to fix a sister bug by not > noticing the second copy. Done. https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... File components/autofill/core/common/form_data.h (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... components/autofill/core/common/form_data.h:43: // TODO (pritam.nikam): Make sure we remove |fields|, and add form fields On 2014/09/30 14:19:49, vabr (Chromium) wrote: > The action described in TODO should be part of this CL. In other words, please > do not land without getting rid of |fields|. Seems, completely removing |fields| will take some time. As I go about interpreting the current logic, fields apart from username & password are |AutofillField| type (derived from FormFieldData). And |fields| (an array of FormFieldData) get distinguished/categorized on heuristic match performed on browser side on receiving AutofillHostMsg_FormsSeen. (Ref: [2]). So there are 2 approaches we can adopt: 1. Perform heuristic match on renderer side inside - WebFormElementToFormData() Ref: [1] and fill the fields. 2. Pass FormFieldData array instead of bundling it in FormData, till it get parsed on browser side and gets categorized based on heuristic (as per current implementation). For both of these approaches I could sense good amount of code changes :) Any pointers to other possible way(s) to go about performing this cleanup will help. [1]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... [2]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
Hi Pritam Nikam. I left a couple of comments. Cowardly, I leave answering your question about the non-password manager autofill fields to Ilya. :) Cheers, Vaclav https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... File components/autofill/content/browser/request_autocomplete_manager.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:38: // Helper function to check whether form having input |field| or not. nit: form -> a form, having -> has Having said that, the comment is confusing (which form does it talk about?). If you address the comment about renaming the function (below), I'd suggest just dropping this comment entirely. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:39: bool HasInputField(const FormFieldData& field) { The name does not match what is computed. What about: HasName? https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:43: // Helper function to set or reset the |is_autofilled| property. |is_autofilled| property... of what? Please specify. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:44: void SetFormFieldsAutocompleted(FormData& form_data, bool is_autofilled) { So is it Autocompleted or autofilled? :) Please be consistent. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:44: void SetFormFieldsAutocompleted(FormData& form_data, bool is_autofilled) { Not non-const references. Use FormData* instead of FormData&. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:54: if (!form_data.fields[i].value.empty()) It looks suspicious, that above the FormFieldData::name is checked against emptiness, and here the FormFieldData::value is. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:65: // Helper funtion to fill the input field. The comment is not useful -- it only says what the function name says. It should describe: * what the arguments are * what the return value means * what non-trivial function it carries out (just filling an input field sounds as easy as a single assignment, so explain the hard job done here) https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:66: static bool FillInputField(blink::WebFormElement* fe, nit: fe -> form_element http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... Also in the signature of FindFormInputElements. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:66: static bool FillInputField(blink::WebFormElement* fe, Drop the "static", this is already in the anonymous namespace. Also in the signature of FindFormInputElements. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:69: bool is_password_field) { optional nit: To make the callsite easier to read, consider using an enum like { PASSWORD_FIELD, TEXT_FIELD } instead of the bool. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:79: found_input = false; While you are changing this code anyway: the old way of found_input being a Boolean is confusing here, since it's value says "no input found", whereas more than one was found. I suggest replacing it with a counter, and check at the end that it's exactly 1. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:94: // Note: This assignment adds a reference to the InputElement. While at it, please replace InputElement with HTMLInputElement. Also below. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:115: bool found_input = FillInputField(fe, data.password.name, result, true); The code will be easier to read if you rename: found_input -> password_found and then exit early below as described there. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:117: found_input = FillInputField(fe, data.username.name, result, false); You can just exit early by return found_input && (data.username.name.empty() || FillInputField(fe, data.username.name, result, false)); https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... File components/autofill/core/common/form_data.h (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/form_data.h:40: FormFieldData username; nit: rename to username_field, password_field https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/form_data.h:44: // for all possible autofillable input fields. Please add a reference to http://crbug.com/417295. https://codereview.chromium.org/614023002/diff/40001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:713: if (!pending.form_data.password.name.empty() && This seems like a change in behaviour -- what about checking that the username element is also not present?
https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... File components/autofill/core/common/form_data.h (right): https://codereview.chromium.org/614023002/diff/20001/components/autofill/core... components/autofill/core/common/form_data.h:43: // TODO (pritam.nikam): Make sure we remove |fields|, and add form fields On 2014/10/13 10:48:00, Pritam Nikam wrote: > On 2014/09/30 14:19:49, vabr (Chromium) wrote: > > The action described in TODO should be part of this CL. In other words, please > > do not land without getting rid of |fields|. > > Seems, completely removing |fields| will take some time. As I go about > interpreting the current logic, fields apart from username & password are > |AutofillField| type (derived from FormFieldData). And |fields| (an array of > FormFieldData) get distinguished/categorized on heuristic match performed on > browser side on receiving AutofillHostMsg_FormsSeen. (Ref: [2]). > > So there are 2 approaches we can adopt: > 1. Perform heuristic match on renderer side inside - WebFormElementToFormData() > Ref: [1] and fill the fields. > 2. Pass FormFieldData array instead of bundling it in FormData, till it get > parsed on browser side and gets categorized based on heuristic (as per current > implementation). > > For both of these approaches I could sense good amount of code changes :) > > Any pointers to other possible way(s) to go about performing this cleanup will > help. > > > [1]. > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... > > [2]. > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... It sounds like we're using this class both for Autofill and for Password Autofill, which is causing some pain. This class is primarily designed to be used with Autofill code. Perhaps the most appropriate change is to use separate classes for Password Autofill vs. Autofill. Indeed, there is already a PasswordFormFillData class. Perhaps it would be appropriate to add the fields to that class, rather than to the shared FormData class. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... File components/autofill/content/browser/request_autocomplete_manager.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:57: } Are the username and password fields ever used with requestAutocomplete? I don't believe that they are. https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/password_form_fill_data.cc:52: result->basic_data.action = form_on_page.action; It looks like this class really shouldn't depend on FormData at all. Instead, it should just have its own origin and action member fields.
Thanks Ilya & Vaclav. I've incorporated some of the review inputs along patch set #3. I'm planning to address rest of the inputs suggested by Ilya mainly to segregate |FormData basic_data| from PasswordFormFillData class in my next patch. I'll be away for a week, and will be able to take care of these only after returning. So, changes here may appear intermediate and are purely WIP. Thanks! https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... File components/autofill/content/browser/request_autocomplete_manager.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:38: // Helper function to check whether form having input |field| or not. On 2014/10/13 12:30:52, vabr (Chromium) wrote: > nit: form -> a form, having -> has > Having said that, the comment is confusing (which form does it talk about?). If > you address the comment about renaming the function (below), I'd suggest just > dropping this comment entirely. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:39: bool HasInputField(const FormFieldData& field) { On 2014/10/13 12:30:52, vabr (Chromium) wrote: > The name does not match what is computed. > What about: HasName? Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:43: // Helper function to set or reset the |is_autofilled| property. On 2014/10/13 12:30:52, vabr (Chromium) wrote: > |is_autofilled| property... of what? > Please specify. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:44: void SetFormFieldsAutocompleted(FormData& form_data, bool is_autofilled) { On 2014/10/13 12:30:52, vabr (Chromium) wrote: > Not non-const references. Use FormData* instead of FormData&. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:44: void SetFormFieldsAutocompleted(FormData& form_data, bool is_autofilled) { On 2014/10/13 12:30:52, vabr (Chromium) wrote: > So is it Autocompleted or autofilled? :) > Please be consistent. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:54: if (!form_data.fields[i].value.empty()) On 2014/10/13 12:30:52, vabr (Chromium) wrote: > It looks suspicious, that above the FormFieldData::name is checked against > emptiness, and here the FormFieldData::value is. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/browser/request_autocomplete_manager.cc:57: } On 2014/10/13 23:49:32, Ilya Sherman wrote: > Are the username and password fields ever used with requestAutocomplete? I > don't believe that they are. Yes, I didn't see this is getting called. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:65: // Helper funtion to fill the input field. On 2014/10/13 12:30:52, vabr (Chromium) wrote: > The comment is not useful -- it only says what the function name says. > It should describe: > * what the arguments are > * what the return value means > * what non-trivial function it carries out (just filling an input field sounds > as easy as a single assignment, so explain the hard job done here) Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:66: static bool FillInputField(blink::WebFormElement* fe, On 2014/10/13 12:30:52, vabr (Chromium) wrote: > nit: fe -> form_element > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#General_Namin... > > Also in the signature of FindFormInputElements. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:66: static bool FillInputField(blink::WebFormElement* fe, On 2014/10/13 12:30:52, vabr (Chromium) wrote: > Drop the "static", this is already in the anonymous namespace. > > Also in the signature of FindFormInputElements. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:69: bool is_password_field) { On 2014/10/13 12:30:53, vabr (Chromium) wrote: > optional nit: To make the callsite easier to read, consider using an enum like { > PASSWORD_FIELD, TEXT_FIELD } instead of the bool. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:79: found_input = false; On 2014/10/13 12:30:53, vabr (Chromium) wrote: > While you are changing this code anyway: > the old way of found_input being a Boolean is confusing here, since it's value > says "no input found", whereas more than one was found. > I suggest replacing it with a counter, and check at the end that it's exactly 1. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:94: // Note: This assignment adds a reference to the InputElement. On 2014/10/13 12:30:53, vabr (Chromium) wrote: > While at it, please replace InputElement with HTMLInputElement. > Also below. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:115: bool found_input = FillInputField(fe, data.password.name, result, true); On 2014/10/13 12:30:52, vabr (Chromium) wrote: > The code will be easier to read if you rename: > found_input -> password_found > and then exit early below as described there. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/cont... components/autofill/content/renderer/password_autofill_agent.cc:117: found_input = FillInputField(fe, data.username.name, result, false); On 2014/10/13 12:30:53, vabr (Chromium) wrote: > You can just exit early by > > return found_input && (data.username.name.empty() || FillInputField(fe, > data.username.name, result, false)); Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... File components/autofill/core/common/form_data.h (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/form_data.h:40: FormFieldData username; On 2014/10/13 12:30:53, vabr (Chromium) wrote: > nit: rename to username_field, password_field Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/form_data.h:44: // for all possible autofillable input fields. On 2014/10/13 12:30:53, vabr (Chromium) wrote: > Please add a reference to http://crbug.com/417295. Done. https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/autofill/core... components/autofill/core/common/password_form_fill_data.cc:52: result->basic_data.action = form_on_page.action; On 2014/10/13 23:49:32, Ilya Sherman wrote: > It looks like this class really shouldn't depend on FormData at all. Instead, > it should just have its own origin and action member fields. Sure, I'll take care this in next patch set. Still WIP. https://codereview.chromium.org/614023002/diff/40001/components/password_mana... File components/password_manager/core/browser/password_form_manager.cc (right): https://codereview.chromium.org/614023002/diff/40001/components/password_mana... components/password_manager/core/browser/password_form_manager.cc:713: if (!pending.form_data.password.name.empty() && On 2014/10/13 12:30:53, vabr (Chromium) wrote: > This seems like a change in behaviour -- what about checking that the username > element is also not present? Done.
Hi Pritam Nikam, Thanks for the update, I'm looking forward to the rest of your changes. Below some comments to the newest patch set. Cheers, Vaclav https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:67: // Utility function to search the unique entry of the |form_element| for the nit: search -> find Currently it sounds like the entry is being searched, i.e., the code is trying to find something in the entry instead of finding the entry. https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:84: ++field_match_counter; Currently the behaviour wrt. duplicates depends on their order. Suppose the form has two elements with the same name, equalt to |field_name|, and that one of those elements is a password field, and the other is a plain text field. Now, if the plain text field comes first, |field_match_counter| is not increased for it (due to the "continue" on line 93), and thus the second field (the password field) is happily accepted. But when the password field comes first, and then the text field is seen, the |field_match_counter| reaches 2 and leads to returning "false". Please decide what is the appropriate result for the situation independent of the order of the two entries, and adjust the code. https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:111: result->input_elements.clear(); optional nit: If you rephrase the last 4 lines to: if (field_match_counter != 1) { result->input_elements.clear(); return false; } return true; then it will be clearer that if the counter is not 1, we are bailing out "early". Currently it does not do much difference (hence this is optional), but if somebody will add some more processing in the case of success, the code above will make it easier to recognise there is no need to handle the counter != 1 case any more.
Hi Vaclav & Ilya. Sorry for replying late. Uploaded a new patch set. With this patch, I've removed FormData from PasswordFormFillData, and addressed the review inputs from past patch set. PTAL. Thanks! https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:67: // Utility function to search the unique entry of the |form_element| for the On 2014/10/16 16:34:24, vabr (Chromium) wrote: > nit: search -> find > Currently it sounds like the entry is being searched, i.e., the code is trying > to find something in the entry instead of finding the entry. Done. https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:84: ++field_match_counter; On 2014/10/16 16:34:24, vabr (Chromium) wrote: > Currently the behaviour wrt. duplicates depends on their order. Suppose the form > has two elements with the same name, equalt to |field_name|, and that one of > those elements is a password field, and the other is a plain text field. > > Now, if the plain text field comes first, |field_match_counter| is not increased > for it (due to the "continue" on line 93), and thus the second field (the > password field) is happily accepted. > > But when the password field comes first, and then the text field is seen, the > |field_match_counter| reaches 2 and leads to returning "false". > > Please decide what is the appropriate result for the situation independent of > the order of the two entries, and adjust the code. Done. In my opinion, even if we would resolve the ordering problem that you highlighted here, because of @line no. 101, field that appears in last shall overwrite the entry |result->input_elements| map. Best way to bail out in such cases is to skip filling and return immediately. Please correct me, if missing something here? https://codereview.chromium.org/614023002/diff/170001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:111: result->input_elements.clear(); On 2014/10/16 16:34:24, vabr (Chromium) wrote: > optional nit: If you rephrase the last 4 lines to: > > if (field_match_counter != 1) { > result->input_elements.clear(); > return false; > } > > return true; > > then it will be clearer that if the counter is not 1, we are bailing out > "early". Currently it does not do much difference (hence this is optional), but > if somebody will add some more processing in the case of success, the code above > will make it easier to recognise there is no need to handle the counter != 1 > case any more. Done.
Hi Pritam Nikam, This LGTM modulo the comment below. Please make sure to get approval from Ilya as well, and from OWNERS of components/autofill/content/common/autofill_messages.h. Cheers, Vaclav https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:69: // and returns |true|. Otherwise clears the references from each This function can return true with an empty |result| -- that's when the sought input field exists, but has wrong type (password for username or plain text for a password). Please unify what the comment says and what the function does. Given the usage below, the function should not return true if the result is empty, AFAIU.
Also, you might want to remove "[WIP]" from the CL title. :)
The changes to password_autofill_agent.cc are somewhat hard to review, as you are both moving the code and changing it. I'd recommend splitting off a separate CL where you *just* move the code, without making any further changes. Then, you can rebase this CL on top of that one, so that the changes you've made are clearly highlighted. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:71: bool FillInputField(blink::WebFormElement* form_element, What does "Fill" refer to in this function name? https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:77: // Fill the username input field. What does this comment mean? https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:79: for (size_t i = 0; i < temp_elements.size(); ++i) { nit: Please use a C++11 range-based for loop. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:82: if (++field_match_counter > 1) Please use the boolean style that was used before. You currently have a bug: if the field_match_counter is 0, the behavior should be the same as if it is 2. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:104: // remain in the |result| set. Note: Clear will remove a reference from each Why did you change the case of "Clear"? The function called below is named "clear()". https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:120: result, PASSWORD_FIELD); nit: I think it would be clearer to inline this code rather than to define a named variable for it. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:314: if (DoUsernamesMatch(fill_data.username_field.value, current_username, nit: Please run "git cl format" over your changes. I believe this is not the format that it recommends. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/autofill_data_validation.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/autofill_data_validation.cc:61: bool IsValidPasswordFormData(const PasswordFormFillData& form) { If you wanted to factor out this function, it would belong in the anonymous namespace, so that it's not linkable externally from this file. However, I don't think it's actually helpful to pull out this as a helper function, in this particular case, since it's very little code, and it's only used from one place. I'd recommend inlining this code. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:25: PasswordFormFillData::PasswordFormFillData() : wait_for_username(false) { Please also initialize the user_submitted field. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:55: result->wait_for_username = wait_for_username_before_autofill; Where do you set the user_submitted value? https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:43: // login. This comment seems obsolete. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:45: base::string16 name; nit: Please leave blank lines between individual fields. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:50: // true if this form was submitted by a user gesture and not javascript. nit: "true" -> "True" https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:54: FormFieldData password_field; Please document these fields.
Thanks Ilya & Vaclav for review! I've incorporated your inputs to latest patch set, PTAL. Thanks! https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:69: // and returns |true|. Otherwise clears the references from each On 2014/11/04 21:28:33, vabr (Chromium) wrote: > This function can return true with an empty |result| -- that's when the sought > input field exists, but has wrong type (password for username or plain text for > a password). Please unify what the comment says and what the function does. > Given the usage below, the function should not return true if the result is > empty, AFAIU. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:71: bool FillInputField(blink::WebFormElement* form_element, On 2014/11/04 23:05:30, Ilya Sherman wrote: > What does "Fill" refer to in this function name? Done. Rephrased to AddInputElementToResultWithMatchingFieldName(). https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:77: // Fill the username input field. On 2014/11/04 23:05:30, Ilya Sherman wrote: > What does this comment mean? Done. Rephrased it to express the below excerpt. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:79: for (size_t i = 0; i < temp_elements.size(); ++i) { On 2014/11/04 23:05:31, Ilya Sherman wrote: > nit: Please use a C++11 range-based for loop. I think we cannot use C++11 range-based for loop here. for loop here, make use of custom implementation of vector i.e. blink::WebVector<blink::WebNode> temp_elements; And unlike std::vector<>, it does not support begin() or end() APIs. Please correct me if I'm missing anything here. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:82: if (++field_match_counter > 1) On 2014/11/04 23:05:30, Ilya Sherman wrote: > Please use the boolean style that was used before. You currently have a bug: if > the field_match_counter is 0, the behavior should be the same as if it is 2. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:104: // remain in the |result| set. Note: Clear will remove a reference from each On 2014/11/04 23:05:30, Ilya Sherman wrote: > Why did you change the case of "Clear"? The function called below is named > "clear()". Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:120: result, PASSWORD_FIELD); On 2014/11/04 23:05:31, Ilya Sherman wrote: > nit: I think it would be clearer to inline this code rather than to define a > named variable for it. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:314: if (DoUsernamesMatch(fill_data.username_field.value, current_username, On 2014/11/04 23:05:30, Ilya Sherman wrote: > nit: Please run "git cl format" over your changes. I believe this is not the > format that it recommends. Done. Formatting here is after "git cl format" only. I've confirmed it again. Shall I manually adjust it instead? https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/autofill_data_validation.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/autofill_data_validation.cc:61: bool IsValidPasswordFormData(const PasswordFormFillData& form) { On 2014/11/04 23:05:31, Ilya Sherman wrote: > If you wanted to factor out this function, it would belong in the anonymous > namespace, so that it's not linkable externally from this file. However, I > don't think it's actually helpful to pull out this as a helper function, in this > particular case, since it's very little code, and it's only used from one place. > I'd recommend inlining this code. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:25: PasswordFormFillData::PasswordFormFillData() : wait_for_username(false) { On 2014/11/04 23:05:31, Ilya Sherman wrote: > Please also initialize the user_submitted field. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.cc:55: result->wait_for_username = wait_for_username_before_autofill; On 2014/11/04 23:05:31, Ilya Sherman wrote: > Where do you set the user_submitted value? Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:43: // login. On 2014/11/04 23:05:31, Ilya Sherman wrote: > This comment seems obsolete. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:45: base::string16 name; On 2014/11/04 23:05:31, Ilya Sherman wrote: > nit: Please leave blank lines between individual fields. Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:50: // true if this form was submitted by a user gesture and not javascript. On 2014/11/04 23:05:31, Ilya Sherman wrote: > nit: "true" -> "True" Done. https://codereview.chromium.org/614023002/diff/190001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:54: FormFieldData password_field; On 2014/11/04 23:05:31, Ilya Sherman wrote: > Please document these fields. Done.
pritam.nikam@samsung.com changed reviewers: + palmer@chromium.org
pritam.nikam@samsung.com changed required reviewers: + isherman@chromium.org, palmer@chromium.org, vabr@chromium.org
+ Palmer palmer@chromium.org: Please review changes in: - components/autofill/content/common/autofill_messages.h Please feel free to go through rest of the changes as well. Thanks!
Hi Pritam Nikam, I have slightly differing views from Ilya, so I think it is more productive if I shut up at this point and let you handle just one reviewer. :) I still added two more comments, though, please have a look at them. Cheers, Vaclav https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:69: // and returns |true|. Otherwise clears the references from each On 2014/11/05 05:58:13, Pritam Nikam wrote: > On 2014/11/04 21:28:33, vabr (Chromium) wrote: > > This function can return true with an empty |result| -- that's when the sought > > input field exists, but has wrong type (password for username or plain text > for > > a password). Please unify what the comment says and what the function does. > > Given the usage below, the function should not return true if the result is > > empty, AFAIU. > > Done. I don't see this comment addressed. The function can still return true with an empty result. https://codereview.chromium.org/614023002/diff/210001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/210001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:84: if (found_input) { I won't fight with Ilya over bool vs. counter here, especially after I managed to miss the bug with the counter being 0. But please at least rename the bool flag to something better expressing it's meaning, e.g., |found_exactly_one_input|. The current name is confusing (the code here reads: if we already had found an input, and we just found another one, then we did not find any).
Thanks Vaclav for review. I've incorporated inputs in new patch set, PTAL! Thanks! https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:69: // and returns |true|. Otherwise clears the references from each On 2014/11/05 08:23:40, vabr (Chromium) wrote: > On 2014/11/05 05:58:13, Pritam Nikam wrote: > > On 2014/11/04 21:28:33, vabr (Chromium) wrote: > > > This function can return true with an empty |result| -- that's when the > sought > > > input field exists, but has wrong type (password for username or plain text > > for > > > a password). Please unify what the comment says and what the function does. > > > Given the usage below, the function should not return true if the result is > > > empty, AFAIU. > > > > Done. > > I don't see this comment addressed. The function can still return true with an > empty result. Done. Sorry! I've missed to incorporate changes. https://codereview.chromium.org/614023002/diff/210001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/210001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:84: if (found_input) { On 2014/11/05 08:23:40, vabr (Chromium) wrote: > I won't fight with Ilya over bool vs. counter here, especially after I managed > to miss the bug with the counter being 0. > But please at least rename the bool flag to something better expressing it's > meaning, e.g., |found_exactly_one_input|. The current name is confusing (the > code here reads: if we already had found an input, and we just found another > one, then we did not find any). Done.
Thank you, Pritam Nikam, for addressing my comments. I left one optional follow-up on your solution. Once you get approval from Ilya, do not wait for me with landing. Cheers, Vaclav https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:112: if (!found_exactly_one_input || (result->input_elements.size() == 0)) { optional nit: This certainly works, although you could just swap the line 89 with the block on lines 91-96 instead. The difference would be that |found_exactly_one_input| would mean precisely what it name claims (now it means: found exactly one input, but maybe that input was not usable), and the check on line 112 would be simple again.
On 2014/11/04 23:05:31, Ilya Sherman wrote: > The changes to password_autofill_agent.cc are somewhat hard to review, as you > are both moving the code and changing it. I'd recommend splitting off a > separate CL where you *just* move the code, without making any further changes. > Then, you can rebase this CL on top of that one, so that the changes you've made > are clearly highlighted. Please do what I've recommended here -- create a separate CL that *just* moves the existing code to a helper method, and does _absolutely_ nothing else. Either Václav or I can quickly approve such a change so that you can land it. That will then make it much easier for me to review this CL. I realize this is a bit of a hassle for you, but the code that you're changing here is very subtle and, unfortunately, poorly tested. Hence, it's important to me as a reviewer to be able to see a very accurate picture of what's changing versus what's just moving.
https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:71: bool AddInputElementToResultWithMatchingFieldName( When moving the code as I've recommended in my previous comment, please name the helper function "FindFormInputElement", to match the name "FindFormInputElements" that's used below. Thanks.
Thanks Vaclav and Ilya for review. As recommended I've added a follow-up CL: https://codereview.chromium.org/700403002/. Please have a look. Thanks! https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:71: bool AddInputElementToResultWithMatchingFieldName( On 2014/11/05 20:40:00, Ilya Sherman wrote: > When moving the code as I've recommended in my previous comment, please name the > helper function "FindFormInputElement", to match the name > "FindFormInputElements" that's used below. Thanks. Done. Added a follow-up CL: https://codereview.chromium.org/700403002/ Please have a look.
On 2014/11/06 06:31:59, Pritam Nikam wrote: > Thanks Vaclav and Ilya for review. > > As recommended I've added a follow-up CL: > https://codereview.chromium.org/700403002/. Please have a look. > > Thanks! > > https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/614023002/diff/230001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:71: bool > AddInputElementToResultWithMatchingFieldName( > On 2014/11/05 20:40:00, Ilya Sherman wrote: > > When moving the code as I've recommended in my previous comment, please name > the > > helper function "FindFormInputElement", to match the name > > "FindFormInputElements" that's used below. Thanks. > > Done. > > Added a follow-up CL: https://codereview.chromium.org/700403002/ > Please have a look. Hi Ilya, Vaclav & Palmer, Please have a look at new patch set. Thanks!
pritam.nikam@samsung.com changed required reviewers: - isherman@chromium.org, palmer@chromium.org, vabr@chromium.org
LGTM, thanks. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:79: for (size_t i = 0; i < temp_elements.size(); ++i) { On 2014/11/05 05:58:13, Pritam Nikam wrote: > On 2014/11/04 23:05:31, Ilya Sherman wrote: > > nit: Please use a C++11 range-based for loop. > > I think we cannot use C++11 range-based for loop here. > for loop here, make use of custom implementation of vector i.e. > blink::WebVector<blink::WebNode> temp_elements; And unlike std::vector<>, it > does not support begin() or end() APIs. > > Please correct me if I'm missing anything here. Yes, you're right -- my bad. https://codereview.chromium.org/614023002/diff/190001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:314: if (DoUsernamesMatch(fill_data.username_field.value, current_username, On 2014/11/05 05:58:14, Pritam Nikam wrote: > On 2014/11/04 23:05:30, Ilya Sherman wrote: > > nit: Please run "git cl format" over your changes. I believe this is not the > > format that it recommends. > > Done. > > Formatting here is after "git cl format" only. I've confirmed it again. > Shall I manually adjust it instead? Thanks for checking that, and sorry for the false alarm.
LGTM mod documentation nit. https://codereview.chromium.org/614023002/diff/250001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/250001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:45: // The URL (minus query parameters) containing the form. Nit: Document this more precisely. An origin is (scheme, host, port) only; it is just exactly a URL with the query string removed.
Thanks Vaclav, Ilya & Palmer, I've incorporated the nit mentioned in new patch. As review seems LGTM from all, I'm going ahead with commit bit. Thanks! https://codereview.chromium.org/614023002/diff/250001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/250001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:45: // The URL (minus query parameters) containing the form. On 2014/11/07 21:19:01, Chromium Palmer wrote: > Nit: Document this more precisely. An origin is (scheme, host, port) only; it is > just exactly a URL with the query string removed. Done.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/614023002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2014/11/08 10:18:10, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) I've fixed the breakage. Request you to have a look. [Overlooked manual merge/copy-paste side effects.] Thanks!
LGTM with a single nit. Thanks for all your effort! Vaclav https://codereview.chromium.org/614023002/diff/290001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/290001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:46: // query string removed. nit: What about path (and fragment identifier)? scheme://host:port/path#fragment_identifier?query
Thanks Vaclav for the correction. I've addressed nit in my newest patch. PTAL! As rest of the changes okayed from all of you, I'll go ahead with commit. Thanks! https://codereview.chromium.org/614023002/diff/290001/components/autofill/cor... File components/autofill/core/common/password_form_fill_data.h (right): https://codereview.chromium.org/614023002/diff/290001/components/autofill/cor... components/autofill/core/common/password_form_fill_data.h:46: // query string removed. On 2014/11/10 15:38:41, vabr (Chromium) wrote: > nit: What about path (and fragment identifier)? > scheme://host:port/path#fragment_identifier?query Done.
The CQ bit was checked by pritam.nikam@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/614023002/310001
Message was sent while issue was closed.
Committed patchset #10 (id:310001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/0acc7f6e27371b58110c5a273de2784c068d1ad8 Cr-Commit-Position: refs/heads/master@{#303603} |
