Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
208453002:
Add "previewing on hover" support for password field. (Closed)
|
Created:
6 years, 9 months ago by ziran.sun Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd "previewing on hover" support for password field.
In terms of previewing on hover, treat password the same as Autofill.
Functions are introduced to handle when a username selection is hovered over,
password and the selected username are previewed, and cleared once preview is done.
This preview also supports "character matching" preview, e.g. type first few letters
of username in, but without affecting the password "inline autocompletion" feature.
R=isherman@chromium.org, tsepez@chromium.org
BUG=63421
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271993
Patch Set 1 #
Total comments: 26
Patch Set 2 : Update code as per Ilya's comment. #
Total comments: 3
Patch Set 3 : Rebase code after password Refactoring[BUG178358]. #Patch Set 4 : Improve rebased code. #
Total comments: 44
Patch Set 5 : Update code as per Ilya's review comments after rebasing code. #
Total comments: 20
Patch Set 6 : Update code as per Ilya's further comments after rebasing. #
Total comments: 8
Patch Set 7 : Update code as per Ilya's comments. #Patch Set 8 : Update code as per review comments on SelectionRange. #
Total comments: 4
Patch Set 9 : Update code as per further review comments. #Messages
Total messages: 24 (0 generated)
Add "previewing on hover" for password. Please review. Thanks!
The messages in and of themselves are OK, but there was some discussion on the bug as to whether we wanted this?
On 2014/03/21 18:01:51, Tom Sepez wrote: Tom, we are aware of the disucssions. I think the decision is to go ahead. There are actually two parts of patches for this bug. First patch is for single input field (https://codereview.chromium.org/148413002/) and you had helped reviewing the first patch :). This is the second part patch for the bug.
Messages LGTM. Thanks!
https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:2873: EXPECT_FALSE(password.isAutofilled()); Hmm, why is the password field no longer autofilled, given that the last parameter to ClearPreviewedFormWithElement() is |true|, indicating that the password field was previously autofilled? Please fix this [or let me know why I'm wrong ;)], and also add additional test coverage. Specifically, please test all four combinations of the username and password fields being previously autofilled or not. Also, please test cases where the username element has a non-empty suggestion vs. where it doesn't. In addition to that, please add test coverage for the PasswordAutofillAgent method responsible for previewing the suggestions. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/browser/content_autofill_driver.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/browser/content_autofill_driver.h:58: const base::string16& username) OVERRIDE; Let's rename this to "RendererShouldFillPassword", to match the related method names. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/common/autofill_messages.h:143: base::string16 /* username value */) nit: "username value" -> "username" here and below. This file is not currently consistent about this, but the intent is for the commented-out text to be a variable name. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/autofill_agent.h:218: bool is_password_previewed_; Please move this state tracking and everything that depends on it into the PasswordAutofillAgent class. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:644: return element && element->formControlType() == kPassword; There is already an "element.isPasswordField()" method. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1063: ExtractAutofillableElements(form_element, requirements, &control_elements); Hmm, would anything go wrong if we were to just always extract elements with REQUIRE_NONE? The if-stmt on line 1080 makes me think that the requirement mask isn't actually needed when extracting elements. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:779: std::vector<base::string16> username_and_password; Please use out-parameters named "string16* username" and "string16* password" rather than returning a vector of two objects. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:915: username.length()); This will always set an empty selection, right? The goal is to have the part of the text that the user didn't type be selected. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:57: // Return true if password was autofilled before preview the form. nit: "before preview" -> "before previewing" https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/autofill_external_delegate.cc:175: query_field_, value); Can we have a DCHECK(success) here as there is in DidAcceptSuggestion()? https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/password_autofill_manager.cc:31: autofill_driver_->RendererShouldPreviewPassword(username); Can we send both the username and the password down to the renderer? The eventual goal is to have the renderer not need to know *all* of the possible passwords, and instead to only have that state available in the browser process. (In fact, if you'd like to work on more broadly implementing that task [as a separate CL, of course], it would be quite helpful :) It might be useful to first upload a CL implementing this for DidAcceptAutofillSuggestion(), and then to implement something similar in this CL. Or you might prefer to start from this CL, and then improve the implementation of DidAcceptAutofillSuggestion() in a later CL. https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/password_autofill_manager.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/password_autofill_manager.h:31: bool DidSelectAutofillSuggestion(const FormFieldData& field, nit: Let's name this method "DidSelectSuggestion()", and rename the method below to "DidAcceptSuggestion()".
Updated code as per review comments. Thanks! https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:2873: EXPECT_FALSE(password.isAutofilled()); On 2014/03/21 22:35:19, Ilya Sherman wrote: > Hmm, why is the password field no longer autofilled, given that the last > parameter to ClearPreviewedFormWithElement() is |true|, indicating that the > password field was previously autofilled? > > Please fix this [or let me know why I'm wrong ;)], and also add additional test > coverage. Specifically, please test all four combinations of the username and > password fields being previously autofilled or not. Also, please test cases > where the username element has a non-empty suggestion vs. where it doesn't. > > In addition to that, please add test coverage for the PasswordAutofillAgent > method responsible for previewing the suggestions. You are right. There is an error in the test code. I've corrected it. Added all four combinations of the username and password fields being previously autofilled or not. Added one test on empty suggestedValue for username. I didn't add non-empty suggestedValue test since I feel that it's covered by the other 4 tests . For PasswordAutofillAgent related test, do you mean add a browser test in autofill_password_agent_browsertest.cc? Otherwise, there is DidSelectAutofillSuggestion unittest covered in PasswordAutofillManagerTest already. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/browser/content_autofill_driver.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/browser/content_autofill_driver.h:58: const base::string16& username) OVERRIDE; On 2014/03/21 22:35:19, Ilya Sherman wrote: > Let's rename this to "RendererShouldFillPassword", to match the related method > names. Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/common/autofill_messages.h:143: base::string16 /* username value */) On 2014/03/21 22:35:19, Ilya Sherman wrote: > nit: "username value" -> "username" here and below. This file is not currently > consistent about this, but the intent is for the commented-out text to be a > variable name. Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/autofill_agent.h:218: bool is_password_previewed_; On 2014/03/21 22:35:19, Ilya Sherman wrote: > Please move this state tracking and everything that depends on it into the > PasswordAutofillAgent class. Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:644: return element && element->formControlType() == kPassword; On 2014/03/21 22:35:19, Ilya Sherman wrote: > There is already an "element.isPasswordField()" method. Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1063: ExtractAutofillableElements(form_element, requirements, &control_elements); On 2014/03/21 22:35:19, Ilya Sherman wrote: > Hmm, would anything go wrong if we were to just always extract elements with > REQUIRE_NONE? The if-stmt on line 1080 makes me think that the requirement mask > isn't actually needed when extracting elements. I feel that it might be safe to use REQUIRE_NONE here. browser_tests hasn't give any complain. Any other tests that I should verify? I will need to clean the code in autofill_agent.cc if calling REQUIRE_NONE is good enough here. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:779: std::vector<base::string16> username_and_password; On 2014/03/21 22:35:19, Ilya Sherman wrote: > Please use out-parameters named "string16* username" and "string16* password" > rather than returning a vector of two objects. Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.cc:915: username.length()); On 2014/03/21 22:35:19, Ilya Sherman wrote: > This will always set an empty selection, right? The goal is to have the part of > the text that the user didn't type be selected. I tried to replace current_username.length() with username_element->value().length() but it doesn't seems making any difference. I will look into this further. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/password_autofill_agent.h:57: // Return true if password was autofilled before preview the form. On 2014/03/21 22:35:19, Ilya Sherman wrote: > nit: "before preview" -> "before previewing" Done. https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/password_autofill_manager.cc:31: autofill_driver_->RendererShouldPreviewPassword(username); On 2014/03/21 22:35:19, Ilya Sherman wrote: > Can we send both the username and the password down to the renderer? The > eventual goal is to have the renderer not need to know *all* of the possible > passwords, and instead to only have that state available in the browser process. > (In fact, if you'd like to work on more broadly implementing that task [as a > separate CL, of course], it would be quite helpful :) > > It might be useful to first upload a CL implementing this for > DidAcceptAutofillSuggestion(), and then to implement something similar in this > CL. Or you might prefer to start from this CL, and then improve the > implementation of DidAcceptAutofillSuggestion() in a later CL. I might have a quick work on DidAcceptAutofillSuggestion() first and see if I got what you meant. Then improve this one. I guess I need to file an issue on this first? https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/password_autofill_manager.h (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/password_autofill_manager.h:31: bool DidSelectAutofillSuggestion(const FormFieldData& field, On 2014/03/21 22:35:19, Ilya Sherman wrote: > nit: Let's name this method "DidSelectSuggestion()", and rename the method below > to "DidAcceptSuggestion()". Done.
Thanks, Ziran! I haven't taken another close look at the PasswordAutofillAgent changes, because I expect that code to change pretty substantially once we pass the password as part of the PreviewPassword IPC message. https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/208453002/diff/1/chrome/renderer/autofill/for... chrome/renderer/autofill/form_autofill_browsertest.cc:2873: EXPECT_FALSE(password.isAutofilled()); On 2014/03/25 18:25:26, ziran.sun wrote: > On 2014/03/21 22:35:19, Ilya Sherman wrote: > > Hmm, why is the password field no longer autofilled, given that the last > > parameter to ClearPreviewedFormWithElement() is |true|, indicating that the > > password field was previously autofilled? > > > > Please fix this [or let me know why I'm wrong ;)], and also add additional > test > > coverage. Specifically, please test all four combinations of the username and > > password fields being previously autofilled or not. Also, please test cases > > where the username element has a non-empty suggestion vs. where it doesn't. > > > > In addition to that, please add test coverage for the PasswordAutofillAgent > > method responsible for previewing the suggestions. > You are right. There is an error in the test code. I've corrected it. > > Added all four combinations of the username and > password fields being previously autofilled or not. > > Added one test on empty suggestedValue for username. I didn't add non-empty > suggestedValue test since I feel that it's covered by the other 4 tests . > > For PasswordAutofillAgent related test, do you mean add a browser test in > autofill_password_agent_browsertest.cc? Otherwise, there is > DidSelectAutofillSuggestion unittest covered in PasswordAutofillManagerTest > already. Yes, I do mean that we should test the renderer code as well as the browser code. Though, you might want to hold off on working on that until you implement the change to send both the username and password across IPC -- it ought to simplify the renderer code, which will hopefully make it both clearer to see what needs to be tested, and easier to test. https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1063: ExtractAutofillableElements(form_element, requirements, &control_elements); On 2014/03/25 18:25:26, ziran.sun wrote: > On 2014/03/21 22:35:19, Ilya Sherman wrote: > > Hmm, would anything go wrong if we were to just always extract elements with > > REQUIRE_NONE? The if-stmt on line 1080 makes me think that the requirement > mask > > isn't actually needed when extracting elements. > > I feel that it might be safe to use REQUIRE_NONE here. browser_tests hasn't give > any complain. Any other tests that I should verify? I will need to clean the > code in autofill_agent.cc if calling REQUIRE_NONE is good enough here. I think since neither of us sees any reason not to use REQUIRE_NONE here, let's go for it. You could launch a try bot run to double-check that no tests complain, if you'd like to be extra sure. https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... File components/autofill/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/1/components/autofill/core/bro... components/autofill/core/browser/password_autofill_manager.cc:31: autofill_driver_->RendererShouldPreviewPassword(username); On 2014/03/25 18:25:26, ziran.sun wrote: > On 2014/03/21 22:35:19, Ilya Sherman wrote: > > Can we send both the username and the password down to the renderer? The > > eventual goal is to have the renderer not need to know *all* of the possible > > passwords, and instead to only have that state available in the browser > process. > > (In fact, if you'd like to work on more broadly implementing that task [as a > > separate CL, of course], it would be quite helpful :) > > > > It might be useful to first upload a CL implementing this for > > DidAcceptAutofillSuggestion(), and then to implement something similar in this > > CL. Or you might prefer to start from this CL, and then improve the > > implementation of DidAcceptAutofillSuggestion() in a later CL. > > I might have a quick work on DidAcceptAutofillSuggestion() first and see if I > got what you meant. Then improve this one. I guess I need to file an issue on > this first? Sounds great -- thanks! I've filed http://crbug.com/356310 to track this. https://codereview.chromium.org/208453002/diff/20001/components/autofill/cont... File components/autofill/content/browser/content_autofill_driver.cc (right): https://codereview.chromium.org/208453002/diff/20001/components/autofill/cont... components/autofill/content/browser/content_autofill_driver.cc:141: host->Send(new AutofillMsg_AcceptPasswordAutofillSuggestion( nit: Let's rename this message as well, to AutofillMsg_FillPassword. https://codereview.chromium.org/208453002/diff/20001/components/autofill/cont... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/20001/components/autofill/cont... components/autofill/content/renderer/autofill_agent.cc:449: password_autofill_agent_->WasPasswordAutofilled()); Please move all of the password-related logic for clearing the previewed form to be part of password_autofill_agent_->DidClearAutofillSelection(). There should be minimal changes to the AutofillAgent class for this change. https://codereview.chromium.org/208453002/diff/20001/components/autofill/core... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/208453002/diff/20001/components/autofill/core... components/autofill/core/browser/autofill_external_delegate.cc:191: query_field_, value); nit: Looks like this now fits on the previous line.
The CQ bit was checked by ziran.sun@samsung.com
The CQ bit was unchecked by ziran.sun@samsung.com
Rebase code after password Refactoring[BUG178358]. Please review. Thanks!
https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:494: password_autofill_agent_->WasPasswordAutofilled()); This call site should not need to change as a result of the password changes. Those changes should be internal to the PasswordAutofillAgent class. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:534: was_query_node_autofilled_ = element_.isAutofilled(); This should be tracked internally to the PasswordAutofillAgent class. So, you'll need two new instance variables within that class: was_username_autofilled_ and was_password_autofilled_. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.h:153: bool was_password_autofilled); There might be multiple passwords. I think it might be easiest not to call this method at all for password forms, since we know there are only two elements to deal with in that case. So, within the PasswordAutofillAgent class's private implementation, you could have a simple ClearPreview() function. That function would pick out the username and password elements, and: (1) Clear their suggested values. (2) Restore their previous autofilled or not-autofilled states. (3) Position the cursor at the end of the username field. (3) is admittedly a bit of nontrivial repeated code; but on the whole, I think the result is cleaner with all of the logic internalized within the PasswordAutofillAgent class. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:387: username_element.setSelectionRange(username.length(), username.length()); If you rebase, you'll see that the line you removed was needed, and that this line was updated. Please make the same change below. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. Hmm, does inline autocompletion actually call this (new) method? If so, what's the full codepath that triggers the call? https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:444: blink::WebInputElement input; nit: I'd rename this to |username|. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:449: return true; This method should actually clear the form, not just return true, if there's work to be done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:49: bool SelectSuggestion(const blink::WebNode& node, nit: Let's name this "PreviewSuggestion". https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:62: bool WasPasswordAutofilled(); This shouldn't need to be a public method. It should be an internal implementation detail for this class. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:214: bool is_password_previewed_; Why is this field necessary? https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:123: NOTREACHED(); nit: Please write these two lines as bool success = SelectSuggestion(form_field_, value); DCHECK(success); https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.h (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.h:78: const base::string16& username); nit: Let's name these methods "FillSuggestion" and "PreviewSuggestion". https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_generation_manager_unittest.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_generation_manager_unittest.cc:67: nit: No need for a blank line here, because these methods are all closely related as implementing the PasswordManagerDriver interface. https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_generation_manager_unittest.cc:72: nit: Same applies here as well. https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_manager_driver.h (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:49: // and fill the password with |password|. nit: "Tells the driver to fill the form with the |username| and |password|." https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:55: // and preview the password with |password|. nit: "Tells the driver to preview filling form with the |username| and |password|." https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:58: const base::string16& password) = 0; nit: I'd recommend renaming these two methods to FillSuggestion() and PreviewSuggestion(). https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:61: virtual void ClearPasswordPreviewedForm() = 0; nit: "ClearPreviewedForm"
update code as per review comments. All review comments have been addressed. Please review. Thanks! https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:494: password_autofill_agent_->WasPasswordAutofilled()); On 2014/05/13 00:44:05, Ilya Sherman wrote: > This call site should not need to change as a result of the password changes. > Those changes should be internal to the PasswordAutofillAgent class. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:534: was_query_node_autofilled_ = element_.isAutofilled(); On 2014/05/13 00:44:05, Ilya Sherman wrote: > This should be tracked internally to the PasswordAutofillAgent class. So, > you'll need two new instance variables within that class: > was_username_autofilled_ and was_password_autofilled_. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.h:153: bool was_password_autofilled); On 2014/05/13 00:44:05, Ilya Sherman wrote: > There might be multiple passwords. I think it might be easiest not to call this > method at all for password forms, since we know there are only two elements to > deal with in that case. So, within the PasswordAutofillAgent class's private > implementation, you could have a simple ClearPreview() function. That function > would pick out the username and password elements, and: > (1) Clear their suggested values. > (2) Restore their previous autofilled or not-autofilled states. > (3) Position the cursor at the end of the username field. > > (3) is admittedly a bit of nontrivial repeated code; but on the whole, I think > the result is cleaner with all of the logic internalized within the > PasswordAutofillAgent class. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:387: username_element.setSelectionRange(username.length(), username.length()); On 2014/05/13 00:44:05, Ilya Sherman wrote: > If you rebase, you'll see that the line you removed was needed, and that this > line was updated. Please make the same change below. I'm not able to see where this line is needed, even after rebasing code. Do please let me know what I have missed. I've put this line back though. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/13 00:44:05, Ilya Sherman wrote: > Hmm, does inline autocompletion actually call this (new) method? If so, what's > the full codepath that triggers the call? I don't think inline autocompletion calls this. The use case is related to inline autocompletion though: Say user has two usernames: ziran_su & zi1234517. When type "zi" into username field, the inline autocompletion auto-completes the field with "ziran_su". When select "zi1234517" from the suggestion box, we will have "17" highlighted as the selection range regardless that "zi12345" doesn't match "ziran_su". I feel that in this case the selection range should be "1234517", starting from the end of matching character between value and suggestedValue. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:444: blink::WebInputElement input; On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: I'd rename this to |username|. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:449: return true; On 2014/05/13 00:44:05, Ilya Sherman wrote: > This method should actually clear the form, not just return true, if there's > work to be done. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:49: bool SelectSuggestion(const blink::WebNode& node, On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: Let's name this "PreviewSuggestion". Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:62: bool WasPasswordAutofilled(); On 2014/05/13 00:44:05, Ilya Sherman wrote: > This shouldn't need to be a public method. It should be an internal > implementation detail for this class. Done. https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:214: bool is_password_previewed_; On 2014/05/13 00:44:05, Ilya Sherman wrote: > Why is this field necessary? Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:123: NOTREACHED(); On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: Please write these two lines as > > bool success = SelectSuggestion(form_field_, value); > DCHECK(success); What about NOTREACHED() uses in other places of this file? Does it make sense to replace them with DCHECK as well? https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.h (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.h:78: const base::string16& username); On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: Let's name these methods "FillSuggestion" and "PreviewSuggestion". Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_generation_manager_unittest.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_generation_manager_unittest.cc:67: On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: No need for a blank line here, because these methods are all closely > related as implementing the PasswordManagerDriver interface. Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_generation_manager_unittest.cc:72: On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: Same applies here as well. Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_manager_driver.h (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:49: // and fill the password with |password|. On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: "Tells the driver to fill the form with the |username| and |password|." Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:55: // and preview the password with |password|. On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: "Tells the driver to preview filling form with the |username| and > |password|." Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:58: const base::string16& password) = 0; On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: I'd recommend renaming these two methods to FillSuggestion() and > PreviewSuggestion(). Done. https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:61: virtual void ClearPasswordPreviewedForm() = 0; On 2014/05/13 00:44:05, Ilya Sherman wrote: > nit: "ClearPreviewedForm" Done.
Thanks, Ziran! This is getting quite close to being good to go :) https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:387: username_element.setSelectionRange(username.length(), username.length()); On 2014/05/14 15:35:12, ziran.sun wrote: > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > If you rebase, you'll see that the line you removed was needed, and that this > > line was updated. Please make the same change below. > > I'm not able to see where this line is needed, even after rebasing code. Do > please let me know what I have missed. I've put this line back though. Oops, sorry, you're right -- I was thinking of a different case. My bad! https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/14 15:35:12, ziran.sun wrote: > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > Hmm, does inline autocompletion actually call this (new) method? If so, > what's > > the full codepath that triggers the call? > > I don't think inline autocompletion calls this. The use case is related to > inline autocompletion though: > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into username > field, the inline autocompletion auto-completes the field with "ziran_su". When > select "zi1234517" from the suggestion box, we will have "17" highlighted as the > selection range regardless that "zi12345" doesn't match "ziran_su". I feel that > in this case the selection range should be "1234517", starting from the end of > matching character between value and suggestedValue. I agree with you there. However, there's a simpler way to implement that, because we only ever show suggestions that match the full value that's been typed: username_element.setSelectionRange(username_element.value().length(), username_element.suggestedValue().length()); https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:123: NOTREACHED(); On 2014/05/14 15:35:12, ziran.sun wrote: > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > nit: Please write these two lines as > > > > bool success = SelectSuggestion(form_field_, value); > > DCHECK(success); > > What about NOTREACHED() uses in other places of this file? Does it make sense to > replace them with DCHECK as well? Yes, let's do that :) https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1093: // username_element value and suggestedValue. value lenght is 0 in this case. nit: "lenght" -> "length" https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1157: &password_element_)); Can you test via the public API of DidClearAutofillSelection() rather than by calling the internal method ClearPreview() (exposed as ClearPreviewForTest())? https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:108: const base::string16& password); nit: Let's name these two methods "OnFillPasswordSuggestion" and "OnPreviewPasswordSuggestion". https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1097: return true; Hmm, it looks like this method can only ever return true. If so, it might as well have a void return type instead. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:45: const blink::WebString& password); nit: Please leave a blank line after this one. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:53: blink::WebInputElement* password); nit: Please move this to the very bottom of the "public" section, since it's not really part of the public API that clients should be using. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:57: // true when |node| is fillable by password Autofill. Please update this documentation. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:186: // Clears previewed username and password. nit: "Clears the preview for the username and password fields, restoring both to their previous filled state." https://codereview.chromium.org/208453002/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_driver.h (right): https://codereview.chromium.org/208453002/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:53: // password|. nit: missing an '|' before "password|". https://codereview.chromium.org/208453002/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:57: // Clear previewed password and username fields. nit: "Tells the driver to clear ..."
Updated code as per review comments. All comments have been addressed. Please review. Thanks! https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/14 23:10:58, Ilya Sherman wrote: > On 2014/05/14 15:35:12, ziran.sun wrote: > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > Hmm, does inline autocompletion actually call this (new) method? If so, > > what's > > > the full codepath that triggers the call? > > > > I don't think inline autocompletion calls this. The use case is related to > > inline autocompletion though: > > > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into > username > > field, the inline autocompletion auto-completes the field with "ziran_su". > When > > select "zi1234517" from the suggestion box, we will have "17" highlighted as > the > > selection range regardless that "zi12345" doesn't match "ziran_su". I feel > that > > in this case the selection range should be "1234517", starting from the end of > > matching character between value and suggestedValue. > > I agree with you there. However, there's a simpler way to implement that, > because we only ever show suggestions that match the full value that's been > typed: > > username_element.setSelectionRange(username_element.value().length(), > username_element.suggestedValue().length()); I'm not sure this works for the use case I described above. When the inline autocompletion is in place, the value of the field is not always the same as we typed in, e.g. typing in "zi", the inline autocompletion auto-completes the field with "ziran_su". However, the suggestion box still shows all the suggestions that match "zi" rather than "ziran_su", e.g. "zi1234517" is listed in the suggestion box as described above. If we take the length of the value of the username field as the selection range starting point, we only have "17" highlighted while hovering over "zi1234517", which might not be what we want. Do let me know if I misses any points here though. https://codereview.chromium.org/208453002/diff/260001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:123: NOTREACHED(); On 2014/05/14 23:10:58, Ilya Sherman wrote: > On 2014/05/14 15:35:12, ziran.sun wrote: > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > nit: Please write these two lines as > > > > > > bool success = SelectSuggestion(form_field_, value); > > > DCHECK(success); > > > > What about NOTREACHED() uses in other places of this file? Does it make sense > to > > replace them with DCHECK as well? > > Yes, let's do that :) Done. https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1093: // username_element value and suggestedValue. value lenght is 0 in this case. On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: "lenght" -> "length" Done. https://codereview.chromium.org/208453002/diff/280001/chrome/renderer/autofil... chrome/renderer/autofill/password_autofill_agent_browsertest.cc:1157: &password_element_)); On 2014/05/14 23:10:58, Ilya Sherman wrote: > Can you test via the public API of DidClearAutofillSelection() rather than by > calling the internal method ClearPreview() (exposed as ClearPreviewForTest())? Done. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:108: const base::string16& password); On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: Let's name these two methods "OnFillPasswordSuggestion" and > "OnPreviewPasswordSuggestion". Done. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1097: return true; On 2014/05/14 23:10:58, Ilya Sherman wrote: > Hmm, it looks like this method can only ever return true. If so, it might as > well have a void return type instead. Done. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:45: const blink::WebString& password); On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:53: blink::WebInputElement* password); On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: Please move this to the very bottom of the "public" section, since it's not > really part of the public API that clients should be using. This function has been removed. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:57: // true when |node| is fillable by password Autofill. On 2014/05/14 23:10:58, Ilya Sherman wrote: > Please update this documentation. Done. https://codereview.chromium.org/208453002/diff/280001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:186: // Clears previewed username and password. On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: "Clears the preview for the username and password fields, restoring both to > their previous filled state." Done. https://codereview.chromium.org/208453002/diff/280001/components/password_man... File components/password_manager/core/browser/password_manager_driver.h (right): https://codereview.chromium.org/208453002/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:53: // password|. On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: missing an '|' before "password|". Done. https://codereview.chromium.org/208453002/diff/280001/components/password_man... components/password_manager/core/browser/password_manager_driver.h:57: // Clear previewed password and username fields. On 2014/05/14 23:10:58, Ilya Sherman wrote: > nit: "Tells the driver to clear ..." Done.
https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/15 12:36:37, ziran.sun wrote: > On 2014/05/14 23:10:58, Ilya Sherman wrote: > > On 2014/05/14 15:35:12, ziran.sun wrote: > > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > > Hmm, does inline autocompletion actually call this (new) method? If so, > > > what's > > > > the full codepath that triggers the call? > > > > > > I don't think inline autocompletion calls this. The use case is related to > > > inline autocompletion though: > > > > > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into > > username > > > field, the inline autocompletion auto-completes the field with "ziran_su". > > When > > > select "zi1234517" from the suggestion box, we will have "17" highlighted as > > the > > > selection range regardless that "zi12345" doesn't match "ziran_su". I feel > > that > > > in this case the selection range should be "1234517", starting from the end > of > > > matching character between value and suggestedValue. > > > > I agree with you there. However, there's a simpler way to implement that, > > because we only ever show suggestions that match the full value that's been > > typed: > > > > username_element.setSelectionRange(username_element.value().length(), > > username_element.suggestedValue().length()); > > I'm not sure this works for the use case I described above. When the inline > autocompletion is in place, the value of the field is not always the same as we > typed in, e.g. typing in "zi", the inline autocompletion auto-completes the > field with "ziran_su". However, the suggestion box still shows all the > suggestions that match "zi" rather than "ziran_su", e.g. "zi1234517" is listed > in the suggestion box as described above. If we take the length of the value of > the username field as the selection range starting point, we only have "17" > highlighted while hovering over "zi1234517", which might not be what we want. > > Do let me know if I misses any points here though. Hmm, that's a fair point. How about this, then? username_element.setSelectionRange(username_element.selectionStart(), username_element.suggestedValue().length()); https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/common/autofill_messages.h:145: // Tells the renderer to fill the username and password with with given nit: You have an extra space between "the" and "username". https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:447: nit: I'd omit this blank line. https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:54: // their previous filled state. Return false when no login information were nit: "when no login information were found" -> "if no login information was found" https://codereview.chromium.org/208453002/diff/300001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/300001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:135: DCHECK(false); nit: This one is clearer as a NOTREACHED(). Just the "if (!SomeFunctionCallSucceeds()) NOTREACHED();" pattern is harder to follow than "do something, then DCHECK that it succeeded".
Updated code as per review comments. All comments have been addressed. Please review. Thanks! https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/15 21:36:08, Ilya Sherman wrote: > On 2014/05/15 12:36:37, ziran.sun wrote: > > On 2014/05/14 23:10:58, Ilya Sherman wrote: > > > On 2014/05/14 15:35:12, ziran.sun wrote: > > > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > > > Hmm, does inline autocompletion actually call this (new) method? If so, > > > > what's > > > > > the full codepath that triggers the call? > > > > > > > > I don't think inline autocompletion calls this. The use case is related to > > > > inline autocompletion though: > > > > > > > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into > > > username > > > > field, the inline autocompletion auto-completes the field with "ziran_su". > > > When > > > > select "zi1234517" from the suggestion box, we will have "17" highlighted > as > > > the > > > > selection range regardless that "zi12345" doesn't match "ziran_su". I feel > > > that > > > > in this case the selection range should be "1234517", starting from the > end > > of > > > > matching character between value and suggestedValue. > > > > > > I agree with you there. However, there's a simpler way to implement that, > > > because we only ever show suggestions that match the full value that's been > > > typed: > > > > > > username_element.setSelectionRange(username_element.value().length(), > > > username_element.suggestedValue().length()); > > > > I'm not sure this works for the use case I described above. When the inline > > autocompletion is in place, the value of the field is not always the same as > we > > typed in, e.g. typing in "zi", the inline autocompletion auto-completes the > > field with "ziran_su". However, the suggestion box still shows all the > > suggestions that match "zi" rather than "ziran_su", e.g. "zi1234517" is listed > > in the suggestion box as described above. If we take the length of the value > of > > the username field as the selection range starting point, we only have "17" > > highlighted while hovering over "zi1234517", which might not be what we want. > > > > Do let me know if I misses any points here though. > > Hmm, that's a fair point. How about this, then? > > username_element.setSelectionRange(username_element.selectionStart(), > username_element.suggestedValue().length()); This will have the beginning of the suggestedValue as selectionRange starting point, which means the whole suggestedValue is selected. It sounds another reasonable solution for me. I'm not sure what's the requirement spec for this case - I had a search on w3c drafts but couldn't find anything about this. https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/common/autofill_messages.h:145: // Tells the renderer to fill the username and password with with given On 2014/05/15 21:36:09, Ilya Sherman wrote: > nit: You have an extra space between "the" and "username". Done. https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:447: On 2014/05/15 21:36:09, Ilya Sherman wrote: > nit: I'd omit this blank line. Done. https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/300001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:54: // their previous filled state. Return false when no login information were On 2014/05/15 21:36:09, Ilya Sherman wrote: > nit: "when no login information were found" -> "if no login information was > found" Done. https://codereview.chromium.org/208453002/diff/300001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/208453002/diff/300001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:135: DCHECK(false); On 2014/05/15 21:36:09, Ilya Sherman wrote: > nit: This one is clearer as a NOTREACHED(). Just the "if > (!SomeFunctionCallSucceeds()) NOTREACHED();" pattern is harder to follow than > "do something, then DCHECK that it succeeded". Done.
https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:414: // characters. On 2014/05/16 09:18:13, ziran.sun wrote: > On 2014/05/15 21:36:08, Ilya Sherman wrote: > > On 2014/05/15 12:36:37, ziran.sun wrote: > > > On 2014/05/14 23:10:58, Ilya Sherman wrote: > > > > On 2014/05/14 15:35:12, ziran.sun wrote: > > > > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > > > > Hmm, does inline autocompletion actually call this (new) method? If > so, > > > > > what's > > > > > > the full codepath that triggers the call? > > > > > > > > > > I don't think inline autocompletion calls this. The use case is related > to > > > > > inline autocompletion though: > > > > > > > > > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into > > > > username > > > > > field, the inline autocompletion auto-completes the field with > "ziran_su". > > > > When > > > > > select "zi1234517" from the suggestion box, we will have "17" > highlighted > > as > > > > the > > > > > selection range regardless that "zi12345" doesn't match "ziran_su". I > feel > > > > that > > > > > in this case the selection range should be "1234517", starting from the > > end > > > of > > > > > matching character between value and suggestedValue. > > > > > > > > I agree with you there. However, there's a simpler way to implement that, > > > > because we only ever show suggestions that match the full value that's > been > > > > typed: > > > > > > > > username_element.setSelectionRange(username_element.value().length(), > > > > username_element.suggestedValue().length()); > > > > > > I'm not sure this works for the use case I described above. When the inline > > > autocompletion is in place, the value of the field is not always the same as > > we > > > typed in, e.g. typing in "zi", the inline autocompletion auto-completes the > > > field with "ziran_su". However, the suggestion box still shows all the > > > suggestions that match "zi" rather than "ziran_su", e.g. "zi1234517" is > listed > > > in the suggestion box as described above. If we take the length of the value > > of > > > the username field as the selection range starting point, we only have "17" > > > highlighted while hovering over "zi1234517", which might not be what we > want. > > > > > > Do let me know if I misses any points here though. > > > > Hmm, that's a fair point. How about this, then? > > > > username_element.setSelectionRange(username_element.selectionStart(), > > > username_element.suggestedValue().length()); > > This will have the beginning of the suggestedValue as selectionRange starting > point, which means the whole suggestedValue is selected. It sounds another > reasonable solution for me. > > I'm not sure what's the requirement spec for this case - I had a search on w3c > drafts but couldn't find anything about this. So, after patching this in locally and testing, I realized that setting the suggestedValue clears the selection, so what I really meant was: int selection_start = username_element.selectionStart(); username_element.setSuggestedValue(username); username_element.setSelectionRange( selection_start, username_element.suggestedValue().length()); Also, I realized from testing locally that we should save the original selection and restore it in ClearPreview(), just as we do the autofill state. In addition to making these changes to the code, please add test coverage for them. Thanks!
On 2014/05/16 22:24:06, Ilya Sherman wrote: > https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... > File components/autofill/content/renderer/password_autofill_agent.cc (right): > > https://codereview.chromium.org/208453002/diff/260001/components/autofill/con... > components/autofill/content/renderer/password_autofill_agent.cc:414: // > characters. > On 2014/05/16 09:18:13, ziran.sun wrote: > > On 2014/05/15 21:36:08, Ilya Sherman wrote: > > > On 2014/05/15 12:36:37, ziran.sun wrote: > > > > On 2014/05/14 23:10:58, Ilya Sherman wrote: > > > > > On 2014/05/14 15:35:12, ziran.sun wrote: > > > > > > On 2014/05/13 00:44:05, Ilya Sherman wrote: > > > > > > > Hmm, does inline autocompletion actually call this (new) method? If > > so, > > > > > > what's > > > > > > > the full codepath that triggers the call? > > > > > > > > > > > > I don't think inline autocompletion calls this. The use case is > related > > to > > > > > > inline autocompletion though: > > > > > > > > > > > > Say user has two usernames: ziran_su & zi1234517. When type "zi" into > > > > > username > > > > > > field, the inline autocompletion auto-completes the field with > > "ziran_su". > > > > > When > > > > > > select "zi1234517" from the suggestion box, we will have "17" > > highlighted > > > as > > > > > the > > > > > > selection range regardless that "zi12345" doesn't match "ziran_su". I > > feel > > > > > that > > > > > > in this case the selection range should be "1234517", starting from > the > > > end > > > > of > > > > > > matching character between value and suggestedValue. > > > > > > > > > > I agree with you there. However, there's a simpler way to implement > that, > > > > > because we only ever show suggestions that match the full value that's > > been > > > > > typed: > > > > > > > > > > username_element.setSelectionRange(username_element.value().length(), > > > > > username_element.suggestedValue().length()); > > > > > > > > I'm not sure this works for the use case I described above. When the > inline > > > > autocompletion is in place, the value of the field is not always the same > as > > > we > > > > typed in, e.g. typing in "zi", the inline autocompletion auto-completes > the > > > > field with "ziran_su". However, the suggestion box still shows all the > > > > suggestions that match "zi" rather than "ziran_su", e.g. "zi1234517" is > > listed > > > > in the suggestion box as described above. If we take the length of the > value > > > of > > > > the username field as the selection range starting point, we only have > "17" > > > > highlighted while hovering over "zi1234517", which might not be what we > > want. > > > > > > > > Do let me know if I misses any points here though. > > > > > > Hmm, that's a fair point. How about this, then? > > > > > > username_element.setSelectionRange(username_element.selectionStart(), > > > > > username_element.suggestedValue().length()); > > > > This will have the beginning of the suggestedValue as selectionRange starting > > point, which means the whole suggestedValue is selected. It sounds another > > reasonable solution for me. > > > > I'm not sure what's the requirement spec for this case - I had a search on w3c > > drafts but couldn't find anything about this. > > So, after patching this in locally and testing, I realized that setting the > suggestedValue clears the selection, so what I really meant was: > > int selection_start = username_element.selectionStart(); > username_element.setSuggestedValue(username); > username_element.setSelectionRange( > selection_start, > username_element.suggestedValue().length()); > > Also, I realized from testing locally that we should save the original selection > and restore it in ClearPreview(), just as we do the autofill state. > > In addition to making these changes to the code, please add test coverage for > them. Thanks! This solution makes sense :). Thanks! Just updated code as per comments. Please let me know if it's okay.
Thanks, Ziran! Just a final two nits left: https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:411: selection_start_= username_element.selectionStart(); nit: Please leave a space before the '=' sign. https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:218: int selection_start_; nit: Please mention that this refers to the username element's selection range, likely by naming the variable something like "username_selection_start_".
Updated code. All review comments have been addressed. Please review. Thanks! https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:411: selection_start_= username_element.selectionStart(); On 2014/05/20 08:34:37, Ilya Sherman wrote: > nit: Please leave a space before the '=' sign. Done. https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/208453002/diff/340001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.h:218: int selection_start_; On 2014/05/20 08:34:37, Ilya Sherman wrote: > nit: Please mention that this refers to the username element's selection range, > likely by naming the variable something like "username_selection_start_". Done.
LGTM, thanks :)
The CQ bit was checked by isherman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/208453002/360001
Message was sent while issue was closed.
Change committed as 271993 |
