Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
140093005:
Add supports that allow Autofill to be initiated from textarea field (Closed)
|
Created:
6 years, 10 months ago by ziran.sun Modified:
6 years, 9 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd supports that allow Autofill to be initiated from textarea field
Handling both keyboard and mouse events in textarea that initiate
autofill preview and fill form actions. Add test cases from browser
tests aspect to cover page click and findformfieldfortextarea cases.
BUG=332557
R=isherman@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257476
Patch Set 1 #Patch Set 2 : Temporarily keep textFieldDidiChange(const WebInputElement&) due to Blink roll delay #
Total comments: 44
Patch Set 3 : Update code as per review comments #
Total comments: 24
Patch Set 4 : Update codes as per Ilya's 2nd set comments #
Total comments: 52
Patch Set 5 : Update code as per Ilya's 3rd set of review comments - all comments addressed #
Total comments: 10
Patch Set 6 : Update code as per Ilya's 4th set review comments #Patch Set 7 : Rebase and clean code using common methods introduced in WebFormControlElement interface #
Total comments: 27
Patch Set 8 : Rebase and update code as per Ilya's review comments #
Total comments: 2
Patch Set 9 : Rebase and addressed Ilya's final comment #Patch Set 10 : Condition wrap correction #
Total comments: 10
Messages
Total messages: 28 (0 generated)
Please review together with https://codereview.chromium.org/133443011/ Thanks!
Temporarily keep textFieldDidiChange(const WebInputElement&) due to Blink roll delay
lgtm for blink::WebAutofillClient update procedure.
https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... File components/autofill/content/renderer/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:295: } Is this method needed in addition to the TextAreaElementClicked() method, or can the two be merged into a single TextFormControlElementClicked() method? https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:299: bool is_focused) { Is is_focused needed? https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:315: // This function is to be removed once next Blink roll is done Please add a TODO. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:333: element)); Can you call into the other textFieldDidChange function from this one, rather than duplicating the code? https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:367: password_generation_agent_->TextDidChangeInTextField(*input_element)) { nit: Indent this line by two more spaces. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:368: return; nit: Indent this line by two fewer spaces. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:382: IsTextAreaElement(element)) && Please move the check for whether this is an autofillable input element or a textarea to the top of the method, and return early if not. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:410: WebInputElement* input_element = toWebInputElement(&element_); nit: Please add a DCHECK that this conversion succeeded, with a comment explaining that datalist suggestions should only appear in conjunction with input elements, not textareas. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:497: WebInputElement* input_element = toWebInputElement(&element_); Ditto. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:564: return; It's important that this follow the assignment of "element_ = element" https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:567: return; nit: Please include curly braces on the else, since they're present on the if. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:606: input_element->autoComplete() ? REQUIRE_AUTOCOMPLETE : REQUIRE_NONE; nit: Please add curly braces, since this spans multiple lines. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:611: &field, requirements)) { nit: This is a strange way to wrap the line. Probably keep "&field" on the line above. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.cc:656: node.toConst<WebFormControlElement>(), nit: This should be indented four spaces from the 'F' in "Find" above, i.e. five more spaces than it currently is. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... File components/autofill/content/renderer/autofill_agent.h (right): https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.h:92: // To be removed once next Blink roll is done Please annotate with a TODO. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/autofill_agent.h:95: // OVERRIDE this function once next Blink roll is done Please annotate with a TODO. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... File components/autofill/content/renderer/form_autofill_util.cc (right): https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/form_autofill_util.cc:799: // Nothing more to do in this case. nit: This comment is no longer relevant. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/form_autofill_util.cc:801: field->is_focusable = element.isFocusable(); You should set should_autocomplete and text_direction as well. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/form_autofill_util.cc:1123: } There is a lot of repeated code throughout this file to handle the case of being an initiating node. Is it possible to share more of that code, possibly by decomposing a helper method or two? https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... File components/autofill/content/renderer/page_click_tracker.cc (right): https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/page_click_tracker.cc:60: return WebTextAreaElement(); Why might this code be reached? Seems like any element that has a tag name of "textarea" would be a textarea element. https://chromiumcodereview.appspot.com/140093005/diff/130001/components/autof... components/autofill/content/renderer/page_click_tracker.cc:118: was_focused_, is_focused); nit: This if-stmt needs curly braces, since it spans multiple lines.
Updated code as per review comments. Thanks! https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:295: } On 2014/02/14 02:44:40, Ilya Sherman wrote: > Is this method needed in addition to the TextAreaElementClicked() method, or can > the two be merged into a single TextFormControlElementClicked() method? Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:299: bool is_focused) { On 2014/02/14 02:44:40, Ilya Sherman wrote: > Is is_focused needed? I'm not sure. It doesn't seem that it's called here. But I wonder why this variable was introduced... https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:315: // This function is to be removed once next Blink roll is done On 2014/02/14 02:44:40, Ilya Sherman wrote: > Please add a TODO. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:333: element)); On 2014/02/14 02:44:40, Ilya Sherman wrote: > Can you call into the other textFieldDidChange function from this one, rather > than duplicating the code? Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:367: password_generation_agent_->TextDidChangeInTextField(*input_element)) { On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: Indent this line by two more spaces. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:368: return; On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: Indent this line by two fewer spaces. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:382: IsTextAreaElement(element)) && On 2014/02/14 02:44:40, Ilya Sherman wrote: > Please move the check for whether this is an autofillable input element or a > textarea to the top of the method, and return early if not. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:410: WebInputElement* input_element = toWebInputElement(&element_); On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: Please add a DCHECK that this conversion succeeded, with a comment > explaining that datalist suggestions should only appear in conjunction with > input elements, not textareas. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:497: WebInputElement* input_element = toWebInputElement(&element_); On 2014/02/14 02:44:40, Ilya Sherman wrote: > Ditto. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:564: return; On 2014/02/14 02:44:40, Ilya Sherman wrote: > It's important that this follow the assignment of "element_ = element" Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:567: return; On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: Please include curly braces on the else, since they're present on the if. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:606: input_element->autoComplete() ? REQUIRE_AUTOCOMPLETE : REQUIRE_NONE; On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: Please add curly braces, since this spans multiple lines. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:611: &field, requirements)) { On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: This is a strange way to wrap the line. Probably keep "&field" on the line > above. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:656: node.toConst<WebFormControlElement>(), On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: This should be indented four spaces from the 'F' in "Find" above, i.e. five > more spaces than it currently is. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:92: // To be removed once next Blink roll is done On 2014/02/14 02:44:40, Ilya Sherman wrote: > Please annotate with a TODO. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:95: // OVERRIDE this function once next Blink roll is done On 2014/02/14 02:44:40, Ilya Sherman wrote: > Please annotate with a TODO. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:799: // Nothing more to do in this case. On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: This comment is no longer relevant. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:801: field->is_focusable = element.isFocusable(); On 2014/02/14 02:44:40, Ilya Sherman wrote: > You should set should_autocomplete and text_direction as well. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1123: } On 2014/02/14 02:44:40, Ilya Sherman wrote: > There is a lot of repeated code throughout this file to handle the case of being > an initiating node. Is it possible to share more of that code, possibly by > decomposing a helper method or two? https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1123: } On 2014/02/14 02:44:40, Ilya Sherman wrote: > There is a lot of repeated code throughout this file to handle the case of being > an initiating node. Is it possible to share more of that code, possibly by > decomposing a helper method or two? Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:60: return WebTextAreaElement(); On 2014/02/14 02:44:40, Ilya Sherman wrote: > Why might this code be reached? Seems like any element that has a tag name of > "textarea" would be a textarea element. Done. https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:118: was_focused_, is_focused); On 2014/02/14 02:44:40, Ilya Sherman wrote: > nit: This if-stmt needs curly braces, since it spans multiple lines. Done.
https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/130001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:299: bool is_focused) { On 2014/02/17 15:43:45, ziran.sun wrote: > On 2014/02/14 02:44:40, Ilya Sherman wrote: > > Is is_focused needed? > > I'm not sure. It doesn't seem that it's called here. But I wonder why this > variable was introduced... Let's go ahead and remove it if it's not used anymore. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:55: using blink::WebElementCollection; nit: While you're here, mind alpha-sorting this line? https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:297: return; nit: Please leave a blank line after this one. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:302: // This applies to both input and textarea elements nit: This comment doesn't seem very useful to anyone who's not thinking about it in context of this CL. I'd omit it. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:320: // Text changes in input text field or textarea field Ditto. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:342: { nit: In the Chromium repository, curly braces should always be on the same line as the closing paren. In this case, that means that the function parameter needs to wrap. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:398: return; nit: Please leave a blank line after this one. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:487: return; nit: Please leave a blank line after this one. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:529: const WebInputElement* input_element = toWebInputElement(&element); nit: Please move this down to where it is used. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:547: static_cast<int>(value.length()))))) { All of these criteria should apply to text areas as well as to single-line text fields. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:598: } Equivalent logic should apply to textarea fields as well. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:621: &data_list_labels); nit: Please fix alignment for these three lines. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:89: // TODO: To be removed once next Blink roll is done nit: TODO's should be formatted as "TODO(ziran.sun):". Ditto below. Though, in this case, I thought the necessary Blink CL had already landed, right? If so, let's just go ahead and address these TODOs now. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:441: void HandleInitiatingNode(WebFormControlElement& element, bool preview) { nit: Pass-by-reference is almost entirely disallowed by the Chromium style guide. In this case, I'd recommend either pass-by-copy or pass-by-pointer. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:467: } This method still makes it hard to tell that the code is doing the same thing for both input elements and textarea elements. Please decompose methods along the lines of WebString GetValue(const WebFormControlElement& element); WebString GetSuggestedValue(const WebFormControlElement& element); void SetSelectionRange(WebFormControlElement element, int start, int end); https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:617: if (is_initiating_node) nit: Curly braces are still needed, since the body of the loop spans two lines of text. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:625: if (is_initiating_node) nit: Curly braces are still needed, since the body of the loop spans two lines of text. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:826: base::i18n::RIGHT_TO_LEFT : base::i18n::LEFT_TO_RIGHT; Is it possible to refactor the code so that it is shared across the two branches of the if/else, rather than duplicated? https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1130: HandleInitiatingNode(*input_element, false); This change doesn't seem appropriate. Is there some specific bug that this is meant to be addressing? https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1132: else { nit: The else should be on the previous line, i.e. "} else {" https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/page_click_listener.h (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/page_click_listener.h:27: // If the previously focused element was an input or textarea field, nit: "an input or textarea field" -> "an input field or a textarea" https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:58: element.toConst<WebFormControlElement>(); Can we skip this cast, and cast directly to a WebTextAreaElement? https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:112: if (!input_element.isNull()) nit: This if-stmt's body spans multiple lines of code, so please add curly braces. https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:116: if (!textarea_element.isNull()) { nit: This can be an else-if.
Update code as per Ilya's 2nd set comments. Please review together with updates in https://codereview.chromium.org/130213005/ Many thanks! https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/140093005/diff/280001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:89: // TODO: To be removed once next Blink roll is done On 2014/02/22 06:24:20, Ilya Sherman wrote: > nit: TODO's should be formatted as "TODO(ziran.sun):". Ditto below. Though, in > this case, I thought the necessary Blink CL had already landed, right? If so, > let's just go ahead and address these TODOs now. textFieldDidChange() change is not in yet. More changes have also been added today to be in line with the code updates in Chromium side. https://codereview.chromium.org/133443011/
Please try to reply to every review comment, either by clicking the "Done" link to add a simple "Done." comment or by writing a more detailed response. This helps me as a reviewer have an easier time keeping track of what's been addressed and what's still outstanding. Thanks :) https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:349: return; Please move this into textFieldDidChange() instead. It should precede any modifications to did_set_node_text_. Also, can this be a DCHECK, rather than an early return? If not, the method name is probably misleading. When exactly can this trigger? https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:396: return; nit: Can this be a DCHECK rather than an early return? https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:486: return; nit: Can this be a DCHECK rather than an early return? https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:546: nit: Please omit this blank line. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:570: } nit: Please revert the indentation change for these four lines. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:602: const WebInputElement* input_element = toWebInputElement(&element); nit: Please move this to be just above line 610. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:613: } else if (IsTextAreaElement(element)) { nit: Can this be a DCHECK rather than an if? https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:24: #include "third_party/WebKit/public/web/WebTextAreaElement.h" nit: Doesn't look like this is needed. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:441: WebString GetValue(const WebFormControlElement& element) { nit: Please document each new function you add. For functions like this one, a very brief documentation string like "// A simple wrapper to get an element's value." helps the reader know that there's nothing surprising going on. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:455: } You can reformat this method to be briefer and clearer by writing it like as I have below. There are several types of changes: (1) No else stmts after "if (...) return ...;" (2) Removing duplicated lines, like 446-448. (3) Removing unneeded curly braces after (1) and (2) WebString GetValue(const WebFormControlElement& element) { const WebInputElement* input_element = toWebInputElement(&element); if (IsAutofillableInputElement(input_element)) return input_element->value(); if (IsTextAreaElement(element)) return element.toConst<WebTextAreaElement>().value(); DCHECK(IsSelectElement(element)); return element.toConst<WebSelectElement>().value(); } https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:467: } Please similarly reformat this method as well as the ones below. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:475: element.to<WebTextAreaElement>(); nit: This looks like it fits on a single line. It might help to get into the habit of running clang-format over your code -- that should take care of fixing up a lot of style nits automatically :) (That said, I would actually combine this with the line below, as element.to<WebTextAreaElement>().setSelectionRange(start, end);) https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:484: IsTextAreaElement(element)) { nit: Please write this as an early return, i.e. if (!IsAutofillableInputElement(input_element) && !IsTextAreaElement(element)) return; This way, you're able to reduce the level of indentation for the main body of the method, which helps a lot with readability. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:491: } else { nit: DCHECK(IsTextAreaElement(element); https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:620: ((input_element && nit: No need to check whether |input_element| is null, since both IsTextInput() and IsMonthInput() null-check their input argument. So, you can simplify lines 620-622 to have the same condition as line 597. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:622: IsMonthInput(input_element))) || nit: IMO the code is a bit easier to read if this line isn't wrapped. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:626: field->document().frame()->unmarkText(); nit: Please preserve the comment "// Clear the current IME composition (the underline), if there is one." above this line. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:627: } Thanks for factoring out this shared logic -- it's much clearer now what's happening! :) https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:658: IsTextAreaElement(*field))) { nit: Please format this as: if (is_initiating_node && (IsTextInput(input_element) || IsTextAreaElement(*field))) { (So, remove the |input_element| null check, which is redundant with the IsTextInput() check, and unwrap the IsTextAreaElement() check.) https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:866: value = GetValue(element); nit: Please combine these two lines into one, i.e. base::string16 value = GetValue(element); https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:870: element.toConst<WebSelectElement>(); nit: This line probably doesn't need to wrap. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1158: } Please move this code as well as the comment outside of the if/else stmt, as you've done above. This has two advantages: (1) It's harder to have a bug in just one of the two branches, when they are meant to be identical. (2) It's easier for a reader to tell that the two branches are meant to be identical. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/page_click_listener.h (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_listener.h:26: // If the previously focused element was an input field or a textarea field, nit: "textarea field" -> "textarea" https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:59: return text_area; nit: Lines 57 through 59 can be shortened to just "return element.toConst<WebTextAreaElement>();" https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:111: was_focused_); nit: Doesn't look like this line needs to wrap any more. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:114: was_focused_); nit: Doesn't look like this line needs to wrap any more.
Update code as per Ilya's 3rd set of comments. All comments have been addressed. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:349: return; On 2014/02/26 00:12:07, Ilya Sherman wrote: > Please move this into textFieldDidChange() instead. It should precede any > modifications to did_set_node_text_. Also, can this be a DCHECK, rather than an > early return? If not, the method name is probably misleading. When exactly can > this trigger? Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:396: return; On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Can this be a DCHECK rather than an early return? Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:486: return; On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Can this be a DCHECK rather than an early return? Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:546: On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please omit this blank line. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:570: } On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please revert the indentation change for these four lines. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:602: const WebInputElement* input_element = toWebInputElement(&element); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please move this to be just above line 610. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:613: } else if (IsTextAreaElement(element)) { On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Can this be a DCHECK rather than an if? Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.h (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/autofill_agent.h:24: #include "third_party/WebKit/public/web/WebTextAreaElement.h" On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Doesn't look like this is needed. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:441: WebString GetValue(const WebFormControlElement& element) { On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please document each new function you add. For functions like this one, a > very brief documentation string like "// A simple wrapper to get an element's > value." helps the reader know that there's nothing surprising going on. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:455: } On 2014/02/26 00:12:07, Ilya Sherman wrote: > You can reformat this method to be briefer and clearer by writing it like as I > have below. There are several types of changes: > > (1) No else stmts after "if (...) return ...;" > (2) Removing duplicated lines, like 446-448. > (3) Removing unneeded curly braces after (1) and (2) > > WebString GetValue(const WebFormControlElement& element) { > const WebInputElement* input_element = toWebInputElement(&element); > if (IsAutofillableInputElement(input_element)) > return input_element->value(); > > if (IsTextAreaElement(element)) > return element.toConst<WebTextAreaElement>().value(); > > DCHECK(IsSelectElement(element)); > return element.toConst<WebSelectElement>().value(); > } Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:467: } On 2014/02/26 00:12:07, Ilya Sherman wrote: > Please similarly reformat this method as well as the ones below. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:475: element.to<WebTextAreaElement>(); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: This looks like it fits on a single line. It might help to get into the > habit of running clang-format over your code -- that should take care of fixing > up a lot of style nits automatically :) > > (That said, I would actually combine this with the line below, as > element.to<WebTextAreaElement>().setSelectionRange(start, end);) Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:484: IsTextAreaElement(element)) { On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please write this as an early return, i.e. > > if (!IsAutofillableInputElement(input_element) && > !IsTextAreaElement(element)) > return; > > This way, you're able to reduce the level of indentation for the main body of > the method, which helps a lot with readability. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:491: } else { On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: DCHECK(IsTextAreaElement(element); Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:620: ((input_element && On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: No need to check whether |input_element| is null, since both IsTextInput() > and IsMonthInput() null-check their input argument. So, you can simplify lines > 620-622 to have the same condition as line 597. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:622: IsMonthInput(input_element))) || On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: IMO the code is a bit easier to read if this line isn't wrapped. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:626: field->document().frame()->unmarkText(); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please preserve the comment "// Clear the current IME composition (the > underline), if there is one." above this line. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:627: } On 2014/02/26 00:12:07, Ilya Sherman wrote: > Thanks for factoring out this shared logic -- it's much clearer now what's > happening! :) Thanks for helping me with this :). https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:658: IsTextAreaElement(*field))) { On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please format this as: > > if (is_initiating_node && > (IsTextInput(input_element) || IsTextAreaElement(*field))) { > > (So, remove the |input_element| null check, which is redundant with the > IsTextInput() check, and unwrap the IsTextAreaElement() check.) Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:866: value = GetValue(element); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Please combine these two lines into one, i.e. base::string16 value = > GetValue(element); Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:870: element.toConst<WebSelectElement>(); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: This line probably doesn't need to wrap. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1158: } On 2014/02/26 00:12:07, Ilya Sherman wrote: > Please move this code as well as the comment outside of the if/else stmt, as > you've done above. This has two advantages: > > (1) It's harder to have a bug in just one of the two branches, when they are > meant to be identical. > (2) It's easier for a reader to tell that the two branches are meant to be > identical. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/page_click_listener.h (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_listener.h:26: // If the previously focused element was an input field or a textarea field, On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: "textarea field" -> "textarea" Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:59: return text_area; On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Lines 57 through 59 can be shortened to just "return > element.toConst<WebTextAreaElement>();" Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:111: was_focused_); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Doesn't look like this line needs to wrap any more. Done. https://codereview.chromium.org/140093005/diff/380001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:114: was_focused_); On 2014/02/26 00:12:07, Ilya Sherman wrote: > nit: Doesn't look like this line needs to wrap any more. Done.
Thanks, this is looking pretty good :) (Note, mostly for myself: I haven't yet looked at the test code changes, nor manually tested the functionality to double-check a couple of edge cases.) https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:329: DCHECK(IsTextAreaElement(element)); nit: You can shorten these five lines to DCHECK(IsAutofillableElement(element) && !IsSelectElement(element)); or to DCHECK(IsAutofillableInputElement(toWebInputElement(&element)) || IsTextAreaElement(element)); https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:539: } else if (IsTextAreaElement(element)) { nit: Can this be a DCHECK rather than an if? https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:574: password_autofill_agent_->ShowSuggestions(*input_element)) Please preserve the line "is_popup_possibly_visible_ = true;" https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1137: bool is_initiating_node; Please make sure to initialize this variable here, i.e. set it to false by default. https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:111: } nit: No need for curly braces.
Update code and all comments have been addressed. Please review. Thanks! https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:329: DCHECK(IsTextAreaElement(element)); On 2014/03/01 02:50:53, Ilya Sherman wrote: > nit: You can shorten these five lines to > > DCHECK(IsAutofillableElement(element) && !IsSelectElement(element)); > > or to > > DCHECK(IsAutofillableInputElement(toWebInputElement(&element)) || > IsTextAreaElement(element)); Done. https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:539: } else if (IsTextAreaElement(element)) { On 2014/03/01 02:50:53, Ilya Sherman wrote: > nit: Can this be a DCHECK rather than an if? Done. https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:574: password_autofill_agent_->ShowSuggestions(*input_element)) On 2014/03/01 02:50:53, Ilya Sherman wrote: > Please preserve the line "is_popup_possibly_visible_ = true;" Done. https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1137: bool is_initiating_node; On 2014/03/01 02:50:53, Ilya Sherman wrote: > Please make sure to initialize this variable here, i.e. set it to false by > default. Done. https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... File components/autofill/content/renderer/page_click_tracker.cc (right): https://codereview.chromium.org/140093005/diff/400001/components/autofill/con... components/autofill/content/renderer/page_click_tracker.cc:111: } On 2014/03/01 02:50:53, Ilya Sherman wrote: > nit: No need for curly braces. Done.
Cleaned code using common methods newly introduced in WebFormControlElement interface. Please review. Thanks!
Lots of little comments, but otherwise looks great. Thanks, Ziran! https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:24: formcontrol_element_lost_focus_called_(false), nit: "formcontrol" -> "form_control" https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:40: void ClearFormControlResults() { nit: The previous name of "ClearResults()" was slightly better, IMO, as it was briefer but equally clear. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:108: // Tests that PageClickTracker does notify correctly when a input nit: "a input" -> "an input" https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:156: nit: Please revert this diff. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:225: } nit: Please add a blank line after this one. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:227: nit: Please revert this diff. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:538: } nit: Please preserve the blank line after this one. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:573: IsTextAreaElement(element)) { Hmm, is this code reachable for any other cases? Seems like this could be a DCHECK instead. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:615: } nit: Please preserve the blank line after this one. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:510: ((IsAutofillableInputElement(input_element) || nit: No need for the first parenthesis on this line. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1075: control_element.suggestedValue().isEmpty())) nit: Can this be shortened to the following? if ((IsTextInput(input_element) || IsMonthInput(input_element) || IsTextAreaElement(control_element)) && control_element.suggestedValue().isEmpty())) { ... } https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1080: bool is_initiating_node = false; nit: Can the logic be extracted out to here as "bool is_initiating_node = (element == control_element);"? https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1087: input_element->setAutofilled(false); nit: Please share these four lines between input elements and text areas.
Rebased and Updated code as per Ilya's comments. Please review. Thanks! https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... File chrome/renderer/autofill/page_click_tracker_browsertest.cc (right): https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:24: formcontrol_element_lost_focus_called_(false), On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: "formcontrol" -> "form_control" Done. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:40: void ClearFormControlResults() { On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: The previous name of "ClearResults()" was slightly better, IMO, as it was > briefer but equally clear. Done. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:108: // Tests that PageClickTracker does notify correctly when a input On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: "a input" -> "an input" Done. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:156: On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please revert this diff. Done. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:225: } On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please add a blank line after this one. Done. https://codereview.chromium.org/140093005/diff/440001/chrome/renderer/autofil... chrome/renderer/autofill/page_click_tracker_browsertest.cc:227: On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please revert this diff. Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:538: } On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please preserve the blank line after this one. Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:573: IsTextAreaElement(element)) { On 2014/03/14 07:24:46, Ilya Sherman wrote: > Hmm, is this code reachable for any other cases? Seems like this could be a > DCHECK instead. Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:615: } On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please preserve the blank line after this one. Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:510: ((IsAutofillableInputElement(input_element) || On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: No need for the first parenthesis on this line. I feel that it's still needed. The condition is - element is either input element or textarea element, and the element is not empty. Or have I missed something here? https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1075: control_element.suggestedValue().isEmpty())) On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Can this be shortened to the following? > > if ((IsTextInput(input_element) || > IsMonthInput(input_element) || > IsTextAreaElement(control_element)) && > control_element.suggestedValue().isEmpty())) { > ... > } Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1080: bool is_initiating_node = false; On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Can the logic be extracted out to here as "bool is_initiating_node = > (element == control_element);"? Done. https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1087: input_element->setAutofilled(false); On 2014/03/14 07:24:46, Ilya Sherman wrote: > nit: Please share these four lines between input elements and text areas. Done.
Thanks! LGTM with a final nit addressed: https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/140093005/diff/440001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:510: ((IsAutofillableInputElement(input_element) || On 2014/03/14 17:01:42, ziran.sun wrote: > On 2014/03/14 07:24:46, Ilya Sherman wrote: > > nit: No need for the first parenthesis on this line. > > I feel that it's still needed. The condition is - element is either input > element or textarea element, and the element is not empty. Or have I missed > something here? Sorry, you're absolutely right. Thanks for correcting me :) https://codereview.chromium.org/140093005/diff/460001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/460001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:576: element.autoComplete() ? REQUIRE_AUTOCOMPLETE : REQUIRE_NONE; nit: please combine this line with the previous one, i.e. restore the code to its original state.
https://codereview.chromium.org/140093005/diff/460001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/460001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:576: element.autoComplete() ? REQUIRE_AUTOCOMPLETE : REQUIRE_NONE; On 2014/03/14 21:42:55, Ilya Sherman wrote: > nit: please combine this line with the previous one, i.e. restore the code to > its original state. Done.
The CQ bit was checked by ziran.sun@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/140093005/480001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by ziran.sun@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/140093005/500001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by ziran.sun@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/140093005/500001
Message was sent while issue was closed.
Change committed as 257476
Message was sent while issue was closed.
https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:290: if (!IsAutofillableInputElement(input_element) && Here too. https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:319: DCHECK(IsAutofillableInputElement(toWebInputElement(&element)) || This DCHECK is incorrect, as the element might not be an autofillable input element. I think this should be DCHECK(toWebInputElement(&element) || IsTextAreaElement(element)); https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:346: if (IsAutofillableInputElement(input_element)) { This should just be if (input_element) https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:390: DCHECK(IsAutofillableInputElement(input_element)); This should just be DCHECK(input_element); https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:466: DCHECK(IsAutofillableInputElement(input_element)); Here too. https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:473: DCHECK(IsAutofillableInputElement(input_element)); Here too. https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:517: if (IsAutofillableInputElement(input_element)) { This should be a check for if (input_element), not if (IsAutofillableInputElement(input_element)) https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:543: if (IsAutofillableInputElement(input_element) && This should be a check for just if (input_element) https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:571: DCHECK(IsAutofillableInputElement(toWebInputElement(&element)) || Here too https://codereview.chromium.org/140093005/diff/500001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:601: if (IsAutofillableInputElement(input_element)) { Here too. |
