Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
112663005:
Add autofill preview support for Textarea (Closed)
|
Created:
6 years, 12 months ago by ziran.sun Modified:
6 years, 11 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd autofill preview support for Textarea
BUG=314976
R=isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243977
Patch Set 1 #
Total comments: 17
Patch Set 2 : Update code as per review comments #Patch Set 3 : Add test code #
Total comments: 20
Patch Set 4 : Update code as per review comments #Patch Set 5 : Update testcode for preview form to use kFormHtml rather than a custom form #
Total comments: 5
Patch Set 6 : Change <option> values in <select> to its existing ones #Patch Set 7 : minor change - use defined variable #
Messages
Total messages: 21 (0 generated)
Thanks for the patch! Some comments below: https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:589: } Can this be shared across the two branches? https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:598: } nit: No need for the final "else if" nor for the "else". https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:977: FormFieldData* field, nit: Spurious diff, please revert. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1060: // text input and textarea elements can be previewed. nit: Please keep the word "only" https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1065: // If the control element is not auto-filled, we did not preview it, nit: I'd omit "control", i.e. simply "If the element is not autofilled, ..." https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1077: control_elements[i].to<WebTextAreaElement>().suggestedValue().isEmpty())) nit: The text wrapping here doesn't match the Chromium style guide's recommendations. I would recommend either decomposing a helper method, or formatting the code as something like: WebFormControlElement control_element = control_elements[i]; if ((IsTextInput(input_element) && input_element->suggestedValue().isEmpty()) || (IsTextAreaElement(control_element) && control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) continue; https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1088: input_element->setAutofilled(false); Can this code be shared between the two branches? https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1098: WebTextAreaElement txtarea = control_elements[i].to<WebTextAreaElement>(); nit: Please avoid abbreviations in names. The way to format this line is WebTextAreaElement text_area = control_elements[i].to<WebTextAreaElement>();
lgtm
lgtm
lgtm
lgtm
lgtm
lgtm
lgtm
https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:589: } On 2013/12/29 03:36:02, Ilya Sherman (Away thru Jan 6) wrote: > Can this be shared across the two branches? Do you mean merge "if(IsTextInput(input_element))" & "else if (IsTextAreaElement(*field))"?
Code updated as per comments. Please let me know if it's okay. Many thanks.
Add test code. Please review.
Thanks! https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:589: } On 2014/01/02 15:22:54, ziran.sun wrote: > On 2013/12/29 03:36:02, Ilya Sherman (Away thru Jan 6) wrote: > > Can this be shared across the two branches? > > Do you mean merge "if(IsTextInput(input_element))" & "else if > (IsTextAreaElement(*field))"? It seems like the "if (is_initiating_node)" code ought to be relevant for both branches. https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:270: return value; nit: No need for the local variable value, i.e. you can express this more concisely as: static WebString GetValueWrapper(WebFormControlElement element) { if (element.formControlType() == "textarea") return element.to<WebTextAreaElement>().value(); return element.to<WebInputElement>().value(); } https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:280: } Likewise for this method. https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1196: "</FORM>"; Please update this test to use kFormHtml rather than a custom form (unless I'm missing a reason why that's quite a pain to do currently). https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:584: input_element->setAutofilled(true); nit: Please de-indent this line two spaces. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:588: input_element->suggestedValue().length()); nit: Indentation is off here. Please format this as below, so that the arguments are aligned: input_element->setSelectionRange( input_element->value().length(), input_element->suggestedValue().length()); https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1070: WebFormControlElement control_element = control_elements[i]; nit: I'd recommend moving this to the top of the loop, so that you can use it on lines 1057, 1058, and 1063. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1074: control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) This is still not wrapped correctly. Specifically, (1) when wrapping code at operators like "||" and "&&", the operators should always appear at the end of the wrapped line, rather than at the start of the next line; and (2) parts of a single expression that is &&'ed or ||'ed together should be aligned. Again, the appropriate formatting for this if-stmt is WebFormControlElement control_element = control_elements[i]; if ((IsTextInput(input_element) && input_element->suggestedValue().isEmpty()) || (IsTextAreaElement(control_element) && control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) continue; https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1085: input_element->setAutofilled(false); Lines 1081-1085 are essentially duplicated on lines 1097-1101. Please refactor the code so that they are shared instead, unless this is not possible (or is possible but looks ugly).
By the way, please note that when addressing review comments, it's very helpful to click the "Done" link. This helps your reviewers keep track of which comments are resolved vs. which ones still need attention.
https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:589: } On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > On 2014/01/02 15:22:54, ziran.sun wrote: > > On 2013/12/29 03:36:02, Ilya Sherman (Away thru Jan 6) wrote: > > > Can this be shared across the two branches? > > > > Do you mean merge "if(IsTextInput(input_element))" & "else if > > (IsTextAreaElement(*field))"? > > It seems like the "if (is_initiating_node)" code ought to be relevant for both > branches. I am not sure if we need to do "setSelectionRange" for Textarea as well. check "FillFormField()", it doesn't seem this is called for Textarea. Do let me know if I missed any points though. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:598: } On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: No need for the final "else if" nor for the "else". Done. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:977: FormFieldData* field, On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Spurious diff, please revert. Done. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1060: // text input and textarea elements can be previewed. On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please keep the word "only" Done. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1065: // If the control element is not auto-filled, we did not preview it, On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: I'd omit "control", i.e. simply "If the element is not autofilled, ..." Done. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1077: control_elements[i].to<WebTextAreaElement>().suggestedValue().isEmpty())) On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: The text wrapping here doesn't match the Chromium style guide's > recommendations. I would recommend either decomposing a helper method, or > formatting the code as something like: > > WebFormControlElement control_element = control_elements[i]; > if ((IsTextInput(input_element) && > input_element->suggestedValue().isEmpty()) || > (IsTextAreaElement(control_element) && > control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) > continue; Done. https://codereview.chromium.org/112663005/diff/1/components/autofill/content/... components/autofill/content/renderer/form_autofill_util.cc:1098: WebTextAreaElement txtarea = control_elements[i].to<WebTextAreaElement>(); On 2013/12/29 03:36:02, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please avoid abbreviations in names. The way to format this line is > > WebTextAreaElement text_area = > control_elements[i].to<WebTextAreaElement>(); Done. https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:270: return value; On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: No need for the local variable value, i.e. you can express this more > concisely as: > > static WebString GetValueWrapper(WebFormControlElement element) { > if (element.formControlType() == "textarea") > return element.to<WebTextAreaElement>().value(); > > return element.to<WebInputElement>().value(); > } Done. https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:280: } On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > Likewise for this method. Done. https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1196: "</FORM>"; On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > Please update this test to use kFormHtml rather than a custom form (unless I'm > missing a reason why that's quite a pain to do currently). I think selection preview is not supported from this discussion thread: http://code.google.com/p/chromium/issues/detail?id=58719 Again, do let me know if I missed any points here. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:584: input_element->setAutofilled(true); On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please de-indent this line two spaces. Done. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:588: input_element->suggestedValue().length()); On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Indentation is off here. Please format this as below, so that the > arguments are aligned: > > input_element->setSelectionRange( > input_element->value().length(), > input_element->suggestedValue().length()); Done. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1070: WebFormControlElement control_element = control_elements[i]; On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: I'd recommend moving this to the top of the loop, so that you can use it on > lines 1057, 1058, and 1063. Done. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1074: control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > This is still not wrapped correctly. Specifically, (1) when wrapping code at > operators like "||" and "&&", the operators should always appear at the end of > the wrapped line, rather than at the start of the next line; and (2) parts of a > single expression that is &&'ed or ||'ed together should be aligned. Again, the > appropriate formatting for this if-stmt is > > WebFormControlElement control_element = control_elements[i]; > if ((IsTextInput(input_element) && > input_element->suggestedValue().isEmpty()) || > (IsTextAreaElement(control_element) && > control_element.to<WebTextAreaElement>().suggestedValue().isEmpty())) > continue; Done. https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1085: input_element->setAutofilled(false); On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > Lines 1081-1085 are essentially duplicated on lines 1097-1101. Please refactor > the code so that they are shared instead, unless this is not possible (or is > possible but looks ugly). This could look ugly. Check other functions, e.g. FillFormField(), it might look a bit clear while handle each of these cases (input, textarea, selection) separately?
Thanks! One last comment, then this should be good to land :) https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1196: "</FORM>"; On 2014/01/07 16:40:41, ziran.sun wrote: > On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > > Please update this test to use kFormHtml rather than a custom form (unless I'm > > missing a reason why that's quite a pain to do currently). > > I think selection preview is not supported from this discussion thread: > http://code.google.com/p/chromium/issues/detail?id=58719 That's true, but I think we could still have test coverage for <select> fields -- the test would simply verify that previewing is a no-op for these fields. I admit that's a little silly, but IMO the advantages of sharing the test cases implicit in the kFormHtml markup outweigh this minor wrinkle. Note that you'll probably need to special-case <select> fields in GetSuggestedValueWrapper(), possibly just always returning an empty string for now. (Yes, that means the test isn't really testing anything "real". IMO that's ok, because there are only two <select> fields that we're hacking around, but an order of magnitude more test cases that we gain by sharing the input test cases.) https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/112663005/diff/170001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1085: input_element->setAutofilled(false); On 2014/01/07 16:40:41, ziran.sun wrote: > On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > > Lines 1081-1085 are essentially duplicated on lines 1097-1101. Please > refactor > > the code so that they are shared instead, unless this is not possible (or is > > possible but looks ugly). > > This could look ugly. Check other functions, e.g. FillFormField(), it might > look a bit clear while handle each of these cases (input, textarea, selection) > separately? Alright, I guess it's ok as is. Thanks.
https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1196: "</FORM>"; On 2014/01/08 04:13:47, Ilya Sherman (Away Jan11-Feb2) wrote: > On 2014/01/07 16:40:41, ziran.sun wrote: > > On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > > > Please update this test to use kFormHtml rather than a custom form (unless > I'm > > > missing a reason why that's quite a pain to do currently). > > > > I think selection preview is not supported from this discussion thread: > > http://code.google.com/p/chromium/issues/detail?id=58719 > > That's true, but I think we could still have test coverage for <select> fields > -- the test would simply verify that previewing is a no-op for these fields. I > admit that's a little silly, but IMO the advantages of sharing the test cases > implicit in the kFormHtml markup outweigh this minor wrinkle. > > Note that you'll probably need to special-case <select> fields in > GetSuggestedValueWrapper(), possibly just always returning an empty string for > now. (Yes, that means the test isn't really testing anything "real". IMO > that's ok, because there are only two <select> fields that we're hacking around, > but an order of magnitude more test cases that we gain by sharing the input test > cases.) I've made a note on <select> fields saying that preview is not yet supported. Adding a case in "GetSuggestedValueWrapper()" probably is not required yet. I guess that we can add <select> case for this function once we have Selection preview supported :). https://codereview.chromium.org/112663005/diff/170001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1196: "</FORM>"; On 2014/01/08 04:13:47, Ilya Sherman (Away Jan11-Feb2) wrote: > On 2014/01/07 16:40:41, ziran.sun wrote: > > On 2014/01/07 01:09:38, Ilya Sherman (Away Jan11-Feb2) wrote: > > > Please update this test to use kFormHtml rather than a custom form (unless > I'm > > > missing a reason why that's quite a pain to do currently). > > > > I think selection preview is not supported from this discussion thread: > > http://code.google.com/p/chromium/issues/detail?id=58719 > > That's true, but I think we could still have test coverage for <select> fields > -- the test would simply verify that previewing is a no-op for these fields. I > admit that's a little silly, but IMO the advantages of sharing the test cases > implicit in the kFormHtml markup outweigh this minor wrinkle. > > Note that you'll probably need to special-case <select> fields in > GetSuggestedValueWrapper(), possibly just always returning an empty string for > now. (Yes, that means the test isn't really testing anything "real". IMO > that's ok, because there are only two <select> fields that we're hacking around, > but an order of magnitude more test cases that we gain by sharing the input test > cases.) Done.
Thanks! LGTM % one remaining nit: https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1204: // Regular <input type="month"> should not be previewed. Hmm, this would be nice to fix as well (as a separate CL) -- no particularly good reason why these shouldn't be previewed, other than there's not support for it in Blink yet. https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1209: {"select-one", "select", "", "", false, "suggested TX", ""}, nit: Please omit "suggested " from "suggested TX" here and below -- <select> fields can only be filled with existing <option> values.
https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... File chrome/renderer/autofill/form_autofill_browsertest.cc (right): https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1204: // Regular <input type="month"> should not be previewed. On 2014/01/08 23:01:36, Ilya Sherman (Away Jan11-Feb2) wrote: > Hmm, this would be nice to fix as well (as a separate CL) -- no particularly > good reason why these shouldn't be previewed, other than there's not support for > it in Blink yet. I can investigate on this. https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1209: {"select-one", "select", "", "", false, "suggested TX", ""}, On 2014/01/08 23:01:36, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please omit "suggested " from "suggested TX" here and below -- <select> > fields can only be filled with existing <option> values. That's a good point - I wasn't thinking. Sorry about this. https://codereview.chromium.org/112663005/diff/320001/chrome/renderer/autofil... chrome/renderer/autofill/form_autofill_browsertest.cc:1209: {"select-one", "select", "", "", false, "suggested TX", ""}, On 2014/01/08 23:01:36, Ilya Sherman (Away Jan11-Feb2) wrote: > nit: Please omit "suggested " from "suggested TX" here and below -- <select> > fields can only be filled with existing <option> values. Done.
LGTM, thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/112663005/420001
Message was sent while issue was closed.
Change committed as 243977 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
