Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10024059:
DataList UI (Chromium part) (Closed)
|
Created:
8 years, 8 months ago by keishi Modified:
8 years, 8 months ago CC:
chromium-reviews, GeorgeY, dhollowa+watch_chromium.org, darin-cc_chromium.org, dyu1, brettw-cc_chromium.org, Ilya Sherman Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDataList UI (Chromium part)
WebKit part is here
http://codereview.chromium.org/10037002/
Example page for <datalist>
https://tinker.io/22681/191
BUG=
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132561
Patch Set 1 #
Total comments: 34
Patch Set 2 : fixed #
Total comments: 30
Patch Set 3 : Fixed #
Total comments: 8
Patch Set 4 : Patch #
Total comments: 3
Patch Set 5 : Updated for WebKit side changes #Patch Set 6 : Fixed #Patch Set 7 : Added type ahead for datalist #
Total comments: 10
Patch Set 8 : Fixed nits and tests #
Messages
Total messages: 15 (0 generated)
Hi, I am trying to implement a UI for the <datalist> on a text field. Other browsers seem datalist suggestions as extra items in their autofill popup, so I am hoping to do the same for Chrome. Here is my preliminary patch. I would like the autofill people to look at it to see if it's OK to pursue this direction. I know you are trying to move the popup to the browser process (Bug 51644) But it seems to be in active development so I want to implement it for the current popup menu for now. I saw that the uniqueID was already being used to distinguish the types of items, http://git.chromium.org/gitweb/?p=chromium/src.git;a=blob;f=chrome/browser/au... so I defined additional IDs to identify all the menu item types. http://codereview.chromium.org/10037002/patch/1/4 I had to do this because datalist entries shouldn't be removable, and I wanted to use an additional separator to separate the datalist entries and the autofill entries. Here is a mac build with datalist turned on https://docs.google.com/open?id=0BwRi59l_ri74a1N4d2NSYXlIdzg And here is the test html that I use. https://tinker.io/22681/191 Every browsers but Chrome has already implemented <datalist> so I really hope we can get it in fast.
The webkit part of the patch is here http://codereview.chromium.org/10037002/
On 2012/04/10 12:50:37, keishi1 wrote: > Hi, > > I am trying to implement a UI for the <datalist> on a text field. > > Other browsers seem datalist suggestions as extra items in their autofill popup, > so I am hoping to do the same for Chrome. > > Here is my preliminary patch. I would like the autofill people to look at it to > see if it's OK to pursue this direction. Generally, this seems ok. My primary concern is that we are currently implementing this in a Chromium-specific way, but might want to instead implement something that all WebKit browsers can use...q > I know you are trying to move the popup to the browser process (Bug 51644) > But it seems to be in active development so I want to implement it for the > current popup menu for now. > > I saw that the uniqueID was already being used to distinguish the types of > items, > http://git.chromium.org/gitweb/?p=chromium/src.git;a=blob;f=chrome/browser/au... > so I defined additional IDs to identify all the menu item types. > http://codereview.chromium.org/10037002/patch/1/4 > > I had to do this because datalist entries shouldn't be removable, and I wanted > to use an additional separator to separate the datalist entries and the autofill > entries. > > Here is a mac build with datalist turned on > https://docs.google.com/open?id=0BwRi59l_ri74a1N4d2NSYXlIdzg > > And here is the test html that I use. > https://tinker.io/22681/191 > > Every browsers but Chrome has already implemented <datalist> > so I really hope we can get it in fast. It doesn't look like Safari implements <datalist> either. Are you sure that we want a Chromium-specific implementation, rather than a general one for WebKit? https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.cc (left): https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:158: // User selected 'Autofill Options'. nit: Please preserve these comments. I admit that they're fairly clear from the code as written, but they still add some value in that they map to the user actions that cause us to reach the code. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:161: case WebKit::WarningMessageMenuItemID: If we are going to be relying on all warnings being assigned this constant id, please also update the code in chrome/browser/autofill/ (in particular, autofill_manager.cc and autofill_external_delegate.cc) to use this named constant. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:165: } nit: No need for curly braces. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:177: WebInputElement element = node.toConst<WebInputElement>(); nit: Perhaps we can skip this cast by using |autofill_query_element_|? Also, it seems rather bad that the constness of |node| is completely ignored... we should probably update the method signature not to take |node| as a const value. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:182: // Fill the values for the whole form. nit: It's probably a good idea to DCHECK_GT(item_id, 0) and mention in a comment that positive ids are unique ids for Autofill (vs. Autocomplete) suggestions. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:192: DCHECK_GE(item_id, 0); Is this still valid? Which sorts of items are selectable? Presumably at least password items -- with id "-2" -- should be, given the if-stmt immediately below. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:330: ids.push_back(WebKit::SeparatorMenuItemID); This was not actually showing up as a separator in the mac build [1] you linked to. Was that build compiled before you added this, or is there something going wrong here? [1] https://docs.google.com/open?id=0BwRi59l_ri74a1N4d2NSYXlIdzg https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:343: ids.push_back(WebKit::SeparatorMenuItemID); This can add two separators where we should have only one: the "Clear form" and "Autofill options" items should have only one separator above them (and no separators between them). https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:362: const std::vector<int>& item_ids) { nit: 80-col, wrapping https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:371: if (options.length() > 0) { nit: Is this check needed, or would it also be covered by the "!option.isNull()" check in the while loop? https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:384: } nit: This code has a lot of levels of nesting. That could be reduced by decomposing this to a helper function with early returns. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:386: if (v.size() > 0 && values.size() > 0) { nit: Please add a comment above this if-stmt -- something like "If there are both <datalist> items and Autofill suggestions, add a separator between them." https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:473: if (!element.isEnabled() || element.isReadOnly() || !element.isTextField() || element.isPasswordField() || !element.suggestedValue().isEmpty()) nit: 80-col https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:499: // If the field has no name, then we won't have values. nit: Please move this comment to be above the if-stmt. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.cc:501: CombineDataListEntriesAndShow(element, std::vector<string16>(), std::vector<string16>(), std::vector<string16>(), std::vector<int>()); nit: 80-col. https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.h:144: const std::vector<int>& unique_ids); nit: item_ids? https://chromiumcodereview.appspot.com/10024059/diff/1/chrome/renderer/autofi... chrome/renderer/autofill/autofill_agent.h:151: int item_id, nit: I would not rename this variable, as this should still be a /unique/ id.
> Generally, this seems ok. My primary concern is that we are currently > implementing this in a Chromium-specific way, but might want to instead > implement something that all WebKit browsers can use...q The popup menu UI code is different for each port so I don't see any part of this patch that can be shared with other ports. The non-UI parts of datalist is shared with all WebKit ports. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... File chrome/renderer/autofill/autofill_agent.cc (left): http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:158: // User selected 'Autofill Options'. On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: Please preserve these comments. I admit that they're fairly clear from the > code as written, but they still add some value in that they map to the user > actions that cause us to reach the code. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:161: case WebKit::WarningMessageMenuItemID: On 2012/04/11 00:18:04, Ilya Sherman wrote: > If we are going to be relying on all warnings being assigned this constant id, > please also update the code in chrome/browser/autofill/ (in particular, > autofill_manager.cc and autofill_external_delegate.cc) to use this named > constant. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:165: } On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: No need for curly braces. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:177: WebInputElement element = node.toConst<WebInputElement>(); On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: Perhaps we can skip this cast by using |autofill_query_element_|? Also, it > seems rather bad that the constness of |node| is completely ignored... we should > probably update the method signature not to take |node| as a const value. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:182: // Fill the values for the whole form. On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: It's probably a good idea to DCHECK_GT(item_id, 0) and mention in a comment > that positive ids are unique ids for Autofill (vs. Autocomplete) suggestions. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:192: DCHECK_GE(item_id, 0); On 2012/04/11 00:18:04, Ilya Sherman wrote: > Is this still valid? Which sorts of items are selectable? Presumably at least > password items -- with id "-2" -- should be, given the if-stmt immediately > below. No, it was wrong. Selecting a non autofill entry should clear the autofill preview. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:330: ids.push_back(WebKit::SeparatorMenuItemID); On 2012/04/11 00:18:04, Ilya Sherman wrote: > This was not actually showing up as a separator in the mac build [1] you linked > to. Was that build compiled before you added this, or is there something going > wrong here? > [1] https://docs.google.com/open?id=0BwRi59l_ri74a1N4d2NSYXlIdzg The separator should come before the menu item. Fixed. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:343: ids.push_back(WebKit::SeparatorMenuItemID); On 2012/04/11 00:18:04, Ilya Sherman wrote: > This can add two separators where we should have only one: the "Clear form" and > "Autofill options" items should have only one separator above them (and no > separators between them). Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:362: const std::vector<int>& item_ids) { On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: 80-col, wrapping Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:371: if (options.length() > 0) { On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: Is this check needed, or would it also be covered by the "!option.isNull()" > check in the while loop? True! Removed. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:384: } On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: This code has a lot of levels of nesting. That could be reduced by > decomposing this to a helper function with early returns. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:386: if (v.size() > 0 && values.size() > 0) { On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: Please add a comment above this if-stmt -- something like "If there are > both <datalist> items and Autofill suggestions, add a separator between them." Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:473: if (!element.isEnabled() || element.isReadOnly() || !element.isTextField() || element.isPasswordField() || !element.suggestedValue().isEmpty()) On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: 80-col Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:499: // If the field has no name, then we won't have values. On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: Please move this comment to be above the if-stmt. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.cc:501: CombineDataListEntriesAndShow(element, std::vector<string16>(), std::vector<string16>(), std::vector<string16>(), std::vector<int>()); On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: 80-col. Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.h:144: const std::vector<int>& unique_ids); On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: item_ids? Done. http://codereview.chromium.org/10024059/diff/1/chrome/renderer/autofill/autof... chrome/renderer/autofill/autofill_agent.h:151: int item_id, On 2012/04/11 00:18:04, Ilya Sherman wrote: > nit: I would not rename this variable, as this should still be a /unique/ id. Done.
https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h" nit: This should come before the "ui/..." include https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:183: DCHECK_GE(item_id, 0); nit: DCHECK_GT https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:201: FillAutofillFormData(node, item_id, AUTOFILL_PREVIEW); nit: Up to you, but I would write lines 198-201 as if (item_id > 0) FillAutofillFormData(node, item_id, AUTOFILL_PREVIEW); https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:210: was_query_node_autofilled_); nit: No longer a need for line wrapping here? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:278: return; Do all of the early returns in this method apply equally well to <datalist> data? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:293: !element_.autoComplete()) { nit: No longer a need for line wrapping here? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:329: // form. Append the 'Clear form' menu item. nit: Please move this comment inside the if-stmt, since it only describes what happens in case the if-stmt condition succeeds. https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:346: element_.isFocusable()) { nit: No longer a need for line wrapping here? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:377: } nit: This should be moved into the anonymous namespace at the top of this file. https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:406: render_view()->GetWebView()->applyAutofillSuggestions(element, v, l, i, ids); Should we call this only if |v| is non-empty? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:472: element_, nit: No longer a need for line wrapping here? https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:509: // If the field has no name, then we won't have values. nit: I would append this to the end of line 507, as "Also, if the field..." https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.cc:549: DCHECK(unique_id > 0); nit: DCHECK_GT https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... File chrome/renderer/autofill/autofill_agent.h (right): https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.h:144: const std::vector<int>& item_ids); nit: Align with the opening paren https://chromiumcodereview.appspot.com/10024059/diff/6002/chrome/renderer/aut... chrome/renderer/autofill/autofill_agent.h:147: // |value| is the current text in the field, and |item_id| is the selected nit: unique_id
http://codereview.chromium.org/10024059/diff/6002/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10024059/diff/6002/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:16: #include "third_party/WebKit/Source/WebKit/chromium/public/WebAutofillClient.h" On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: This should come before the "ui/..." include Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:183: DCHECK_GE(item_id, 0); On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: DCHECK_GT Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:201: FillAutofillFormData(node, item_id, AUTOFILL_PREVIEW); On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: Up to you, but I would write lines 198-201 as > > if (item_id > 0) > FillAutofillFormData(node, item_id, AUTOFILL_PREVIEW); Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:210: was_query_node_autofilled_); On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: No longer a need for line wrapping here? Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:278: return; On 2012/04/11 19:09:58, Ilya Sherman wrote: > Do all of the early returns in this method apply equally well to <datalist> > data? You're right. I added similar checks to AutofillAgent::CombineDataListEntriesAndShow http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:293: !element_.autoComplete()) { On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: No longer a need for line wrapping here? Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:329: // form. Append the 'Clear form' menu item. On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: Please move this comment inside the if-stmt, since it only describes what > happens in case the if-stmt condition succeeds. Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:346: element_.isFocusable()) { On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: No longer a need for line wrapping here? Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:377: } On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: This should be moved into the anonymous namespace at the top of this file. Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:406: render_view()->GetWebView()->applyAutofillSuggestions(element, v, l, i, ids); On 2012/04/11 19:09:58, Ilya Sherman wrote: > Should we call this only if |v| is non-empty? Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:472: element_, On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: No longer a need for line wrapping here? Unwrapping "element_," makes it 82 chars. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:509: // If the field has no name, then we won't have values. On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: I would append this to the end of line 507, as "Also, if the field..." Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.cc:549: DCHECK(unique_id > 0); On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: DCHECK_GT Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... File chrome/renderer/autofill/autofill_agent.h (right): http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.h:144: const std::vector<int>& item_ids); On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: Align with the opening paren Done. http://codereview.chromium.org/10024059/diff/6002/chrome/renderer/autofill/au... chrome/renderer/autofill/autofill_agent.h:147: // |value| is the current text in the field, and |item_id| is the selected On 2012/04/11 19:09:58, Ilya Sherman wrote: > nit: unique_id Done.
https://chromiumcodereview.appspot.com/10024059/diff/12001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10024059/diff/12001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:299: return; nit: It looks like |web_view| is no longer used in this method; so let's remove the variable from here, even though we can save a bit of work in the rare case that it's somehow NULL. https://chromiumcodereview.appspot.com/10024059/diff/12001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:307: // If autofill is disabled and we had suggestions, show a warning instead. The DCHECK_GT previously guaranteed that we had suggestions if we reached this code. It's now possible that we don't have any suggestions, in which case we shouldn't show a warning. https://chromiumcodereview.appspot.com/10024059/diff/12001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:331: if (has_autofill_item && has_warning_item) This case should be unreachable -- autofill items and warnings are mutually exclusive. https://chromiumcodereview.appspot.com/10024059/diff/12001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:363: } I don't think you need to move this logic from where it was before. I'm pretty sure the warning is guaranteed to be the only item in the vector by the time we were checking for it; and it should be safe to just clear all the vectors rather than returning early.
http://codereview.chromium.org/10024059/diff/12001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/12001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:299: return; On 2012/04/12 23:38:10, Ilya Sherman wrote: > nit: It looks like |web_view| is no longer used in this method; so let's remove > the variable from here, even though we can save a bit of work in the rare case > that it's somehow NULL. Done. http://codereview.chromium.org/10024059/diff/12001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:307: // If autofill is disabled and we had suggestions, show a warning instead. On 2012/04/12 23:38:10, Ilya Sherman wrote: > The DCHECK_GT previously guaranteed that we had suggestions if we reached this > code. It's now possible that we don't have any suggestions, in which case we > shouldn't show a warning. Done. http://codereview.chromium.org/10024059/diff/12001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:331: if (has_autofill_item && has_warning_item) On 2012/04/12 23:38:10, Ilya Sherman wrote: > This case should be unreachable -- autofill items and warnings are mutually > exclusive. Done. http://codereview.chromium.org/10024059/diff/12001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:363: } On 2012/04/12 23:38:10, Ilya Sherman wrote: > I don't think you need to move this logic from where it was before. I'm pretty > sure the warning is guaranteed to be the only item in the vector by the time we > were checking for it; and it should be safe to just clear all the vectors rather > than returning early. Done.
http://codereview.chromium.org/10024059/diff/12004/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/12004/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:329: } If you keep the |has_warning_item| logic, these if-stmts should update the variable's value. http://codereview.chromium.org/10024059/diff/12004/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:332: if (!display_warning_if_disabled_ && has_warning_item) { Rather than checking |has_warning_item|, you can check (!v.empty() && ids[0] == WebAutofillClient::MenuItemIDWarningMessage). I would prefer that approach, as it is a little bit less fragile.
http://codereview.chromium.org/10024059/diff/12004/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/12004/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:332: if (!display_warning_if_disabled_ && has_warning_item) { On 2012/04/13 21:39:47, Ilya Sherman wrote: > Rather than checking |has_warning_item|, you can check (!v.empty() && ids[0] == > WebAutofillClient::MenuItemIDWarningMessage). I would prefer that approach, as > it is a little bit less fragile. Done.
By the way the WebKit patches have landed. http://trac.webkit.org/changeset/114223 http://trac.webkit.org/changeset/114108
LGTM with a last few nits. Thanks! :) http://codereview.chromium.org/10024059/diff/33001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/10024059/diff/33001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager_unittest.cc:915: int expected_unique_ids[] = {WebKit::WarningMessageMenuItemID}; nit: I think you need to update the namespace unwrapping here to include WebAutofillClient. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:65: continue; nit: Please add a blank line after this line, for easier visual scanning. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:311: } nit: It looks like you don't use |has_autofill_item| until further down in the function. If that's accurate, please restore this code's previous position, so that it doesn't show up as a diff, since nothing has functionally changed. In either case, please restore the loop index to be |i| rather than |j|. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:360: if (!element_.isNull() && element_.isFocusable()) nit: Can these checks be moved to the very top of the function, so that if they fail, we just return early? If so, you can also remove the element_.isNull() check at line 313 (of this current Patch Set, #7 I think). http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:390: ids.insert(ids.end(), item_ids.begin(), item_ids.end()); nit: Please add a brief comment for this block, something like "Append the Autofill suggestions."
Thanks for the quick reviews! http://codereview.chromium.org/10024059/diff/33001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_manager_unittest.cc (right): http://codereview.chromium.org/10024059/diff/33001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_manager_unittest.cc:915: int expected_unique_ids[] = {WebKit::WarningMessageMenuItemID}; On 2012/04/16 21:45:49, Ilya Sherman wrote: > nit: I think you need to update the namespace unwrapping here to include > WebAutofillClient. Done. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:65: continue; On 2012/04/16 21:45:49, Ilya Sherman wrote: > nit: Please add a blank line after this line, for easier visual scanning. Done. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:311: } On 2012/04/16 21:45:49, Ilya Sherman wrote: > nit: It looks like you don't use |has_autofill_item| until further down in the > function. If that's accurate, please restore this code's previous position, so > that it doesn't show up as a diff, since nothing has functionally changed. In > either case, please restore the loop index to be |i| rather than |j|. Done. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:360: if (!element_.isNull() && element_.isFocusable()) On 2012/04/16 21:45:49, Ilya Sherman wrote: > nit: Can these checks be moved to the very top of the function, so that if they > fail, we just return early? If so, you can also remove the element_.isNull() > check at line 313 (of this current Patch Set, #7 I think). Done. http://codereview.chromium.org/10024059/diff/33001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:390: ids.insert(ids.end(), item_ids.begin(), item_ids.end()); On 2012/04/16 21:45:49, Ilya Sherman wrote: > nit: Please add a brief comment for this block, something like "Append the > Autofill suggestions." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/10024059/36005
Change committed as 132561 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
