Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1458903003:
[Autofill] Deduplicate Autocomplete and Datalist suggestions, keeping the latter (Closed)
|
Created:
5 years, 1 month ago by Mathieu Modified:
5 years, 1 month ago CC:
chromium-reviews, rouslan+autofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, bondd+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill] Deduplicate Autocomplete and Datalist suggestions, keeping the latter
Doesn't make sense to have both if they are the same value, as described in the bug.
BUG=419239
TEST=AutofillExternalDelegateTest
Committed: https://crrev.com/aedbb48c0b3e495622cf6698c88cc241be3b20b0
Cr-Commit-Position: refs/heads/master@{#360627}
Patch Set 1 : Initial #
Total comments: 10
Patch Set 2 : lambda function #
Total comments: 4
Patch Set 3 : addressed comments #
Messages
Total messages: 19 (8 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Autofill] Draft Autocomplete + Datalist BUG= ========== to ========== [Autofill] De-duplicate Autocomplete and Datalist suggestions, keeping the latter Doesn't make sense to have both if they are the same value, as described in the bug. BUG=419239 TEST=AutofillExternalDelegateTest ==========
mathp@chromium.org changed reviewers: + estade@chromium.org
Description was changed from ========== [Autofill] De-duplicate Autocomplete and Datalist suggestions, keeping the latter Doesn't make sense to have both if they are the same value, as described in the bug. BUG=419239 TEST=AutofillExternalDelegateTest ========== to ========== [Autofill] Deduplicate Autocomplete and Datalist suggestions, keeping the latter Doesn't make sense to have both if they are the same value, as described in the bug. BUG=419239 TEST=AutofillExternalDelegateTest ==========
Patchset #1 (id:20001) has been deleted
Hello, Please have a look!
https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:314: std::set<base::string16> datalist_set(data_list_values_.begin(), please keep datalist/data_list consistent https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { std::remove_if(suggestions->begin(), suggestions->end(), [](const Suggestion& suggestion){return data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == POPUP...;});
https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { On 2015/11/19 03:13:03, Evan Stade wrote: > std::remove_if(suggestions->begin(), > suggestions->end(), > [](const Suggestion& suggestion){return > data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == > POPUP...;}); note that you might have to put a & or something in the [] of the lambda to capture the data_list_set
vabr@chromium.org changed reviewers: + vabr@chromium.org
Hi Mathieu, I added some more comments to Evan's. With all of them addressed, this LGTM. Cheers, Vaclav https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { On 2015/11/19 03:19:51, Evan Stade wrote: > On 2015/11/19 03:13:03, Evan Stade wrote: > > std::remove_if(suggestions->begin(), > > suggestions->end(), > > [](const Suggestion& suggestion){return > > data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == > > POPUP...;}); > > note that you might have to put a & or something in the [] of the lambda to > capture the data_list_set Please use [&datalist_set], not just [&], because the latter is forbidden by the style. Also don't forget to call erase: suggestions.erase(std::remove_if(...), suggestions.end()); Other than that, big +1 to using remove_if, it is much more efficient. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:317: if (datalist_set.count(iter->value) && optional: While using .count() results in succinct code, it is not ideal, because it asks the program to compute more than needed. My suggestion here is ContainsKey from base/stl_util.h.
Thanks both, didn't know we could use lambda functions. Applied your comments, PTAL. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:314: std::set<base::string16> datalist_set(data_list_values_.begin(), On 2015/11/19 03:13:03, Evan Stade wrote: > please keep datalist/data_list consistent Done. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { On 2015/11/19 08:26:04, vabr (Chromium) wrote: > On 2015/11/19 03:19:51, Evan Stade wrote: > > On 2015/11/19 03:13:03, Evan Stade wrote: > > > std::remove_if(suggestions->begin(), > > > suggestions->end(), > > > [](const Suggestion& suggestion){return > > > data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == > > > POPUP...;}); > > > > note that you might have to put a & or something in the [] of the lambda to > > capture the data_list_set > > Please use [&datalist_set], not just [&], because the latter is forbidden by the > style. > > Also don't forget to call erase: > suggestions.erase(std::remove_if(...), suggestions.end()); > > Other than that, big +1 to using remove_if, it is much more efficient. Done. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { On 2015/11/19 03:13:03, Evan Stade wrote: > std::remove_if(suggestions->begin(), > suggestions->end(), > [](const Suggestion& suggestion){return > data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == > POPUP...;}); Done. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:316: for (auto iter = suggestions->begin(); iter != suggestions->end();) { On 2015/11/19 08:26:04, vabr (Chromium) wrote: > On 2015/11/19 03:19:51, Evan Stade wrote: > > On 2015/11/19 03:13:03, Evan Stade wrote: > > > std::remove_if(suggestions->begin(), > > > suggestions->end(), > > > [](const Suggestion& suggestion){return > > > data_list_set.count(suggestion.value) > 0 && suggestion.frontend_id == > > > POPUP...;}); > > > > note that you might have to put a & or something in the [] of the lambda to > > capture the data_list_set > > Please use [&datalist_set], not just [&], because the latter is forbidden by the > style. > > Also don't forget to call erase: > suggestions.erase(std::remove_if(...), suggestions.end()); > > Other than that, big +1 to using remove_if, it is much more efficient. Done. https://codereview.chromium.org/1458903003/diff/40001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:317: if (datalist_set.count(iter->value) && On 2015/11/19 08:26:04, vabr (Chromium) wrote: > optional: While using .count() results in succinct code, it is not ideal, > because it asks the program to compute more than needed. My suggestion here is > ContainsKey from base/stl_util.h. Ah thanks! Done.
LGTM, thanks! Because this does not look urgent, and Evan already started to review the patch, I suggest waiting whether he has any more concerns before landing, though. As for what parts of C++11 are allowed, http://chromium-cpp.appspot.com/ is your friend. :) The language features are mostly OK, and now we start to get some nice C++11 library functionality as well. The hottest news are allowing value moving. Cheers, Vaclav
lgtm https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:322: POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY; nit: i'd reverse the order of these checks since the second one is less expensive https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.h (right): https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.h:113: // duplicate autocomplete (not autofill) suggestions, keeping their datalist nit: Autofill is capitalized
Thanks both of you! https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:322: POPUP_ITEM_ID_AUTOCOMPLETE_ENTRY; On 2015/11/19 16:40:10, Evan Stade wrote: > nit: i'd reverse the order of these checks since the second one is less > expensive Done. https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.h (right): https://codereview.chromium.org/1458903003/diff/60001/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.h:113: // duplicate autocomplete (not autofill) suggestions, keeping their datalist On 2015/11/19 16:40:10, Evan Stade wrote: > nit: Autofill is capitalized Done.
The CQ bit was checked by mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1458903003/#ps80001 (title: "addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1458903003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1458903003/80001
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/aedbb48c0b3e495622cf6698c88cc241be3b20b0 Cr-Commit-Position: refs/heads/master@{#360627} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
