Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10443084:
Add Datalist Support to New Autofill UI (Closed)
|
Created:
8 years, 6 months ago by csharp Modified:
8 years, 6 months ago Reviewers:
Ilya Sherman CC:
chromium-reviews, dhollowa+watch_chromium.org, brettw-cc_chromium.org, Ilya Sherman, dyu1, darin-cc_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAdd Datalist Support to New Autofill UI
Show Datalist elements in the new Autofill UI, properly separated from the other data and fully selectable.
BUG=51644
TEST=The datalist elements can be selected like normal autofill elements and unit tests pass.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141287
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fix order of adding datalist items to keep warning code #
Total comments: 6
Patch Set 3 : #
Total comments: 27
Patch Set 4 : #
Total comments: 28
Patch Set 5 : Better Data List Tests #
Total comments: 8
Patch Set 6 : #Patch Set 7 : Fixing linux compile #Patch Set 8 : Merging #Messages
Total messages: 19 (0 generated)
https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_external_delegate.cc:40: unique_id == WebAutofillClient::MenuItemIDWarningMessage) Hmm, I would have expected this if-stmt to be updated as part of this CL. https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_external_delegate.cc:115: } else if (ids[0] == WebAutofillClient::MenuItemIDWarningMessage && ids[0] is no longer guaranteed to be the first Autofill item, so this if-stmt logic will need to be updated to accomodate for that. https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_external_delegate.cc:202: } Shouldn't this method also update the popup contents, if there is already a popup showing? https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/common/autofill... File chrome/common/autofill_messages.h (right): https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/common/autofill... chrome/common/autofill_messages.h:114: string16) nit: Please annotate this parameter with a /* description */ https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/common/autofill... chrome/common/autofill_messages.h:118: string16) nit: Please annotate this parameter with a /* description */
http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_external_delegate.cc:40: unique_id == WebAutofillClient::MenuItemIDWarningMessage) On 2012/05/30 21:55:43, Ilya Sherman wrote: > Hmm, I would have expected this if-stmt to be updated as part of this CL. If the datalist element was added to this list then if the user hovered over a profile entry before moving over a data list entry, the profile data would still be previewed. This current logic clears the preview from anything a suggestion that could fill in the input field is selected (which seems to be the cleanest way to me), but I could also remove this if statement and the password one so that the preview form is cleared as soon as you move off the item, no matter what you move onto. http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_external_delegate.cc:115: } else if (ids[0] == WebAutofillClient::MenuItemIDWarningMessage && On 2012/05/30 21:55:43, Ilya Sherman wrote: > ids[0] is no longer guaranteed to be the first Autofill item, so this if-stmt > logic will need to be updated to accomodate for that. Done. http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_external_delegate.cc:202: } On 2012/05/30 21:55:43, Ilya Sherman wrote: > Shouldn't this method also update the popup contents, if there is already a > popup showing? This method shouldn't ever get called when the popup is visible. The datalist values are set once the input field is selected (which is before before the popup appears) and they shouldn't change until we selected a new input field. I've add a DCHECK to ensure this holds. http://codereview.chromium.org/10443084/diff/1/chrome/common/autofill_messages.h File chrome/common/autofill_messages.h (right): http://codereview.chromium.org/10443084/diff/1/chrome/common/autofill_message... chrome/common/autofill_messages.h:114: string16) On 2012/05/30 21:55:43, Ilya Sherman wrote: > nit: Please annotate this parameter with a /* description */ Done. http://codereview.chromium.org/10443084/diff/1/chrome/common/autofill_message... chrome/common/autofill_messages.h:118: string16) On 2012/05/30 21:55:43, Ilya Sherman wrote: > nit: Please annotate this parameter with a /* description */ Done.
https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_external_delegate.cc:40: unique_id == WebAutofillClient::MenuItemIDWarningMessage) On 2012/05/31 14:20:26, csharp wrote: > On 2012/05/30 21:55:43, Ilya Sherman wrote: > > Hmm, I would have expected this if-stmt to be updated as part of this CL. > > If the datalist element was added to this list then if the user hovered over a > profile entry before moving over a data list entry, the profile data would still > be previewed. > > This current logic clears the preview from anything a suggestion that could fill > in the input field is selected (which seems to be the cleanest way to me), but I > could also remove this if statement and the password one so that the preview > form is cleared as soon as you move off the item, no matter what you move onto. Ah, I see. Yeah, I think the preview should vanish as soon as you're no longer hovering over the row that is being previewed, even if you hover over a separator or a menu item or whatever. https://chromiumcodereview.appspot.com/10443084/diff/1/chrome/browser/autofil... chrome/browser/autofill/autofill_external_delegate.cc:202: } On 2012/05/31 14:20:26, csharp wrote: > On 2012/05/30 21:55:43, Ilya Sherman wrote: > > Shouldn't this method also update the popup contents, if there is already a > > popup showing? > > This method shouldn't ever get called when the popup is visible. The datalist > values are set once the input field is selected (which is before before the > popup appears) and they shouldn't change until we selected a new input field. > > I've add a DCHECK to ensure this holds. Ok. The DCHECK is probably good enough for now; but eventually we will likely want to be able to update the <datalist> suggestions in response to JavaScript changes even while the popup is open. Maybe worth adding a TODO or filing a bug to tackle that case someday. https://chromiumcodereview.appspot.com/10443084/diff/5001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/5001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:76: // List any data list values at the start. nit: Remove this comment? https://chromiumcodereview.appspot.com/10443084/diff/5001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:151: // Insert the separator between the datalist and Autofill values. Only if there are non-datalist items? https://chromiumcodereview.appspot.com/10443084/diff/5001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.cc:181: } This method is getting pretty long... would be nice to break it up into some smaller constituent parts.
http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_external_delegate.cc:40: unique_id == WebAutofillClient::MenuItemIDWarningMessage) On 2012/06/01 06:28:16, Ilya Sherman wrote: > On 2012/05/31 14:20:26, csharp wrote: > > On 2012/05/30 21:55:43, Ilya Sherman wrote: > > > Hmm, I would have expected this if-stmt to be updated as part of this CL. > > > > If the datalist element was added to this list then if the user hovered over a > > profile entry before moving over a data list entry, the profile data would > still > > be previewed. > > > > This current logic clears the preview from anything a suggestion that could > fill > > in the input field is selected (which seems to be the cleanest way to me), but > I > > could also remove this if statement and the password one so that the preview > > form is cleared as soon as you move off the item, no matter what you move > onto. > > Ah, I see. Yeah, I think the preview should vanish as soon as you're no longer > hovering over the row that is being previewed, even if you hover over a > separator or a menu item or whatever. Done. http://codereview.chromium.org/10443084/diff/1/chrome/browser/autofill/autofi... chrome/browser/autofill/autofill_external_delegate.cc:202: } On 2012/06/01 06:28:16, Ilya Sherman wrote: > On 2012/05/31 14:20:26, csharp wrote: > > On 2012/05/30 21:55:43, Ilya Sherman wrote: > > > Shouldn't this method also update the popup contents, if there is already a > > > popup showing? > > > > This method shouldn't ever get called when the popup is visible. The datalist > > values are set once the input field is selected (which is before before the > > popup appears) and they shouldn't change until we selected a new input field. > > > > I've add a DCHECK to ensure this holds. > > Ok. The DCHECK is probably good enough for now; but eventually we will likely > want to be able to update the <datalist> suggestions in response to JavaScript > changes even while the popup is open. Maybe worth adding a TODO or filing a bug > to tackle that case someday. Had to replace the DCHECK with an if, since if you double clicked on an input element twice the DCHECK would fail (I had thought the first popup would get hidden before the second one appears). Add a bug. http://codereview.chromium.org/10443084/diff/5001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/5001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:76: // List any data list values at the start. On 2012/06/01 06:28:16, Ilya Sherman wrote: > nit: Remove this comment? Done. http://codereview.chromium.org/10443084/diff/5001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:151: // Insert the separator between the datalist and Autofill values. On 2012/06/01 06:28:16, Ilya Sherman wrote: > Only if there are non-datalist items? Done. http://codereview.chromium.org/10443084/diff/5001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:181: } On 2012/06/01 06:28:16, Ilya Sherman wrote: > This method is getting pretty long... would be nice to break it up into some > smaller constituent parts. Done.
http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (left): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:108: return; What happened to this early return? http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:88: ApplyAutofillWarnings(&v, &l, &i, &ids); This should happen before determining the value of |has_autofill_item|, since suggestions can be replaced by a warning. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:146: // crbug.com/131003 nit: Please prepend http:// to the link URL http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:266: l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED)); nit: I would probably move this to previous line, but it's also fine as-is if you prefer this formatting. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:270: } else if ((*autofill_unique_ids)[0] == What if the vector is empty? http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:271: WebAutofillClient::MenuItemIDWarningMessage && nit: This should probably be indented two more spaces http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:282: if ((*autofill_unique_ids)[0] == What if the vector is empty? http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:283: WebAutofillClient::MenuItemIDWarningMessage && nit: This should probably be indented 4 more spaces. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:320: if (!data_list_values_.empty()) { nit: Please write this as an early-return. This allows you to reduce the indentation level for the body of the implementation. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:323: if (autofill_values->size() != 0) { nit: if (!autofill_values.empty()) http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:150: // Handle applying any Autofill option listings to the Autofill popup. nit: This comment should mention that the method should only be called in the case that there is at least one multi-field suggestion in the list of suggestions. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:161: std::vector<int>* autofill_unique_ids); Do these methods all need access to the instantiated object's state? If not, please tuck them into the anonymous namespace in the implementation file. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:774: const std::vector<string16>& labels, nit: Indent by one space fewer.
ptal http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (left): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:108: return; On 2012/06/04 22:05:01, Ilya Sherman wrote: > What happened to this early return? Because we now have datalist elements that might appear in the popup we don't want to return here because then we fail to show them. The code in ApplyAutofillWarnings deletes the warning now if we added it and shouldn't show it. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:88: ApplyAutofillWarnings(&v, &l, &i, &ids); On 2012/06/04 22:05:01, Ilya Sherman wrote: > This should happen before determining the value of |has_autofill_item|, since > suggestions can be replaced by a warning. Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:146: // crbug.com/131003 On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: Please prepend http:// to the link URL Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:266: l10n_util::GetStringUTF16(IDS_AUTOFILL_WARNING_FORM_DISABLED)); On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: I would probably move this to previous line, but it's also fine as-is if > you prefer this formatting. Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:270: } else if ((*autofill_unique_ids)[0] == On 2012/06/04 22:05:01, Ilya Sherman wrote: > What if the vector is empty? Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:271: WebAutofillClient::MenuItemIDWarningMessage && On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: This should probably be indented two more spaces Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:282: if ((*autofill_unique_ids)[0] == On 2012/06/04 22:05:01, Ilya Sherman wrote: > What if the vector is empty? Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:283: WebAutofillClient::MenuItemIDWarningMessage && On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: This should probably be indented 4 more spaces. Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:320: if (!data_list_values_.empty()) { On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: Please write this as an early-return. This allows you to reduce the > indentation level for the body of the implementation. Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.cc:323: if (autofill_values->size() != 0) { On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: if (!autofill_values.empty()) Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_external_delegate.h (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:150: // Handle applying any Autofill option listings to the Autofill popup. On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: This comment should mention that the method should only be called in the > case that there is at least one multi-field suggestion in the list of > suggestions. Done. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_external_delegate.h:161: std::vector<int>* autofill_unique_ids); On 2012/06/04 22:05:01, Ilya Sherman wrote: > Do these methods all need access to the instantiated object's state? If not, > please tuck them into the anonymous namespace in the implementation file. They both need access to a few booleans in the class, it seems cleaner to me to keep them as member function, I have no problem adding the variables to the arguments and putting them in the anonymous namespace if you feel that it would be cleaner. http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... File chrome/browser/autofill/autofill_manager.cc (right): http://codereview.chromium.org/10443084/diff/7001/chrome/browser/autofill/aut... chrome/browser/autofill/autofill_manager.cc:774: const std::vector<string16>& labels, On 2012/06/04 22:05:01, Ilya Sherman wrote: > nit: Indent by one space fewer. Done.
https://chromiumcodereview.appspot.com/10443084/diff/7001/chrome/browser/auto... File chrome/browser/autofill/autofill_external_delegate.h (right): https://chromiumcodereview.appspot.com/10443084/diff/7001/chrome/browser/auto... chrome/browser/autofill/autofill_external_delegate.h:161: std::vector<int>* autofill_unique_ids); On 2012/06/06 14:06:52, csharp wrote: > On 2012/06/04 22:05:01, Ilya Sherman wrote: > > Do these methods all need access to the instantiated object's state? If not, > > please tuck them into the anonymous namespace in the implementation file. > > They both need access to a few booleans in the class, it seems cleaner to me to > keep them as member function, I have no problem adding the variables to the > arguments and putting them in the anonymous namespace if you feel that it would > be cleaner. Ok, sounds good. Since they access member booleans, leaving them as private methods is fine. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:63: const std::vector<int>& unique_ids) { nit: It's always bothered me that we use "v", "l", and "i" as names for the copied vectors in the body of this method. For one thing, we also use "i" as a loop variable. Perhaps we should name the parameters "autofill_values", "autofill_labels", etc., and use "values", "labels", etc. in the method body? It's fine if you don't want to make this change as part of this CL, but I think it would be nice cleanup to do at some point. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:110: if (!v.empty() && autofill_query_field_.is_focusable) { nit: v.empty() is checked on line 103, so no need to check it again here. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:117: has_shown_autofill_popup_for_current_edit_ |= has_autofill_item; Should these lines also be part of the if-stmt on lines 110-113? https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:271: WebAutofillClient::MenuItemIDWarningMessage) { nit: Looks like this still needs to be indented by one more space. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:281: if (autofill_unique_ids->size() > 0 && nit: !autofill_unique_ids->empty() https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:91: // Setup the expectation for a platform specific OnQuery call and then execute nit: "Setup" -> "Set up" https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:93: void SetupOnQuery(int query_id) { nit: Perhaps name this "IssueOnQuery()" or just "OnQuery()"? https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:101: OnQueryPlatformSpecific(query_id, form, field, bounds)); nit: I would move this back into the TestExternalDelegateVirtualCalls() test, since it's not likely to be relevant to other tests. (If you move this, please move out the "// This should call OnQueryPlatform specific." comment as well.) https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:118: const int kQueryId = 5; nit: Why not define this outside the test, so that it can be shared among the tests? https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:176: } There are a lot more branches in the code than this test covers. It would be nice to add more coverage. For example: verify that datalist items precede other items; verify that the popup is shown if it contains only datalist items; verify that separators are positioned correctly depending on whether there are datalist items and/or other suggestions; etc. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:609: // Find the datalist values and send them to the the browser process. nit: "the the" -> "the" https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:624: data_list_unique_ids)); To avoid the potential for DOS, we should limit the size of the vectors (and their contents) that we are willing to send across IPC
Something that I was just wondering about. The current code will always show data list elements right now, could that be a problem if a website want to do there own Autofill popup? Or can we safely ignore that case because if they wanted their own popup they wouldn't have the datalist elements? http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:63: const std::vector<int>& unique_ids) { On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: It's always bothered me that we use "v", "l", and "i" as names for the > copied vectors in the body of this method. For one thing, we also use "i" as a > loop variable. Perhaps we should name the parameters "autofill_values", > "autofill_labels", etc., and use "values", "labels", etc. in the method body? > > It's fine if you don't want to make this change as part of this CL, but I think > it would be nice cleanup to do at some point. Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:110: if (!v.empty() && autofill_query_field_.is_focusable) { On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: v.empty() is checked on line 103, so no need to check it again here. Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:117: has_shown_autofill_popup_for_current_edit_ |= has_autofill_item; On 2012/06/06 22:25:45, Ilya Sherman wrote: > Should these lines also be part of the if-stmt on lines 110-113? I'm not sure. The old autofill_agent logic didn't have them as part of the if, but that changed in http://codereview.chromium.org/10024059 (I looked through the comments but couldn't figure out why the change occurred). I feel like the lines should be in the if, but since affect the metrics sent out by Autofill I'm not too confident about changing it. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:271: WebAutofillClient::MenuItemIDWarningMessage) { On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: Looks like this still needs to be indented by one more space. Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:281: if (autofill_unique_ids->size() > 0 && On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: !autofill_unique_ids->empty() Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right): http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:91: // Setup the expectation for a platform specific OnQuery call and then execute On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: "Setup" -> "Set up" Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:93: void SetupOnQuery(int query_id) { On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: Perhaps name this "IssueOnQuery()" or just "OnQuery()"? Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:101: OnQueryPlatformSpecific(query_id, form, field, bounds)); On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: I would move this back into the TestExternalDelegateVirtualCalls() test, > since it's not likely to be relevant to other tests. (If you move this, please > move out the "// This should call OnQueryPlatform specific." comment as well.) I agree that its not too relevant to other tests, but I feel that moving it out of this function will just make it a bit more complex then it needs to be. The TestExternalDelegateVirtualCalls would need to be kept in sync with the code here and all the other tests that used this function would see warnings messages about OnQuery hitting the empty mock function. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:118: const int kQueryId = 5; On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: Why not define this outside the test, so that it can be shared among the > tests? Done. http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:176: } On 2012/06/06 22:25:45, Ilya Sherman wrote: > There are a lot more branches in the code than this test covers. It would be > nice to add more coverage. For example: verify that datalist items precede > other items; verify that the popup is shown if it contains only datalist items; > verify that separators are positioned correctly depending on whether there are > datalist items and/or other suggestions; etc. Updated some of the tests to ensure the placement of all elements are correct. (Good advice, I did catch a small bug) http://codereview.chromium.org/10443084/diff/16001/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10443084/diff/16001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:609: // Find the datalist values and send them to the the browser process. On 2012/06/06 22:25:45, Ilya Sherman wrote: > nit: "the the" -> "the" Done. http://codereview.chromium.org/10443084/diff/16001/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:624: data_list_unique_ids)); On 2012/06/06 22:25:45, Ilya Sherman wrote: > To avoid the potential for DOS, we should limit the size of the vectors (and > their contents) that we are willing to send across IPC Done, I set the limit to 30. I don't have any real basis for figuring out what a good value would be, so let me know if I made it too high or low.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2012/06/07 15:29:23, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a lowly provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. Ah, I'd mixed up this window with another and clicked the wrong commit queue box, sorry.
LGTM, thanks https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate.cc:117: has_shown_autofill_popup_for_current_edit_ |= has_autofill_item; On 2012/06/07 14:57:43, csharp wrote: > On 2012/06/06 22:25:45, Ilya Sherman wrote: > > Should these lines also be part of the if-stmt on lines 110-113? > > I'm not sure. The old autofill_agent logic didn't have them as part of the if, > but that changed in http://codereview.chromium.org/10024059 (I looked through > the comments but couldn't figure out why the change occurred). > > I feel like the lines should be in the if, but since affect the metrics sent out > by Autofill I'm not too confident about changing it. I think it was an oversight on my part previously that it was outside the if-stmt. We shouldn't report showing the popup when we didn't... My guess is that it's just rare enough for the element's focusability to change that we didn't notice. https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:101: OnQueryPlatformSpecific(query_id, form, field, bounds)); On 2012/06/07 14:57:43, csharp wrote: > On 2012/06/06 22:25:45, Ilya Sherman wrote: > > nit: I would move this back into the TestExternalDelegateVirtualCalls() test, > > since it's not likely to be relevant to other tests. (If you move this, > please > > move out the "// This should call OnQueryPlatform specific." comment as well.) > > I agree that its not too relevant to other tests, but I feel that moving it out > of this function will just make it a bit more complex then it needs to be. The > TestExternalDelegateVirtualCalls would need to be kept in sync with the code > here and all the other tests that used this function would see warnings messages > about OnQuery hitting the empty mock function. Ok, I'm convinced :) https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/16001/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:624: data_list_unique_ids)); On 2012/06/07 14:57:43, csharp wrote: > On 2012/06/06 22:25:45, Ilya Sherman wrote: > > To avoid the potential for DOS, we should limit the size of the vectors (and > > their contents) that we are willing to send across IPC > > > Done, I set the limit to 30. I don't have any real basis for figuring out what a > good value would be, so let me know if I made it too high or low. Thanks, 30 seems reasonable to me. We can always tweak it later if we need to :) https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/browser/aut... File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:30: // A constant value to use as the Autofill query Id. nit: "Id" -> either "id" or "ID" https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/browser/aut... chrome/browser/autofill/autofill_external_delegate_unittest.cc:33: // A constant vale to use as an Autofill profile Id. nit: "vale" -> "value" https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/renderer/au... File chrome/renderer/autofill/autofill_agent.cc (right): https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:629: } nit: You should probably also trim each entry of the vectors to be at most kMaximumTextSizeForAutofill characters long. https://chromiumcodereview.appspot.com/10443084/diff/19010/chrome/renderer/au... chrome/renderer/autofill/autofill_agent.cc:631: nit: Extra blank line
http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate.cc (right): http://codereview.chromium.org/10443084/diff/16001/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate.cc:117: has_shown_autofill_popup_for_current_edit_ |= has_autofill_item; On 2012/06/07 22:56:05, Ilya Sherman wrote: > On 2012/06/07 14:57:43, csharp wrote: > > On 2012/06/06 22:25:45, Ilya Sherman wrote: > > > Should these lines also be part of the if-stmt on lines 110-113? > > > > I'm not sure. The old autofill_agent logic didn't have them as part of the if, > > but that changed in http://codereview.chromium.org/10024059 (I looked through > > the comments but couldn't figure out why the change occurred). > > > > I feel like the lines should be in the if, but since affect the metrics sent > out > > by Autofill I'm not too confident about changing it. > > I think it was an oversight on my part previously that it was outside the > if-stmt. We shouldn't report showing the popup when we didn't... My guess is > that it's just rare enough for the element's focusability to change that we > didn't notice. Done. http://codereview.chromium.org/10443084/diff/19010/chrome/browser/autofill/au... File chrome/browser/autofill/autofill_external_delegate_unittest.cc (right): http://codereview.chromium.org/10443084/diff/19010/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:30: // A constant value to use as the Autofill query Id. On 2012/06/07 22:56:05, Ilya Sherman wrote: > nit: "Id" -> either "id" or "ID" Done. http://codereview.chromium.org/10443084/diff/19010/chrome/browser/autofill/au... chrome/browser/autofill/autofill_external_delegate_unittest.cc:33: // A constant vale to use as an Autofill profile Id. On 2012/06/07 22:56:05, Ilya Sherman wrote: > nit: "vale" -> "value" Done. http://codereview.chromium.org/10443084/diff/19010/chrome/renderer/autofill/a... File chrome/renderer/autofill/autofill_agent.cc (right): http://codereview.chromium.org/10443084/diff/19010/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:629: } On 2012/06/07 22:56:05, Ilya Sherman wrote: > nit: You should probably also trim each entry of the vectors to be at most > kMaximumTextSizeForAutofill characters long. Done. http://codereview.chromium.org/10443084/diff/19010/chrome/renderer/autofill/a... chrome/renderer/autofill/autofill_agent.cc:631: On 2012/06/07 22:56:05, Ilya Sherman wrote: > nit: Extra blank line Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10443084/20003
Try job failure for 10443084-20003 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10443084/16004
Failed to apply patch for chrome/browser/autofill/autofill_external_delegate.cc: While running patch -p1 --forward --force; patching file chrome/browser/autofill/autofill_external_delegate.cc Hunk #1 succeeded at 30 with fuzz 1. Hunk #3 FAILED at 87. Hunk #4 succeeded at 140 with fuzz 2. Hunk #5 FAILED at 183. Hunk #6 succeeded at 216 (offset -3 lines). Hunk #7 succeeded at 252 (offset -4 lines). 2 out of 7 hunks FAILED -- saving rejects to file chrome/browser/autofill/autofill_external_delegate.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/10443084/20007
Change committed as 141287 |
