Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
962673004:
[Autofill/Autocomplete Feature] Substring matching instead of prefix matching. (Closed)
|
Created:
5 years, 9 months ago by Pritam Nikam Modified:
5 years, 5 months ago Reviewers:
please use gerrit instead, jungshik at Google, Evan Stade, danakj, vabr (Chromium), Ilya Sherman CC:
chromium-reviews, mlamouri+watch-content_chromium.org, browser-components-watch_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, rouslan+autofillwatch_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Autofill/Autocomplete Feature] Substring matching instead of prefix matching.
With present implementation autofill/autocomplete suggestions are matched for
prefix for what user has typed.
This patch brings in suggestions with substring matching (i.e. substring
matching to the beginning of suggestion tokens) instead of prefix matching.
This patch keeps the feature behind a command line switch
--enable-suggestions-with-substring-match.
BUG=77194
Committed: https://crrev.com/2dfdae10eacafadd4103018bde5b9481b3cec9d2
Cr-Commit-Position: refs/heads/master@{#338677}
Patch Set 1 #Patch Set 2 : Changes for password forms. #Patch Set 3 : Changes for autocomplete. #Patch Set 4 : Rebase. #Patch Set 5 : Moved changes under command line switch. #Patch Set 6 : Just a rebase. #Patch Set 7 : Amend histograms.xml. #Patch Set 8 : Added unittests. #
Total comments: 32
Patch Set 9 : Addresses Vaclav's & Evan's Inputs. #
Total comments: 40
Patch Set 10 : Addresses Vaclav's inputs. #
Total comments: 2
Patch Set 11 : Just a rebase. #Patch Set 12 : Removed suggestions ordering prefix/substring and unittests. #
Total comments: 30
Patch Set 13 : Addresses Vaclav's & Evan's Inputs. #Patch Set 14 : Few More inputs addressed. #Patch Set 15 : #
Total comments: 8
Patch Set 16 : Addresses Vaclav's inputs unittests. #
Total comments: 20
Patch Set 17 : Addresses review comments. #
Total comments: 5
Patch Set 18 : Addresses Evan's review comments. #
Total comments: 10
Patch Set 19 : Incorporated Vaclav's review comments. #
Total comments: 6
Patch Set 20 : Incorporates danakj's review comments. #Patch Set 21 : Just a rebase. #Patch Set 22 : Removed changes from "base/strings". #
Total comments: 8
Patch Set 23 : Just a code rebase. #Patch Set 24 : Incorporated Evan's review inputs. #
Total comments: 17
Patch Set 25 : Code rebase. #Patch Set 26 : Addresses Evan's review comments. #Patch Set 27 : #
Total comments: 10
Patch Set 28 : Incorporates Evan's review comments. #
Total comments: 4
Patch Set 29 : Code rebase. #Patch Set 30 : Addresses Evan's review comments. #Patch Set 31 : Ordering prefix matched followed by substring. #Patch Set 32 : Code rebase. #Patch Set 33 : Fixed ut breakages. #Patch Set 34 : Changes in BUILD.gn #
Total comments: 8
Patch Set 35 : Just a code rebase. #Patch Set 36 : Incorporated Evan's review. #
Total comments: 8
Patch Set 37 : Just a code rebase. #Patch Set 38 : Just code rebase. #
Total comments: 33
Patch Set 39 : Code rebase. #Patch Set 40 : Incorporated Rouslan's review comments. #Patch Set 41 : Added comments in autocomplete_history_manager.cc. #
Total comments: 59
Patch Set 42 : Code rebase. #Patch Set 43 : Incorporated Rouslan's review comments. #
Total comments: 47
Patch Set 44 : Incorporated Rouslan's review comments. #Patch Set 45 : Added handling for (_), (%) and (!) for LIKE clause. #Patch Set 46 : #
Total comments: 17
Patch Set 47 : Code rebase. #Patch Set 48 : Incorporated review comments. #Patch Set 49 : #Patch Set 50 : #
Total comments: 27
Patch Set 51 : Incorporated Rouslan's review comments. #
Total comments: 2
Patch Set 52 : #Patch Set 53 : code rebase. #Patch Set 54 : Modified to resolve bot breakages. #
Total comments: 23
Patch Set 55 : Code rebase. #Patch Set 56 : Incorporated Vaclav's review comments. #Patch Set 57 : Fixed unittest for Android bot. #
Total comments: 14
Patch Set 58 : Incorporated Vaclav's review comments. #Messages
Total messages: 124 (22 generated)
Patchset #4 (id:60001) has been deleted
Patchset #6 (id:120001) has been deleted
pritam.nikam@samsung.com changed reviewers: + estade@chromium.org, isherman@chromium.org, vabr@chromium.org
Hi Vaclav, Ilya & Evan, Please have a look. Thanks!
Hi Pritam Nikam, I explicitly only had a look at components/password_manager/*, and expect that you'll get review for the rest from Ilya or Evan. I left some comments, and while the idea looks good in general, I propose to wait until you get the non-password autofill part reviewed, because the password autofill part depends on it. Cheers, Vaclav https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/common/autofill_util.h:20: bool HasTokoneStartsWith(const base::string16& field_suggestion, Apart from the typo (Tokone), the name is still a bit cryptic -- it is not clear what a token means in this context. https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:62: // is on and |field_contents| matches the sub-string within |field_suggestion|; What does "the sub-string within" mean? This comment makes it not clear, what the matching criterion is. Is any substring match OK? and if you introduce "tokens", then please do not forget to explain what are token separators. https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:68: autofill::HasTokoneStartsWith(field_suggestion, field_contents); typo: Tokone https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:78: std::vector<autofill::Suggestion> substring_matched_suggestions; Why do you separate the substring matched suggestions from the rest, when you end up appending them anyway? Is it because of the order? https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:132: suggestions->push_back(suggestion); optional nit: What about using std::insert instead? https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:360: // on we return only sub-string matching username suggestions when the selected Just writing "only" makes it not clear which cases should not be matched. I propose splitting this test into at least two parts, one for cases which should be matched, and other for cases which should not.
Thanks, Pritam. I'll defer to Evan for the initial review passes.
tests? https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:241: ? HasTokoneStartsWith(username1, username2, true) spelling of this function https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:242: : StartsWith(username1, username2, true); Don't you need to change StartsWith? It seems like everywhere that calls it should be using the substring matching as appropriate. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:92: for (auto suggestion : suggestions) { please don't use auto for simple types like base::string16 https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:95: if (StartsWith(suggestion, user_input_, false)) { I feel like you should be using sql for this, as is done currently sqlite's LIKE is your friend https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:846: // Order |profile|s with pre-fix before sub-string match. prefix, not pre-fix substring, not sub-string https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:854: } else if (IsFeatureSubStringMatchEnabled() && Substring (not SubString) https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:856: substring_matched_profiles.push_back(profile); add a field to Suggestion to indicate if it's a prefix match, sort suggestions https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:867: suggestions.back().backend_id.variant = k + prefix_match; I do not understand this line at all
Thanks Vaclav & Evan for review inputs. I've addresses inputs along new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:241: ? HasTokoneStartsWith(username1, username2, true) On 2015/03/24 00:48:26, Evan Stade wrote: > spelling of this function Done. https://codereview.chromium.org/962673004/diff/180001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:242: : StartsWith(username1, username2, true); On 2015/03/24 00:48:26, Evan Stade wrote: > Don't you need to change StartsWith? It seems like everywhere that calls it > should be using the substring matching as appropriate. I my opinion, StartsWith() being used at other places as well, and even it shall work when this feature flag in turned off, keeping it as is. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:92: for (auto suggestion : suggestions) { On 2015/03/24 00:48:26, Evan Stade wrote: > please don't use auto for simple types like base::string16 Done. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:95: if (StartsWith(suggestion, user_input_, false)) { On 2015/03/24 00:48:26, Evan Stade wrote: > I feel like you should be using sql for this, as is done currently > > sqlite's LIKE is your friend In my opinion, doing it in SQL will be bit difficult as we are not performing just any substring matching, instead we have to match tokens within suggestions to be match to first token from user input. Moreover, I've tried using LIKE, but for me it didn't work (may be I'd goofed up in query statement) even to fetch prefixes. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:846: // Order |profile|s with pre-fix before sub-string match. On 2015/03/24 00:48:26, Evan Stade wrote: > prefix, not pre-fix > substring, not sub-string Done. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:854: } else if (IsFeatureSubStringMatchEnabled() && On 2015/03/24 00:48:26, Evan Stade wrote: > Substring (not SubString) Done. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:856: substring_matched_profiles.push_back(profile); On 2015/03/24 00:48:26, Evan Stade wrote: > add a field to Suggestion to indicate if it's a prefix match, sort suggestions Done. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:867: suggestions.back().backend_id.variant = k + prefix_match; On 2015/03/24 00:48:26, Evan Stade wrote: > I do not understand this line at all substring matched suggestion's |backend_id.variant| are assigned with next index in the suggestion list. https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/cor... components/autofill/core/common/autofill_util.h:20: bool HasTokoneStartsWith(const base::string16& field_suggestion, On 2015/03/23 14:17:33, vabr (Chromium) wrote: > Apart from the typo (Tokone), the name is still a bit cryptic -- it is not clear > what a token means in this context. Done. https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:62: // is on and |field_contents| matches the sub-string within |field_suggestion|; On 2015/03/23 14:17:33, vabr (Chromium) wrote: > What does "the sub-string within" mean? > This comment makes it not clear, what the matching criterion is. Is any > substring match OK? and if you introduce "tokens", then please do not forget to > explain what are token separators. Done. Rephrased to "Returns |true| if |kEnableSuggestionsWithSubstringMatch| command line switch is on and |field_contents| matches substring token within |field_suggestion| delimited by characters ' ', '.', ',', '-', '_' and '@'; otherwise |false|.". https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:68: autofill::HasTokoneStartsWith(field_suggestion, field_contents); On 2015/03/23 14:17:33, vabr (Chromium) wrote: > typo: Tokone Done. https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:78: std::vector<autofill::Suggestion> substring_matched_suggestions; On 2015/03/23 14:17:33, vabr (Chromium) wrote: > Why do you separate the substring matched suggestions from the rest, when you > end up appending them anyway? Is it because of the order? Yes. Just to keep prefix matched suggestions up in the order followed by substring matched suggestions. https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:132: suggestions->push_back(suggestion); On 2015/03/23 14:17:33, vabr (Chromium) wrote: > optional nit: What about using std::insert instead? With std::insert we have to pass position as well, moreover here we need to simply append to the end of the list. Currently, I'm planning to keep it as is. https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:360: // on we return only sub-string matching username suggestions when the selected On 2015/03/23 14:17:33, vabr (Chromium) wrote: > Just writing "only" makes it not clear which cases should not be matched. I > propose splitting this test into at least two parts, one for cases which should > be matched, and other for cases which should not. Done.
Hi Pritam Nikam, I left some more comments. I only looked at components/password_manager/* + components/autofill/content/renderer/password_autofill_agent.cc + the declaration of HasTokenStartsWith. Cheers, Vaclav https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:78: std::vector<autofill::Suggestion> substring_matched_suggestions; On 2015/03/24 11:39:37, Pritam Nikam wrote: > On 2015/03/23 14:17:33, vabr (Chromium) wrote: > > Why do you separate the substring matched suggestions from the rest, when you > > end up appending them anyway? Is it because of the order? > > Yes. Just to keep prefix matched suggestions up in the order followed by > substring matched suggestions. First of all -- this is a significant UI decision. Reading http://crbug.com/77194, I only see you mentioning in #16, but nobody else comments on whether this is the desired order. Please clarify that on the bug. If a consensus is reached that prefix matches should precede other token matches, then please document that with a code comment here to explain the reason for having |substring_matched_suggestions| separately. Also please do mention a link to the consensus comment (http://crbug.com/77194#cXY for a suitable XY). https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:132: suggestions->push_back(suggestion); On 2015/03/24 11:39:37, Pritam Nikam wrote: > On 2015/03/23 14:17:33, vabr (Chromium) wrote: > > optional nit: What about using std::insert instead? > > With std::insert we have to pass position as well, moreover here we need to > simply append to the end of the list. Currently, I'm planning to keep it as is. Having looked more deeply into this: There are plenty [1] of places in the codebase where insert is used for appending at the end. Technically, that's not std::insert, but the insert method of the vector, but that's what I meant. Calling Z.insert(...end(), X, Y) is therefore well understood as appending X...Y to Z. When you write this as a for-loop, one has to check if you did it because you do some further processing (like dereferencing) on the elements. So I drop the "optional" from my comment, please rewrite this with suggestions->insert(). [1] https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=i... https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:433: size_t start = 0, end = 0; nit: Although the style guide does not speak about this, the usual way to declare in Chromium is one line per variable: size_t start = 0; size_t end = 0; https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:779: static_cast<base::string16>(username_element.value()); You do not need the static_cast. WebString defines a base::string16 operator, so initialising current_username with a Webstring should compile. (Also on the line below.) https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:787: size_t start = 0, end = 0; As above, please declare the variables separately. https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1361: if (!IsFeatureSubstringMatchEnabled() && Why do we switch off inline autocomplete also for prefix matches if token-matching suggestions are enabled? https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:16: // Returns |true| if a substring token within |field_suggestion| begins with Not clear what a "substring token" is. What about rewriting the whole comment to: // Splits |field_suggestion| into tokens, separated by characters from " .,-_@". Returns true if |field_content| is a prefix of some token. https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:21: bool HasTokenStartsWith(const base::string16& field_suggestion, The name still sounds hard to understand. What about IsContentsPrefixOfSuggestionToken ? https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: bool case_sensitive = false); The style guide forbids default arguments in this case. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum... https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: bool case_sensitive = false); optional: By changing the bool to an enum with well described type name and values, you can make the callsites much easier to understand, and spare the last 2 sentences of the comment above. https://codereview.chromium.org/962673004/diff/200001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:62: // is on and |field_contents| matches substring token within |field_suggestion| Now the vague part is "matches" (no indication of prefix matching). I suggest giving up on repeating the comment for autofill::HasTokenStartsWith. The comment here does not have to explain what the 2-line function does (it is easy to read right below). Instead, you can comment on how this should be used: // Use this to check if a given suggestion matches the current contents of a field. I also suggest renaming to DoesSuggestionMatchContents (The name of the function will never explain fully how the match is done, and just saying that substrings are involved is not helpful, while making the name longer.) https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:88: autofill::Suggestion suggestion( I am a bit concerned about the code duplication. It was not ideal with 3 blocks of nearly-identical code, but now there are 6. Could you please separate the common part of creating the autofill::Suggestion into a separate function, and call it appropriately? https://codereview.chromium.org/962673004/diff/200001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:360: // on we return only username suggestions having substring token begins with the This sentence has broken grammar ("having token begins with"). Also, mentioning the switch in the comment is not necessary, if you comment about that below (see my comment there). What about: // Test that suggestion tokens (substrings separated by characters from // " .,-_@") are matched against field contents. No need to say "only", because you test the negative case in the new test below. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:362: TEST_F(PasswordAutofillManagerTest, ExtractSuggestionsWithSubstringMatchToken) { I propose removing "Substring" from the test name. It does not describe the matching criterion (it's prefix matching, not substring on the tokens, right?). Also, Match->Matching, and I don't fully understand Extract -- did you mean display? In total: DisplaySuggestionsWithMatchingTokens https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:363: base::CommandLine::ForCurrentProcess()->AppendSwitch( Instead of mentioning the flag in the test comment, you can just say here: // Token matching is currently behind a flag. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:387: base::string16 other_username2(base::ASCIIToUTF16("example@bar.com")); Do not include this username -- you should test the negative cases in different tests. Try to keep the test as focussed and small as possible. To test all the newly added codepaths, you need more tests anyway (here you only check negative filtering of other usernames). https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:394: // Simulate displaying suggestions with substring token begins with "foo". Sentence has broken grammar. What about: // Simulate displaying suggestions for field contents "foo", check that matching ones are displayed. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:406: // Test that when |kEnableSuggestionsWithSubstringMatch| command line switch is Similarly to above, consider shortening the comment to: // Test that suggestions are not matched when the field contents // spans multiple tokens (substrings separated by characters // from " .,-_@"). https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:410: NoSuggestionWithSubstringMatchButTokenDoesNot) { The name has broken grammar (Match is a noun, so "does not" cannot be applied). What about NoSuggestionForNonPrefixTokenMatch ? https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:411: base::CommandLine::ForCurrentProcess()->AppendSwitch( Instead of mentioning the flag in the test comment, you can just say here: // Token matching is currently behind a flag. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:442: // Simulate no suggestion popup appears with substring matching "oo". This is not really simulating that nothing appears. Please rephrase, e.g., as // Simulate displaying suggestions for field contents "oo". Check that none appear, because none has a token with a prefix "oo". https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:450: } // namespace password_manager Further suggestions for tests: (1) Test (and thereby document) what happens if the field contents contains the token separators, e.g., "user.typed". (2) If UI confirms that the prefix-matched suggestions should be displayed before the token-matched ones, add a test for that as well.
Thanks Vaclav for review comments. I've integrated most of your inputs along new patch set, please glance through, or we can wait till UX team confirms the order for suggestion list. Thanks! https://codereview.chromium.org/962673004/diff/180001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:78: std::vector<autofill::Suggestion> substring_matched_suggestions; On 2015/03/24 13:32:28, vabr (Chromium) wrote: > On 2015/03/24 11:39:37, Pritam Nikam wrote: > > On 2015/03/23 14:17:33, vabr (Chromium) wrote: > > > Why do you separate the substring matched suggestions from the rest, when > you > > > end up appending them anyway? Is it because of the order? > > > > Yes. Just to keep prefix matched suggestions up in the order followed by > > substring matched suggestions. > > First of all -- this is a significant UI decision. Reading > http://crbug.com/77194, I only see you mentioning in #16, but nobody else > comments on whether this is the desired order. Please clarify that on the bug. > > If a consensus is reached that prefix matches should precede other token > matches, then please document that with a code comment here to explain the > reason for having |substring_matched_suggestions| separately. Also please do > mention a link to the consensus comment (http://crbug.com/77194#cXY for a > suitable XY). Sure! I'll wait for UX approval from ainslie@ regarding ordering for suggestions. As you mentioned I'll document it in code providing the link to the consensus comment. https://codereview.chromium.org/962673004/diff/180001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:132: suggestions->push_back(suggestion); On 2015/03/24 13:32:28, vabr (Chromium) wrote: > On 2015/03/24 11:39:37, Pritam Nikam wrote: > > On 2015/03/23 14:17:33, vabr (Chromium) wrote: > > > optional nit: What about using std::insert instead? > > > > With std::insert we have to pass position as well, moreover here we need to > > simply append to the end of the list. Currently, I'm planning to keep it as > is. > > Having looked more deeply into this: > There are plenty [1] of places in the codebase where insert is used for > appending at the end. Technically, that's not std::insert, but the insert method > of the vector, but that's what I meant. > > Calling Z.insert(...end(), X, Y) is therefore well understood as appending X...Y > to Z. When you write this as a for-loop, one has to check if you did it because > you do some further processing (like dereferencing) on the elements. > > So I drop the "optional" from my comment, please rewrite this with > suggestions->insert(). > > [1] > https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=i... Done. Got rid of |substring_matched_suggestions| and instead populated all suggestions marking their match type - prefix match or substring match and std::sorted them to get desire order. https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:433: size_t start = 0, end = 0; On 2015/03/24 13:32:28, vabr (Chromium) wrote: > nit: Although the style guide does not speak about this, the usual way to > declare in Chromium is one line per variable: > size_t start = 0; > size_t end = 0; Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:779: static_cast<base::string16>(username_element.value()); On 2015/03/24 13:32:28, vabr (Chromium) wrote: > You do not need the static_cast. WebString defines a base::string16 operator, so > initialising current_username with a Webstring should compile. > > (Also on the line below.) Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:787: size_t start = 0, end = 0; On 2015/03/24 13:32:28, vabr (Chromium) wrote: > As above, please declare the variables separately. Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1361: if (!IsFeatureSubstringMatchEnabled() && On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Why do we switch off inline autocomplete also for prefix matches if > token-matching suggestions are enabled? Will address this input in next patch set. https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:16: // Returns |true| if a substring token within |field_suggestion| begins with On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Not clear what a "substring token" is. What about rewriting the whole comment > to: > > // Splits |field_suggestion| into tokens, separated by characters from " .,-_@". > Returns true if |field_content| is a prefix of some token. Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:21: bool HasTokenStartsWith(const base::string16& field_suggestion, On 2015/03/24 13:32:28, vabr (Chromium) wrote: > The name still sounds hard to understand. What about > IsContentsPrefixOfSuggestionToken > ? Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: bool case_sensitive = false); On 2015/03/24 13:32:28, vabr (Chromium) wrote: > The style guide forbids default arguments in this case. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argum... Done. https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: bool case_sensitive = false); On 2015/03/24 13:32:28, vabr (Chromium) wrote: > optional: By changing the bool to an enum with well described type name and > values, you can make the callsites much easier to understand, and spare the last > 2 sentences of the comment above. Done. Added enum instead. enum CaseSensitivity { CASE_SENSITIVE, // for case sensitive match; CASE_IGNORE // for case insensitive match; }; https://codereview.chromium.org/962673004/diff/200001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:62: // is on and |field_contents| matches substring token within |field_suggestion| On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Now the vague part is "matches" (no indication of prefix matching). > I suggest giving up on repeating the comment for autofill::HasTokenStartsWith. > The comment here does not have to explain what the 2-line function does (it is > easy to read right below). > Instead, you can comment on how this should be used: > // Use this to check if a given suggestion matches the current contents of a > field. > > I also suggest renaming to > DoesSuggestionMatchContents > (The name of the function will never explain fully how the match is done, and > just saying that substrings are involved is not helpful, while making the name > longer.) Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:88: autofill::Suggestion suggestion( On 2015/03/24 13:32:28, vabr (Chromium) wrote: > I am a bit concerned about the code duplication. It was not ideal with 3 blocks > of nearly-identical code, but now there are 6. Could you please separate the > common part of creating the autofill::Suggestion into a separate function, and > call it appropriately? Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:360: // on we return only username suggestions having substring token begins with the On 2015/03/24 13:32:29, vabr (Chromium) wrote: > This sentence has broken grammar ("having token begins with"). Also, mentioning > the switch in the comment is not necessary, if you comment about that below (see > my comment there). > What about: > > // Test that suggestion tokens (substrings separated by characters from > // " .,-_@") are matched against field contents. > > No need to say "only", because you test the negative case in the new test below. Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:362: TEST_F(PasswordAutofillManagerTest, ExtractSuggestionsWithSubstringMatchToken) { On 2015/03/24 13:32:29, vabr (Chromium) wrote: > I propose removing "Substring" from the test name. > It does not describe the matching criterion (it's prefix matching, not substring > on the tokens, right?). > Also, Match->Matching, and I don't fully understand Extract -- did you mean > display? > In total: > DisplaySuggestionsWithMatchingTokens Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:363: base::CommandLine::ForCurrentProcess()->AppendSwitch( On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Instead of mentioning the flag in the test comment, you can just say here: > // Token matching is currently behind a flag. Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:387: base::string16 other_username2(base::ASCIIToUTF16("example@bar.com")); On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Do not include this username -- you should test the negative cases in different > tests. Try to keep the test as focussed and small as possible. To test all the > newly added codepaths, you need more tests anyway (here you only check negative > filtering of other usernames). Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:394: // Simulate displaying suggestions with substring token begins with "foo". On 2015/03/24 13:32:29, vabr (Chromium) wrote: > Sentence has broken grammar. What about: > > // Simulate displaying suggestions for field contents "foo", check that matching > ones are displayed. Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:406: // Test that when |kEnableSuggestionsWithSubstringMatch| command line switch is On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Similarly to above, consider shortening the comment to: > > // Test that suggestions are not matched when the field contents > // spans multiple tokens (substrings separated by characters > // from " .,-_@"). Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:410: NoSuggestionWithSubstringMatchButTokenDoesNot) { On 2015/03/24 13:32:29, vabr (Chromium) wrote: > The name has broken grammar (Match is a noun, so "does not" cannot be applied). > What about > NoSuggestionForNonPrefixTokenMatch > ? Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:411: base::CommandLine::ForCurrentProcess()->AppendSwitch( On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Instead of mentioning the flag in the test comment, you can just say here: > // Token matching is currently behind a flag. Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:442: // Simulate no suggestion popup appears with substring matching "oo". On 2015/03/24 13:32:28, vabr (Chromium) wrote: > This is not really simulating that nothing appears. Please rephrase, e.g., as > // Simulate displaying suggestions for field contents "oo". Check that none > appear, because none has a token with a prefix "oo". Done. https://codereview.chromium.org/962673004/diff/200001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:450: } // namespace password_manager On 2015/03/24 13:32:28, vabr (Chromium) wrote: > Further suggestions for tests: > (1) Test (and thereby document) what happens if the field contents contains the > token separators, e.g., "user.typed". Done. > (2) If UI confirms that the prefix-matched suggestions should be displayed > before the token-matched ones, add a test for that as well. Will add this test after confirmation from UX.
On 2015/03/25 14:25:27, Pritam Nikam wrote: > Thanks Vaclav for review comments. > > I've integrated most of your inputs along new patch set, please glance through, > or we can wait till UX team confirms the order for suggestion list. > > Thanks! instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place and doing nothing special for prefix/non
Patchset #12 (id:260001) has been deleted
On 2015/03/25 21:28:17, Evan Stade wrote: > On 2015/03/25 14:25:27, Pritam Nikam wrote: > > Thanks Vaclav for review comments. > > > > I've integrated most of your inputs along new patch set, please glance > through, > > or we can wait till UX team confirms the order for suggestion list. > > > > Thanks! > > instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place > and doing nothing special for prefix/non Thanks Evan for your inputs. Hi all, I've added a new patch set removing prefix/substring ordering related changes, PTAL. Thanks!
estade@chromium.org changed reviewers: + jshin@chromium.org
+jshin --- question below https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:39: Tokenize(suggestion_string, base::ASCIIToUTF16(" .,-_@"), can we just check if the character right before offset is one of these splitting characters? https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:41: Tokenize(search_string, base::ASCIIToUTF16(" .,-_@"), &search_tokens); why tokenize the search term? https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:65: base::string16::size_type offset = suggestion.find(user_input); I wouldn't expect this to happen, but it seems like this could happen: switching some unicode character from upper to lower case changes the number of bytes it uses in UTF-16. Therefore |start| and |end| will be off. This stack overflow question seems to confirm this fear (at least for utf8): http://stackoverflow.com/questions/14792841/are-uppercase-utf8-characters-alw... +jshin who knows if this is a problem. If it is, it seems like base::StartsWith and base::EndsWith are also possibly broken. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:12: enum CaseSensitivity { use a bool imo. base::strings just uses bools for this. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:30: base::string16::size_type ComputeRange(const base::string16& field_suggestion, seems like this belongs next to base::StartsWith/EndsWith (base::Contains or something)
Hi Pritam Nikam, Thanks for the update! I added some more comments, mostly nits, but also some questions. Thanks, Vaclav https://codereview.chromium.org/962673004/diff/220001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/220001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1363: if (!IsFeatureSubstringMatchEnabled() && I still do not understand why do we switch off inline autocomplete also for prefix matches if token-matching suggestions are enabled. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:12: enum CaseSensitivity { On 2015/03/26 18:54:38, Evan Stade wrote: > use a bool imo. base::strings just uses bools for this. I actually suggested using enum here in https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor.... base::strings context might make it more clear that a true/false argument is about case sensitivity, than it is clear for IsContentsPrefixOfSuggestionToken. But Evan is the OWNER of this file, so I defer to his decision. https://codereview.chromium.org/962673004/diff/280001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:70: // Helper function to fill username suggestion and adds it to |suggestions| grammar: to fill ... and adds it ... Also, how does this function fill anything? Suggested correction of comment and function name: // If |field_suggestion| matches |field_content|, creates a Suggestion out of it and appends to |suggestions|. void AppendSuggestionIfMatching(...) https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:77: if (show_all || StartsWith(field_suggestion, field_contents, false) || What about moving the StartsWith check to DoesSuggestionMatchContents? That would nicely put all kinds of matching tests in one place. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:88: // suggestions where the username has |current_username| as a substring. Saying "substring" does not make the comment true either. What about just ...where the username matches |current_username|. It is vague in not saying what does "match" mean, but at least does not make false promises. https://codereview.chromium.org/962673004/diff/280001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:359: // Test that suggestion tokens (substrings separated by characters from " Note that clangformat discarded the space character in the token separators list. Please break manually, as I suggested in the previous comment. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:405: // Test that suggestions are not matched when the field contents spans multiple I'm sorry, when I was suggesting this comment, I got into an inconsistent state -- wanted to suggest a test for field contents spanning multiple tokens (which you added below), and commented in that spirit on this test. Please adjust to match the actual test. Something like // Test that suggestions which do not have a prefix match or prefix-token match with the field contents are not matched. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:447: // Test that suggestion tokens (substrings separated by characters from " Again: the space in the token separator list got discarded. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:448: // .,-_@") are matched against field contents containing tokens along the token I don't understand this sentence. What does "containing tokens along (...) separators" mean? Also, how is matching tested? Did you mean something like: // Test matching when field contents contains suggestion token separators. ? https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:451: DisplaySuggestionsWithMatchingTokensAndContentsHaveSeperator) { typo: Seperator Also, proposed simplification of the test name: MatchingContentsWithSuggestionTokenSeparator https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:483: // matching ones are displayed. nit: It is slightly confusing, that you use plural (matching ones) while there is only one matching suggestion. I would also appreciate a mention of the importance of token separators present. Ideally, could you explain why each of the usernames is or is not matched? I'm actually surprised that any of them match -- the presence of the token separator should prevent token matching, and foo@exam is also not a prefix of any of them.
Thanks Evan & Vaclav, Patch set #13 addresses your inputs, please have a look. Thanks! https://codereview.chromium.org/962673004/diff/220001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/220001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:1363: if (!IsFeatureSubstringMatchEnabled() && On 2015/03/27 10:13:34, vabr (Chromium) wrote: > I still do not understand why do we switch off inline autocomplete also for > prefix matches if token-matching suggestions are enabled. With autocomplete on, user experience for token matching seems problematic. IMO, it suits more for prefix matching. Moreover, with latest patch set I have restore it back to original code. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:39: Tokenize(suggestion_string, base::ASCIIToUTF16(" .,-_@"), On 2015/03/26 18:54:38, Evan Stade wrote: > can we just check if the character right before offset is one of these splitting > characters? Acknowledged. This seems much cleaner, I'll take care of this in next patch set (Use of base::ContainsOnlyChars). https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:41: Tokenize(search_string, base::ASCIIToUTF16(" .,-_@"), &search_tokens); On 2015/03/26 18:54:38, Evan Stade wrote: > why tokenize the search term? To handle cases where user input contains token separator as well. For e.g. Field's suggestion (super)list: { "foo.bar@example.com", "bar.foo@example.com", "example@foo.com" } User punches in: "foo@exam" Suggestions that expected to get populate: {"bar.foo@example.com"} But if I try to match suggestion tokens against user input (i.e. without tokenize the search string), none suggestion would display. IMO, if we get |search_string|'s first token matches any of the |suggestion_string|'s token, we are safe to assume thats the suggestion we are interested in. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:12: enum CaseSensitivity { On 2015/03/27 10:13:35, vabr (Chromium) wrote: > On 2015/03/26 18:54:38, Evan Stade wrote: > > use a bool imo. base::strings just uses bools for this. > > I actually suggested using enum here in > https://codereview.chromium.org/962673004/diff/200001/components/autofill/cor.... > base::strings context might make it more clear that a true/false argument is > about case sensitivity, than it is clear for IsContentsPrefixOfSuggestionToken. > > But Evan is the OWNER of this file, so I defer to his decision. In that case I'll restore it back to bool, and just remove the default argument that I've used in API prototype. https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:30: base::string16::size_type ComputeRange(const base::string16& field_suggestion, On 2015/03/26 18:54:38, Evan Stade wrote: > seems like this belongs next to base::StartsWith/EndsWith (base::Contains or > something) Sorry, I didn't get this input. Did you mean I shall move this to string_util.h/.cc instead? https://codereview.chromium.org/962673004/diff/280001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:70: // Helper function to fill username suggestion and adds it to |suggestions| On 2015/03/27 10:13:35, vabr (Chromium) wrote: > grammar: to fill ... and adds it ... > Also, how does this function fill anything? > Suggested correction of comment and function name: > // If |field_suggestion| matches |field_content|, creates a Suggestion out of it > and appends to |suggestions|. > void AppendSuggestionIfMatching(...) Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:77: if (show_all || StartsWith(field_suggestion, field_contents, false) || On 2015/03/27 10:13:35, vabr (Chromium) wrote: > What about moving the StartsWith check to DoesSuggestionMatchContents? That > would nicely put all kinds of matching tests in one place. Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:88: // suggestions where the username has |current_username| as a substring. On 2015/03/27 10:13:35, vabr (Chromium) wrote: > Saying "substring" does not make the comment true either. > What about just > ...where the username matches |current_username|. > It is vague in not saying what does "match" mean, but at least does not make > false promises. Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:359: // Test that suggestion tokens (substrings separated by characters from " On 2015/03/27 10:13:35, vabr (Chromium) wrote: > Note that clangformat discarded the space character in the token separators > list. Please break manually, as I suggested in the previous comment. Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:405: // Test that suggestions are not matched when the field contents spans multiple On 2015/03/27 10:13:35, vabr (Chromium) wrote: > I'm sorry, when I was suggesting this comment, I got into an inconsistent state > -- wanted to suggest a test for field contents spanning multiple tokens (which > you added below), and commented in that spirit on this test. > Please adjust to match the actual test. Something like > // Test that suggestions which do not have a prefix match or prefix-token match > with the field contents are not matched. Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:447: // Test that suggestion tokens (substrings separated by characters from " On 2015/03/27 10:13:35, vabr (Chromium) wrote: > Again: the space in the token separator list got discarded. Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:448: // .,-_@") are matched against field contents containing tokens along the token On 2015/03/27 10:13:35, vabr (Chromium) wrote: > I don't understand this sentence. What does "containing tokens along (...) > separators" mean? > Also, how is matching tested? Did you mean something like: > // Test matching when field contents contains suggestion token separators. > ? Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:451: DisplaySuggestionsWithMatchingTokensAndContentsHaveSeperator) { On 2015/03/27 10:13:35, vabr (Chromium) wrote: > typo: Seperator > Also, proposed simplification of the test name: > MatchingContentsWithSuggestionTokenSeparator Done. https://codereview.chromium.org/962673004/diff/280001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:483: // matching ones are displayed. On 2015/03/27 10:13:35, vabr (Chromium) wrote: > nit: It is slightly confusing, that you use plural (matching ones) while there > is only one matching suggestion. I would also appreciate a mention of the > importance of token separators present. Ideally, could you explain why each of > the usernames is or is not matched? I'm actually surprised that any of them > match -- the presence of the token separator should prevent token matching, and > foo@exam is also not a prefix of any of them. Done. I've rephrased it as below: // Simulate displaying suggestion for field contents "foo@exam", check that matching one is displayed. Please also note here that field contents contains token separators '@', |IsContentsPrefixOfSuggestionToken()| only picks suggestions that passes 2 constrains; firstly perform substring match, and secondly |Tokenize| field contents and suggestion and check whether the field contents's first token matches any of the suggestion's token. In our testcase, only |additional_username| passes both the constrains and apparently we could see only that in matching suggestion list.
Hi Pritam, I had a look in autofill_util.cc as well, because I sensed a misunderstanding about how the matching is meant to work. I left some comments concerning IsContentsPrefixOfSuggestionToken, please have a look. Cheers, Vaclav https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:29: bool IsContentsPrefixOfSuggestionToken(const base::string16& field_suggestion, What should IsContentsPrefixOfSuggestionToken("ab@cd.b", "b", false) return? My guess is |true|, because "b" is the last token of "ab@cd.b". But the code looks like it returns |false|, because the offset from line 41 will be 1, and "a" is not a separator. Am I missing something? https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... components/autofill/core/common/autofill_util.h:17: // Returns true if |field_content| is a prefix of some token; otherwise false. The comment seems different from what this function does. Consider IsContentsPrefixOfSuggestionToken("a.b.c", "b.c", true). According to the comment, this should return false, because "b.c" is not a prefix of a token (the tokens are "a", "b", "c"). But the function actually returns true, if I understood it correctly, because it is meant to start matching at the beginning of a token, and carry on across token separators. That's why I was confused in the tests. Please check and improve, if needed. https://codereview.chromium.org/962673004/diff/340001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/340001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:65: return StartsWith(field_suggestion, field_contents, false) || Looking at the definition of IsContentsPrefixOfSuggestionToken, isn't the StartsWith test actually redundant? What it does is covered by the case when offset == 0 on line 46 in components/autofill/core/common/autofill_util.cc, no?
Patchset #16 (id:360001) has been deleted
Thanks Vaclav once again. I've addresses your inputs with new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:29: bool IsContentsPrefixOfSuggestionToken(const base::string16& field_suggestion, On 2015/03/30 14:51:03, vabr (Chromium) wrote: > What should mailto:IsContentsPrefixOfSuggestionToken("ab@cd.b", "b", false) return? > My guess is |true|, because "b" is the last token of mailto:"ab@cd.b". But the code > looks like it returns |false|, because the offset from line 41 will be 1, and > "a" is not a separator. > Am I missing something? You are right. I did a blunder here. I want to keep it like below excerpt: 42 if (base::string16::npos != offset) { 43 std::vector<base::string16> suggestion_tokens, search_tokens; 44 Tokenize(suggestion_string, kTokenSeperatorsUTF16, &suggestion_tokens); 46 47 // Check whether the |search_string| prefixes any of the 49 // |suggestion_string|'s token. 50 if (suggestion_tokens.size() != 0) { 51 for (auto token : suggestion_tokens) { 52 result = StartsWith(token, search_string, case_sensitive); 53 if (result) 54 break; 55 } 55 } that way may be I'm align with what I'm intended to. https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... components/autofill/core/common/autofill_util.h:17: // Returns true if |field_content| is a prefix of some token; otherwise false. On 2015/03/30 14:51:03, vabr (Chromium) wrote: > The comment seems different from what this function does. > Consider IsContentsPrefixOfSuggestionToken("a.b.c", "b.c", true). > According to the comment, this should return false, because "b.c" is not a > prefix of a token (the tokens are "a", "b", "c"). > > But the function actually returns true, if I understood it correctly, because it > is meant to start matching at the beginning of a token, and carry on across > token separators. That's why I was confused in the tests. Please check and > improve, if needed. Reverted back the logic aligned with the comments + added unit tests for the same. https://codereview.chromium.org/962673004/diff/340001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/340001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:65: return StartsWith(field_suggestion, field_contents, false) || On 2015/03/30 14:51:04, vabr (Chromium) wrote: > Looking at the definition of IsContentsPrefixOfSuggestionToken, isn't the > StartsWith test actually redundant? What it does is covered by the case when > offset == 0 on line 46 in components/autofill/core/common/autofill_util.cc, no? IMO, substring matching is still behind "enable-suggestions-with-substring-match" command line switch, using StartsWith here seems apparent.
https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:30: base::string16::size_type ComputeRange(const base::string16& field_suggestion, On 2015/03/27 14:57:42, Pritam Nikam wrote: > On 2015/03/26 18:54:38, Evan Stade wrote: > > seems like this belongs next to base::StartsWith/EndsWith (base::Contains or > > something) > > Sorry, I didn't get this input. Did you mean I shall move this to > string_util.h/.cc instead? yes
Thanks, Pritam Nikam! The IsContentsPrefixOfSuggestionToken method is now much clearer (the name now matches exactly what it does). I still have some comments below, but we are getting there! :) Cheers, Vaclav https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/340001/components/autofill/cor... components/autofill/core/common/autofill_util.h:17: // Returns true if |field_content| is a prefix of some token; otherwise false. On 2015/03/30 17:35:48, Pritam Nikam wrote: > On 2015/03/30 14:51:03, vabr (Chromium) wrote: > > The comment seems different from what this function does. > > Consider IsContentsPrefixOfSuggestionToken("a.b.c", "b.c", true). > > According to the comment, this should return false, because "b.c" is not a > > prefix of a token (the tokens are "a", "b", "c"). > > > > But the function actually returns true, if I understood it correctly, because > it > > is meant to start matching at the beginning of a token, and carry on across > > token separators. That's why I was confused in the tests. Please check and > > improve, if needed. > > Reverted back the logic aligned with the comments + added unit tests for the > same. Acknowledged. https://codereview.chromium.org/962673004/diff/340001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/340001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:65: return StartsWith(field_suggestion, field_contents, false) || On 2015/03/30 17:35:48, Pritam Nikam wrote: > On 2015/03/30 14:51:04, vabr (Chromium) wrote: > > Looking at the definition of IsContentsPrefixOfSuggestionToken, isn't the > > StartsWith test actually redundant? What it does is covered by the case when > > offset == 0 on line 46 in components/autofill/core/common/autofill_util.cc, > no? > > IMO, substring matching is still behind > "enable-suggestions-with-substring-match" command line switch, using StartsWith > here seems apparent. Acknowledged. (Also now it is needed also behind the flag, because IsContentsPrefixOfSuggestionToken is now restricted to tokens only.) https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3241: // picks suggestions that passes the constrain that |field_contents| prefix grammar: passes -> pass (suggestions are plural) prefix -> prefixes (|field_contents| is a single string) https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3243: // passes both the constrains and apparently we could see only that in The comment confuses me. It says "both the constraints", and only one is mentioned above: "|field_contents| prefix some token in |field_suggestion|". The mentioned constraint is moreover not satisfied by "buddy@gma", as it spans multiple tokens. Please check the comment, and if possible, try to simplify it. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:20: base::string16 kTokenSeperatorsUTF16 = base::ASCIIToUTF16(" .,-_@"); I'm surprised none of the compilers complained about static variable with a non-trivial destructor. The guide complains, however -- http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... forbids static variables of class type. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:41: base::string16::size_type offset = suggestion_string.find(search_string); Why are you doing this pre-search? If it is just for performance, I'm not so sure -- converting to lowercase on lines 37-40 might also cost something. Keeping the code to minimum helps eliminate future bugs and incerases readability, so there is a value in getting rid of lines 37-42. If performance is your only reason to keep them, could you please do a benchmark to justify the need for them? https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:48: if (suggestion_tokens.size() != 0) { You do not really need this if. For size() == 0 the for loop below won't execute. Also -- next time you need to check if a collection is empty, use .empty(), not comparing .size() to 0 (which is less readable and generally less efficient). https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:15: base::ASCIIToUTF16("ab@cd.b"), base::ASCIIToUTF16("a"), false)); Please also test the same for the 2nd argument being "b" (that was the problematic case in the previous version). https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:17: base::ASCIIToUTF16("ab@cd.b"), base::ASCIIToUTF16("Ab"), false)); Please also add the same test call with the 3rd argument changed to |true|, so that the case sensitivity is tested. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:36: } TestCases[] = { TestCases is a bad naming style for a const variable. Use either kTestCases, or test_cases. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:47: size_t start = 0; Please use SCOPED_TRACE(testing::Message() << "suggestion = " << TestCases[i].field_suggestion << ", contents = " << TestCases[i].field_contents); to make it easy to understand the resulting failure messages. (SCOPED_TRACE automatically appends the above information to all outputs of EXPECT_* in the same scope.) https://codereview.chromium.org/962673004/diff/380001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:449: MatchingContentsWithSuggestionTokenSeparator) { Should there also be a test for situations like this? Suggestions include a@b.com, contents = "a@b.co". Then the a@b.com suggestion should match the contents by the whole-text prefix match, although not by the token-prefix match.
Patchset #17 (id:400001) has been deleted
Thanks Vaclav and Evan for review inputs. I've incorporated all inputs in new patch set, please have a look. Thanks! https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/cor... components/autofill/core/common/autofill_util.h:30: base::string16::size_type ComputeRange(const base::string16& field_suggestion, On 2015/03/30 17:44:58, Evan Stade wrote: > On 2015/03/27 14:57:42, Pritam Nikam wrote: > > On 2015/03/26 18:54:38, Evan Stade wrote: > > > seems like this belongs next to base::StartsWith/EndsWith (base::Contains or > > > something) > > > > Sorry, I didn't get this input. Did you mean I shall move this to > > string_util.h/.cc instead? > > yes Done. Added a utility API to string_util.h/cc to get the offset of search string. // Returns offset to the beginning of first occurence of |search| in the |str|. // If no such subsequence is found, |base::string16::npos| is returned. BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, const base::string16& search, bool case_sensitive); ComputeRange() wraps it for autocomplete/autofill need to calculate the selection range indices. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3241: // picks suggestions that passes the constrain that |field_contents| prefix On 2015/03/31 09:38:07, vabr (Chromium) wrote: > grammar: > passes -> pass (suggestions are plural) > prefix -> prefixes (|field_contents| is a single string) Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3243: // passes both the constrains and apparently we could see only that in On 2015/03/31 09:38:07, vabr (Chromium) wrote: > The comment confuses me. It says "both the constraints", and only one is > mentioned above: "|field_contents| prefix some token in |field_suggestion|". The > mentioned constraint is moreover not satisfied by "buddy@gma", as it spans > multiple tokens. Please check the comment, and if possible, try to simplify it. I've corrected this and chosen "gmail.co" as field contents. With "buddy@gma" as field contents and StartsWith() still in place "buddy@gma" will still brings up buddy@gmail.com" in suggestions. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:20: base::string16 kTokenSeperatorsUTF16 = base::ASCIIToUTF16(" .,-_@"); On 2015/03/31 09:38:07, vabr (Chromium) wrote: > I'm surprised none of the compilers complained about static variable with a > non-trivial destructor. > The guide complains, however -- > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Static_and_Gl... > forbids static variables of class type. Done. Removed it and used in-place substitution. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:41: base::string16::size_type offset = suggestion_string.find(search_string); On 2015/03/31 09:38:07, vabr (Chromium) wrote: > Why are you doing this pre-search? > If it is just for performance, I'm not so sure -- converting to lowercase on > lines 37-40 might also cost something. > > Keeping the code to minimum helps eliminate future bugs and incerases > readability, so there is a value in getting rid of lines 37-42. If performance > is your only reason to keep them, could you please do a benchmark to justify the > need for them? Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:48: if (suggestion_tokens.size() != 0) { On 2015/03/31 09:38:07, vabr (Chromium) wrote: > You do not really need this if. For size() == 0 the for loop below won't > execute. > Also -- next time you need to check if a collection is empty, use .empty(), not > comparing .size() to 0 (which is less readable and generally less efficient). Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:15: base::ASCIIToUTF16("ab@cd.b"), base::ASCIIToUTF16("a"), false)); On 2015/03/31 09:38:07, vabr (Chromium) wrote: > Please also test the same for the 2nd argument being "b" (that was the > problematic case in the previous version). Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:17: base::ASCIIToUTF16("ab@cd.b"), base::ASCIIToUTF16("Ab"), false)); On 2015/03/31 09:38:07, vabr (Chromium) wrote: > Please also add the same test call with the 3rd argument changed to |true|, so > that the case sensitivity is tested. Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:36: } TestCases[] = { On 2015/03/31 09:38:07, vabr (Chromium) wrote: > TestCases is a bad naming style for a const variable. > Use either kTestCases, or test_cases. Done. https://codereview.chromium.org/962673004/diff/380001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:47: size_t start = 0; On 2015/03/31 09:38:07, vabr (Chromium) wrote: > Please use > SCOPED_TRACE(testing::Message() << "suggestion = " << > TestCases[i].field_suggestion << ", contents = " << > TestCases[i].field_contents); > to make it easy to understand the resulting failure messages. > (SCOPED_TRACE automatically appends the above information to all outputs of > EXPECT_* in the same scope.) Done. https://codereview.chromium.org/962673004/diff/380001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/380001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:449: MatchingContentsWithSuggestionTokenSeparator) { On 2015/03/31 09:38:07, vabr (Chromium) wrote: > Should there also be a test for situations like this? > Suggestions include mailto:a@b.com, contents = mailto:"a@b.co". Then the mailto:a@b.com suggestion > should match the contents by the whole-text prefix match, although not by the > token-prefix match. Done.
https://codereview.chromium.org/962673004/diff/420001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/962673004/diff/420001/base/strings/string_uti... base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, I think you want to call this Find. You'll need a base/ owner to review this part of the change. https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:890: autofill::ComputeRange(field->suggestedValue(), field->value(), can you use StartsAt directly https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:37: result = StartsWith(token, field_contents, case_sensitive); this doesn't work if you type user@exa and the suggestion is user@example.com. I think you should just search for the first occurrence of field_contents in field_suggestion (without tokenization) then check if pos - 1 is a separator character (or -1). That would also not work if you typed "example" and the suggestion was "texample@example.com", but the only time that's likely to come up is if you typed "a" or something else really short/common, in which case it's arguable we shouldn't be suggesting anything. But if you do want to handle that case you need StartsAt to be able to return more results than just the first one.
https://codereview.chromium.org/962673004/diff/420001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/962673004/diff/420001/base/strings/string_uti... base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, I think you want to call this Find. You'll need a base/ owner to review this part of the change. https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:890: autofill::ComputeRange(field->suggestedValue(), field->value(), can you use StartsAt directly https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:37: result = StartsWith(token, field_contents, case_sensitive); this doesn't work if you type user@exa and the suggestion is user@example.com. I think you should just search for the first occurrence of field_contents in field_suggestion (without tokenization) then check if pos - 1 is a separator character (or -1). That would also not work if you typed "example" and the suggestion was "texample@example.com", but the only time that's likely to come up is if you typed "a" or something else really short/common, in which case it's arguable we shouldn't be suggesting anything. But if you do want to handle that case you need StartsAt to be able to return more results than just the first one.
pritam.nikam@samsung.com changed reviewers: + jam@chromium.org
+ jam@chromium.org: Please review changes in base/strings/string_util.cc base/strings/string_util.h base/strings/string_util_unittest.cc Thanks Evan for your inputs. With new patch set I've incorporated your review comments, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:890: autofill::ComputeRange(field->suggestedValue(), field->value(), On 2015/03/31 21:41:52, Evan Stade wrote: > can you use StartsAt directly StartsAt() is not equipped handle all complected cases. for eg. 1. field_suggestion = L"ba.a.ab" and field_contents = L"a.a" After executing ComputeRange() -> offset = 3, start = 6 & end = 7. 2. field_suggestion = L"texample@example.com" and field_contents = L"example" After executing ComputeRange() -> offset = 9, start = 16 & end = 20. IMO StartsAt() shall handle only basic case i.e. to return the first occurrence offset of search string. And ComputeRange() shall use it to get actual data (i.e. selection start and end indexes) that we are interested in. I've modified ComputeRange() to meet these these cases. https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/420001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:37: result = StartsWith(token, field_contents, case_sensitive); On 2015/03/31 21:41:52, Evan Stade wrote: > this doesn't work if you type user@exa and the suggestion is mailto:user@example.com. I > think you should just search for the first occurrence of field_contents in > field_suggestion (without tokenization) then check if pos - 1 is a separator > character (or -1). > > That would also not work if you typed "example" and the suggestion was > mailto:"texample@example.com", but the only time that's likely to come up is if you > typed "a" or something else really short/common, in which case it's arguable we > shouldn't be suggesting anything. But if you do want to handle that case you > need StartsAt to be able to return more results than just the first one. Done. Modified ComputeRange() so that it will take care of these cases. Also added few unit tests as well.
Hi Pritam Nikam, My remaining comments are nits. components/autofill/content/renderer/password_autofill_agent.cc, components/password_manager/core/browser/password_autofill_manager*, and the definition of IsContentsPrefixOfSuggestionToken LGTM. Thanks! Vaclav https://codereview.chromium.org/962673004/diff/440001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:16: EXPECT_FALSE(IsContentsPrefixOfSuggestionToken( nit: Please add a comment like: // IsContentsPrefixOfSuggestionToken should not work yet without a flag. https://codereview.chromium.org/962673004/diff/440001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:67: StartsWith(field_suggestion, field_contents, false); StartsWith is much simpler (and therefore faster) than IsContentsPrefixOfSuggestionToken. It is reasonable to expect that the contents will match the prefix of a suggestion often. So swapping the order of the disjunction to StartsWith(...) || IsContentsPrefixOfSuggestionToken(...) looks like a performance win. What do you think? https://codereview.chromium.org/962673004/diff/440001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:480: // Simulate displaying suggestions for field contents "foo@exam", check that optional nit: I found the comment difficult to read. Below I suggest a simplification, but I'm not forcing that on you. // Simulate displaying suggestions for field contents "foo@exam". Because "@" is a token separator, the field contents cannot be a prefix of any suggestion token. Moreover, no suggestion starts with "foo@exam". Therefore, no suggestions should match. https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:484: // constrain that |field_contents| prefixes some token in |field_suggestion|. grammar: constraint (note the "t" at the end, without the "t" it is a verb, with the "t" it is a noun) This occurs on multiple places. https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:492: // Simulate displaying suggestions for field contents "foo.bar@ex", check that optional nit: I found the comment difficult to read. Below I suggest a simplification, but I'm not forcing that on you. // Simulate displaying suggestions for field contents "foo.bar@ex". Because "@" is a token separator, the field contents cannot be a prefix of any suggestion token. However, the suggestion "foo.bar@example.com" starts with "foo.bar@ex". That suggestion should be the only match.
Thanks Vaclav once again for your inputs. I've incorporated these along new patch set, please have a look. Thanks! https://codereview.chromium.org/962673004/diff/440001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:16: EXPECT_FALSE(IsContentsPrefixOfSuggestionToken( On 2015/04/01 11:33:00, vabr (Chromium) wrote: > nit: Please add a comment like: > // IsContentsPrefixOfSuggestionToken should not work yet without a flag. Done. https://codereview.chromium.org/962673004/diff/440001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:67: StartsWith(field_suggestion, field_contents, false); On 2015/04/01 11:33:00, vabr (Chromium) wrote: > StartsWith is much simpler (and therefore faster) than > IsContentsPrefixOfSuggestionToken. It is reasonable to expect that the contents > will match the prefix of a suggestion often. So swapping the order of the > disjunction to > StartsWith(...) || IsContentsPrefixOfSuggestionToken(...) > looks like a performance win. What do you think? Done. Yeah, point taken! https://codereview.chromium.org/962673004/diff/440001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:480: // Simulate displaying suggestions for field contents "foo@exam", check that On 2015/04/01 11:33:00, vabr (Chromium) wrote: > optional nit: > I found the comment difficult to read. Below I suggest a simplification, but I'm > not forcing that on you. > > // Simulate displaying suggestions for field contents "foo@exam". Because "@" is > a token separator, the field contents cannot be a prefix of any suggestion > token. Moreover, no suggestion starts with "foo@exam". Therefore, no suggestions > should match. Done. https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:484: // constrain that |field_contents| prefixes some token in |field_suggestion|. On 2015/04/01 11:33:00, vabr (Chromium) wrote: > grammar: constraint (note the "t" at the end, without the "t" it is a verb, with > the "t" it is a noun) > This occurs on multiple places. Done. https://codereview.chromium.org/962673004/diff/440001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:492: // Simulate displaying suggestions for field contents "foo.bar@ex", check that On 2015/04/01 11:33:00, vabr (Chromium) wrote: > optional nit: > I found the comment difficult to read. Below I suggest a simplification, but I'm > not forcing that on you. > > // Simulate displaying suggestions for field contents "foo.bar@ex". Because "@" > is a token separator, the field contents cannot be a prefix of any suggestion > token. However, the suggestion mailto:"foo.bar@example.com" starts with "foo.bar@ex". > That suggestion should be the only match. Done.
Thanks Pritam Nikam, I have no more comments. components/autofill/content/renderer/password_autofill_agent.cc, components/password_manager/core/browser/password_autofill_manager*, and the definition of IsContentsPrefixOfSuggestionToken still LGTM. Thanks! Vaclav
On 2015/04/01 09:10:37, Pritam Nikam wrote: > + mailto:jam@chromium.org: Please review changes in > base/strings/string_util.cc > base/strings/string_util.h > base/strings/string_util_unittest.cc please pick a reviewer from base/OWNERS
pritam.nikam@samsung.com changed reviewers: + danakj@chromium.org - jam@chromium.org
On 2015/04/01 18:19:39, jam wrote: > On 2015/04/01 09:10:37, Pritam Nikam wrote: > > + mailto:jam@chromium.org: Please review changes in > > base/strings/string_util.cc > > base/strings/string_util.h > > base/strings/string_util_unittest.cc > > please pick a reviewer from base/OWNERS Thanks Jam! + danakj@chromium.org Please review changes in base/strings/string_util.cc base/strings/string_util.h base/strings/string_util_unittest.cc Thanks!
https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... File base/strings/string_util.cc (right): https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... base/strings/string_util.cc:1021: return (it != str.end()) ? std::distance(str.begin(), it) : string16::npos; does (it - str.begin()) not work instead of std::distance? distance() can be O(n) where as operator- should always be cheap, so if it can work with operator- that'd be nice to be clear about the cost. https://codereview.chromium.org/962673004/diff/460001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, why size_type and not size_t like everything else in this file? https://codereview.chromium.org/962673004/diff/460001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/460001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:56: offset = StartsAt(suggestion, field_contents, false); Why not just std::search() here? Is this a common operation? I see no other callers of it in this CL.
Thanks danakj for inputs. I've incorporated these in new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... File base/strings/string_util.cc (right): https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... base/strings/string_util.cc:1021: return (it != str.end()) ? std::distance(str.begin(), it) : string16::npos; On 2015/04/02 18:31:42, danakj wrote: > does (it - str.begin()) not work instead of std::distance? distance() can be > O(n) where as operator- should always be cheap, so if it can work with operator- > that'd be nice to be clear about the cost. Done. https://codereview.chromium.org/962673004/diff/460001/base/strings/string_util.h File base/strings/string_util.h (right): https://codereview.chromium.org/962673004/diff/460001/base/strings/string_uti... base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, On 2015/04/02 18:31:42, danakj wrote: > why size_type and not size_t like everything else in this file? Done. IMO, using size_type keeps it consistent for container data types. For std::[w]string (here base::string16), std::[w]string::size_type is equal to std::allocator<T>::size_type. But I guess its a typedef of std::size_t anyways, so I'm restoring it to std::size_t. https://codereview.chromium.org/962673004/diff/460001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/460001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:56: offset = StartsAt(suggestion, field_contents, false); On 2015/04/02 18:31:43, danakj wrote: > Why not just std::search() here? Is this a common operation? I see no other > callers of it in this CL. Earlier, plan was to use StartsAt at all places where ComputeRange currently added. But calculating selection range may need to consider delimiter characters as well. Just like StartsWith/EndsWith utility APIs, I thought of keeping basic string utility that shall return the beginning of search string i.e. StartAt (with case-sensitivity) next to these APIs in string_util.h/cc, and add a wrapper utility API very specific to this feature under autofill_util.cc. That is why you could find only single caller of StartsAt, and through out this CL you could find ComputeRange being called. I'm kind of confuse what to keep and what to shred. Please revert with your inputs.
Hi all, I've removed changes from "base/strings/" in latest patch set, PTAL. Thanks!
Hi all, PTAL, Thanks!
danakj@chromium.org changed reviewers: - danakj@chromium.org
On 2015/04/22 12:09:28, Pritam Nikam wrote: > Hi all, > > PTAL, Thanks! Hi Pritam Nikam, Do you really need a review from everybody? For example I did not notice significant changes in the code I reviewed, so: components/autofill/content/renderer/password_autofill_agent.cc, components/password_manager/core/browser/password_autofill_manager*, and the definition of IsContentsPrefixOfSuggestionToken still LGTM, as they did in #31. Cheers, Vaclav
On 2015/04/23 08:36:55, vabr (Chromium) wrote: > On 2015/04/22 12:09:28, Pritam Nikam wrote: > > Hi all, > > > > PTAL, Thanks! > > Hi Pritam Nikam, > > Do you really need a review from everybody? For example I did not notice > significant changes in the code I reviewed, so: > components/autofill/content/renderer/password_autofill_agent.cc, > components/password_manager/core/browser/password_autofill_manager*, and the > definition of IsContentsPrefixOfSuggestionToken still LGTM, as they did in #31. > > Cheers, > Vaclav Thanks Vaclav once again. I meant danakj and Evan specially to have a look. Patch set is aligned with danakj's input to revert changes from base/string. Thanks!
On 2015/04/23 09:02:58, Pritam Nikam wrote: > On 2015/04/23 08:36:55, vabr (Chromium) wrote: > > On 2015/04/22 12:09:28, Pritam Nikam wrote: > > > Hi all, > > > > > > PTAL, Thanks! > > > > Hi Pritam Nikam, > > > > Do you really need a review from everybody? For example I did not notice > > significant changes in the code I reviewed, so: > > components/autofill/content/renderer/password_autofill_agent.cc, > > components/password_manager/core/browser/password_autofill_manager*, and the > > definition of IsContentsPrefixOfSuggestionToken still LGTM, as they did in > #31. > > > > Cheers, > > Vaclav > > Thanks Vaclav once again. > I meant danakj and Evan specially to have a look. Patch set is aligned with > danakj's input to revert changes from base/string. > > Thanks! This LGTM but am not an owner in these parts :)
https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:49: for (auto token : suggestion_tokens) { nit: don't use auto for simple types (like const base::string16&) https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:64: do { please write this loop in a more orthodox way. 1. |offset| is used for multiple purposes (some times relative to the beginning of the string, sometimes relative a substring) 2. you had to write the escape condition (base::string16::npos != offset) twice 3. |next_offset| is not a helpful variable name 4. |end| is always field_suggestion.size(), so why is it an outparam? 5. |start| also seems redundant with the return value and input params. https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.h:19: bool IsContentsPrefixOfSuggestionToken(const base::string16& field_suggestion, nit: s/field_suggestion/suggestion https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.h:25: // returns |base::string16::npos|. can you work on making this comment clearer? What is the "selection range"?
Thanks Evan and danakj. I've addressed Evan's review inputs in new patch set, ptal. Thanks! https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:49: for (auto token : suggestion_tokens) { On 2015/04/23 20:21:18, Evan Stade wrote: > nit: don't use auto for simple types (like const base::string16&) Done. https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:64: do { On 2015/04/23 20:21:18, Evan Stade wrote: > please write this loop in a more orthodox way. > > 1. |offset| is used for multiple purposes (some times relative to the beginning > of the string, sometimes relative a substring) > 2. you had to write the escape condition (base::string16::npos != offset) twice > 3. |next_offset| is not a helpful variable name > 4. |end| is always field_suggestion.size(), so why is it an outparam? > 5. |start| also seems redundant with the return value and input params. Done. Change this API to size_t GetTextSelectionStart(const base::string16& suggestion, const base::string16& field_contents); https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.h:19: bool IsContentsPrefixOfSuggestionToken(const base::string16& field_suggestion, On 2015/04/23 20:21:18, Evan Stade wrote: > nit: s/field_suggestion/suggestion Done. https://codereview.chromium.org/962673004/diff/520001/components/autofill/cor... components/autofill/core/common/autofill_util.h:25: // returns |base::string16::npos|. On 2015/04/23 20:21:18, Evan Stade wrote: > can you work on making this comment clearer? What is the "selection range"? Done.
Thanks Evan and danakj. I've addressed Evan's review inputs in new patch set, ptal. Thanks!
https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:730: if (start != base::string16::npos) { no curlies https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:877: if (start != base::string16::npos) { no curlies https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.h (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.h:87: base::string16 user_input_; this should be called pending_query_prefix_ and should go near the other query vars https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:34: } no curlies https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:63: if ((it == suggestion.begin()) || offset == 0 https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:69: ++offset; what is the purpose of this? https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:72: return (it != suggestion.end()) ? offset : base::string16::npos; is it valid if it == suggestion.end()? It seems like this should not happen.
this patch also seems to get case sensitivity wrong. For example, if I have an autofill address with the name "Jill Smith", it doesn't suggest it based on "s", but does suggest based on "S". Also, it doesn't work on credit card stuff (I don't know why we'd want to substring match on address names but not cardholder names).
Thanks Evan for review inputs. I've addressed your inputs in new patch. Credit card name suggestion I had missed earlier, added it here along some unit tests. Also case-insensitive profile suggestion match also corrected. PTAL! Thanks! https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:730: if (start != base::string16::npos) { On 2015/04/27 19:25:21, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:877: if (start != base::string16::npos) { On 2015/04/27 19:25:21, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.h (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.h:87: base::string16 user_input_; On 2015/04/27 19:25:21, Evan Stade wrote: > this should be called pending_query_prefix_ and should go near the other query > vars Done. https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:34: } On 2015/04/27 19:25:21, Evan Stade wrote: > no curlies Done. https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:63: if ((it == suggestion.begin()) || On 2015/04/27 19:25:21, Evan Stade wrote: > offset == 0 Done. https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:69: ++offset; On 2015/04/27 19:25:21, Evan Stade wrote: > what is the purpose of this? It will advance the suggestion iterator (or first1 in the range) in search() so that we could find next occurrence of pattern (contents prefix) in the text (suggestion). ForwardIterator1 search (ForwardIterator1 first1, ForwardIterator1 last1, ForwardIterator2 first2, ForwardIterator2 last2, BinaryPredicate pred); e.g. Let suggestion text: "texample@example.com", and user input: "example", after first search(), will return iterator & offset will be at "1". To make sure, we'll get next occurrence we've increment our search text iterator by one i.e. ++offset, this will advance suggestion iterator first1 to "xample@example.com" so that the next search() will return iterator at offset as "9". https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:72: return (it != suggestion.end()) ? offset : base::string16::npos; On 2015/04/27 19:25:21, Evan Stade wrote: > is it valid if it == suggestion.end()? It seems like this should not happen. it == suggestion.end() is equivalent to std::search() unable to find the field_contents in suggestion text.
Hi Evan, PTAL. Thanks!
having patched this into my local machine and tried it out, I think my previous comment was wrong: "instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place and doing nothing special for prefix/non" https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:72: return (it != suggestion.end()) ? offset : base::string16::npos; On 2015/04/28 14:45:55, Pritam Nikam wrote: > On 2015/04/27 19:25:21, Evan Stade wrote: > > is it valid if it == suggestion.end()? It seems like this should not happen. > > it == suggestion.end() is equivalent to std::search() unable to find the > field_contents in suggestion text. in which case you probably shouldn't have called GetTextSelectionStart on these inputs then, right? https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... chrome/app/generated_resources.grd:6387: + Enable showing of autofill, autocomplete and password manager suggestions for substring matching. "Substring matching for Autofill suggestions." https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... chrome/app/generated_resources.grd:6390: + Enable showing of autofill, autocomplete and password manager suggestions for substring matching. This allows only match against the beginning of each token like how omnibar autocomplete does. It's the omnibox, not omnibar. This description should take the form "Do x y z", i.e. "Match Autofill suggestions based on substrings (token prefixes) rather than just prefixes." https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:42: result = StartsWith(token, field_contents, case_sensitive); for(...) { if (StartsWith...) return true; } return false; https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:56: while ((it = std::search( this should be a for loop. Why is |it| declared outside this loop? https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:62: if ((offset == 0) || no extra parens
Thanks Evan for review. I'll address these on monday. Thanks! On 2015/05/11 19:07:12, Evan Stade wrote: > having patched this into my local machine and tried it out, I think my previous > comment was wrong: > > "instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place > and doing nothing special for prefix/non" > > https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... > File components/autofill/core/common/autofill_util.cc (right): > > https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... > components/autofill/core/common/autofill_util.cc:72: return (it != > suggestion.end()) ? offset : base::string16::npos; > On 2015/04/28 14:45:55, Pritam Nikam wrote: > > On 2015/04/27 19:25:21, Evan Stade wrote: > > > is it valid if it == suggestion.end()? It seems like this should not happen. > > > > it == suggestion.end() is equivalent to std::search() unable to find the > > field_contents in suggestion text. > > in which case you probably shouldn't have called GetTextSelectionStart on these > inputs then, right? > > https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... > File chrome/app/generated_resources.grd (right): > > https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... > chrome/app/generated_resources.grd:6387: + Enable showing of autofill, > autocomplete and password manager suggestions for substring matching. > "Substring matching for Autofill suggestions." > > https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... > chrome/app/generated_resources.grd:6390: + Enable showing of autofill, > autocomplete and password manager suggestions for substring matching. This > allows only match against the beginning of each token like how omnibar > autocomplete does. > It's the omnibox, not omnibar. > > This description should take the form "Do x y z", i.e. "Match Autofill > suggestions based on substrings (token prefixes) rather than just prefixes." > > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > File components/autofill/core/common/autofill_util.cc (right): > > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > components/autofill/core/common/autofill_util.cc:42: result = StartsWith(token, > field_contents, case_sensitive); > for(...) { > if (StartsWith...) > return true; > } > > return false; > > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > components/autofill/core/common/autofill_util.cc:56: while ((it = std::search( > this should be a for loop. Why is |it| declared outside this loop? > > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > components/autofill/core/common/autofill_util.cc:62: if ((offset == 0) || > no extra parens
Thanks Evan, I've incorporated reviews in new patch, & may need to discuss this further: On 2015/05/11 19:07:12, Evan Stade wrote: > having patched this into my local machine and tried it out, I think my previous > comment was wrong: > > "instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place > and doing nothing special for prefix/non" > Thanks! https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:72: return (it != suggestion.end()) ? offset : base::string16::npos; On 2015/05/11 19:07:12, Evan Stade wrote: > On 2015/04/28 14:45:55, Pritam Nikam wrote: > > On 2015/04/27 19:25:21, Evan Stade wrote: > > > is it valid if it == suggestion.end()? It seems like this should not happen. > > > > it == suggestion.end() is equivalent to std::search() unable to find the > > field_contents in suggestion text. > > in which case you probably shouldn't have called GetTextSelectionStart on these > inputs then, right? Yes, thats right. But there is no restriction on doing so. https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... chrome/app/generated_resources.grd:6387: + Enable showing of autofill, autocomplete and password manager suggestions for substring matching. On 2015/05/11 19:07:12, Evan Stade wrote: > "Substring matching for Autofill suggestions." Done. https://codereview.chromium.org/962673004/diff/620001/chrome/app/generated_re... chrome/app/generated_resources.grd:6390: + Enable showing of autofill, autocomplete and password manager suggestions for substring matching. This allows only match against the beginning of each token like how omnibar autocomplete does. On 2015/05/11 19:07:12, Evan Stade wrote: > It's the omnibox, not omnibar. > > This description should take the form "Do x y z", i.e. "Match Autofill > suggestions based on substrings (token prefixes) rather than just prefixes." Done. https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:42: result = StartsWith(token, field_contents, case_sensitive); On 2015/05/11 19:07:12, Evan Stade wrote: > for(...) { > if (StartsWith...) > return true; > } > > return false; Done. https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:56: while ((it = std::search( On 2015/05/11 19:07:12, Evan Stade wrote: > this should be a for loop. Why is |it| declared outside this loop? Ok. I've changed this to: for (base::string16::const_iterator it = suggestion.begin(); (it = std::search( offset + suggestion.begin(), suggestion.end(), field_contents.begin(), field_contents.end(), base::CaseInsensitiveCompare<base::string16::value_type>())) != suggestion.end();) { ... } https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:62: if ((offset == 0) || On 2015/05/11 19:07:12, Evan Stade wrote: > no extra parens Done.
> having patched this into my local machine and tried it out, I think my previous > comment was wrong: > > "instead of waiting for UX feedback, I'd just go ahead with leaving MFU in place > and doing nothing special for prefix/non" > Have you addressed the above comment? It still looks like it's ordered by MFU. My concern after trying it out is that when I have two values [ Adam Smith ] and [ Shawn Smith ], if the first one is more popular, I can no longer type S [ down arrow ] [ enter ] to get Shawn Smith, which feels broken. https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:72: return (it != suggestion.end()) ? offset : base::string16::npos; On 2015/05/21 10:58:22, Pritam Nikam wrote: > On 2015/05/11 19:07:12, Evan Stade wrote: > > On 2015/04/28 14:45:55, Pritam Nikam wrote: > > > On 2015/04/27 19:25:21, Evan Stade wrote: > > > > is it valid if it == suggestion.end()? It seems like this should not > happen. > > > > > > it == suggestion.end() is equivalent to std::search() unable to find the > > > field_contents in suggestion text. > > > > in which case you probably shouldn't have called GetTextSelectionStart on > these > > inputs then, right? > > Yes, thats right. > But there is no restriction on doing so. then you should DCHECK and not attempt to handle this misuse https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:875: IsContentsPrefixOfSuggestionToken(values[i], field_contents, false)) { to match the naming format of StartsWith, can you make this "ContainsTokenThatStartsWith" also, why do you not use field_contents_canon and value_canon? I don't know if it's correct or not, but it looks weirdly divergent from the line above, so if it's correct you should justify it with a comment in the code. https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:71: // Unable to find the |field_contents| in |suggestion| text. notreached()
Patchset #30 (id:680001) has been deleted
Thanks Evan for review comments. I've tried incorporating your review inputs, ptal. Thanks! On 2015/05/22 18:26:48, Evan Stade wrote: > > having patched this into my local machine and tried it out, I think my > previous > > comment was wrong: > > > > "instead of waiting for UX feedback, I'd just go ahead with leaving MFU in > place > > and doing nothing special for prefix/non" > > > > Have you addressed the above comment? It still looks like it's ordered by MFU. > My concern after trying it out is that when I have two values [ Adam Smith ] and > [ Shawn Smith ], if the first one is more popular, I can no longer type > > S [ down arrow ] [ enter ] > > to get Shawn Smith, which feels broken. Done. https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:875: IsContentsPrefixOfSuggestionToken(values[i], field_contents, false)) { On 2015/05/22 18:26:48, Evan Stade wrote: > to match the naming format of StartsWith, can you make this > "ContainsTokenThatStartsWith" > Done. > also, why do you not use field_contents_canon and value_canon? I don't know if > it's correct or not, but it looks weirdly divergent from the line above, so if > it's correct you should justify it with a comment in the code. If we opt to use field_contents_canon and value_canon in IsContentsPrefixOfSuggestionToken(), field_contents_canon holds canonicalize input i.e. text having separators replaced with whitespace, and leads to unexpected results. For instance, values[i]: "test@example.org" -> value_canon: "test example org" field_contents: "exam...." -> field_contents_canon: "exam" This 'll pass IsContentsPrefixOfSuggestionToken() and will bring "test@example.org" in suggestions list, which seems wrong. https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/640001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:71: // Unable to find the |field_contents| in |suggestion| text. On 2015/05/22 18:26:48, Evan Stade wrote: > notreached() Done.
Patchset #32 (id:730001) has been deleted
Hi Evan, As suggested, with this patch I've ordered prefix followed by sub-string match suggestions, PTAL. Thanks!
Hi Pritam, can you rebase? This patch doesn't apply cleanly on my local machine.
https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:899: cards_to_suggest.insert(cards_to_suggest.end(), so it seems the substring matches are not ranked in any meaningful way? https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/suggestion.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.cc:50: return s1.match < s2.match; You probably want std::stable_sort (because we don't want to clobber the ordering that otherwise exists, i.e. MFU). https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/suggestion.h (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.h:57: void OrderPrefixBeforeSubstring(std::vector<Suggestion>* suggestions); I don't really think this belongs here --- it should go in a util file. Given the brevity of the implementation, though, it can probably just be inlined wherever it's needed.
estade@chromium.org changed reviewers: + rouslan@chromium.org
+rouslan to take over this review for me as I'll be on vacation soon. https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/suggestion.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.cc:50: return s1.match < s2.match; On 2015/06/08 23:23:38, Evan Stade wrote: > You probably want std::stable_sort (because we don't want to clobber the > ordering that otherwise exists, i.e. MFU). actually, I would suggest just extending OrderByMfu to also consider the match type.
On 2015/06/08 23:24:35, Evan Stade wrote: > +rouslan to take over this review for me as I'll be on vacation soon. Waiting for response to Evan's comments and a rebase before I start my review.
Thanks Evan and Rouslan for review. I've incorporated review inputs in new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:899: cards_to_suggest.insert(cards_to_suggest.end(), On 2015/06/08 23:23:38, Evan Stade wrote: > so it seems the substring matches are not ranked in any meaningful way? Done. https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/suggestion.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.cc:50: return s1.match < s2.match; On 2015/06/08 23:23:38, Evan Stade wrote: > You probably want std::stable_sort (because we don't want to clobber the > ordering that otherwise exists, i.e. MFU). Done. https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.cc:50: return s1.match < s2.match; On 2015/06/08 23:24:34, Evan Stade wrote: > On 2015/06/08 23:23:38, Evan Stade wrote: > > You probably want std::stable_sort (because we don't want to clobber the > > ordering that otherwise exists, i.e. MFU). > > actually, I would suggest just extending OrderByMfu to also consider the match > type. Did you mean RankByMfu (Ref [1])? This API does sort profiles (i.e. AutofillDataModel) and not the suggestion (struct Suggestion). Adding match type check does not seem right to me. [1]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... File components/autofill/core/browser/suggestion.h (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/cor... components/autofill/core/browser/suggestion.h:57: void OrderPrefixBeforeSubstring(std::vector<Suggestion>* suggestions); On 2015/06/08 23:23:38, Evan Stade wrote: > I don't really think this belongs here --- it should go in a util file. Given > the brevity of the implementation, though, it can probably just be inlined > wherever it's needed. Done.
https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:736: size_t start = autofill::GetTextSelectionStart(value, node->value()); You've calculated the |start| value previously, so it seems a bad design to re-calculate it again. I would change this particular function to the following. void AutofillAgent::PreviewFieldWithValue(size_t start, const base::string16& value, WebInputElement* node) { was_query_node_autofilled_ = element_.isAutofilled(); node->setSuggestedValue(value.substr(0, node->maxLength())); node->setAutofilled(true); node->setSelectionRange(start, node->suggestedValue().length()); } The next comment suggests that you change WebFormControlElement to have setSuggestedValue() setter. After you make that change, this function will look like this: void AutofillAgent::PreviewFieldWithValue(size_t start, const base::string16& value, WebInputElement* node) { was_query_node_autofilled_ = element_.isAutofilled(); node->setSuggestedValueStart( start > node->maxLength() ? node->maxLength() : start); node->setSuggestedValue(value.substr(0, node->maxLength())); node->setAutofilled(true); node->setSelectionRange(nodde->suggestedValueStart(), node->suggestedValue().length()); } https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:876: size_t start = autofill::GetTextSelectionStart(field->suggestedValue(), As in previous comment, recalculating the |start| seems like a bad design. The code here uses field->suggestedValue() to set the selected range. I think that WebFormControlElement should also contain suggestedValueStart() accessor and setSuggestedValueStart(int) setter. (Blink uses ints for range offsets by convention.) This is one of the files that you will need to modify in Blink: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Unfortunately Blink is still a separate repository, so you would need to create a separate patch. I can guide you through creating it, but you will need an LG from a Blink owner, who may have their own requirements. (One important thing to take care of will be a clear comment explaining the purpose of suggestedValueStart().) Once your Blink change lands, the code here will become much simpler. filed->setSelectionRange(field->suggestedValueStart(), field->suggetedValue().length());
Thanks Rouslan for review inputs. Currently we are using autofill::ContainsTokenThatStartsWith() verify substring matching check to populate suggestions and autofill::GetTextSelectionStart() for calculating |start| for selection range. We are calculating |start| only once per field. I've provided my understanding with couple of questions in comments. I'm happy to do blink side changes that you've suggested for text selection & that way use seems simpler and cleaner. Can you please help me with below comments for my understanding. Thanks! https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:736: size_t start = autofill::GetTextSelectionStart(value, node->value()); On 2015/06/11 19:10:25, Rouslan wrote: > You've calculated the |start| value previously, so it seems a bad design to > re-calculate it again. I would change this particular function to the following. > > void AutofillAgent::PreviewFieldWithValue(size_t start, > const base::string16& value, > WebInputElement* node) { > was_query_node_autofilled_ = element_.isAutofilled(); > node->setSuggestedValue(value.substr(0, node->maxLength())); > node->setAutofilled(true); > node->setSelectionRange(start, node->suggestedValue().length()); > } > > The next comment suggests that you change WebFormControlElement to have > setSuggestedValue() setter. After you make that change, this function will look > like this: > > void AutofillAgent::PreviewFieldWithValue(size_t start, > const base::string16& value, > WebInputElement* node) { > was_query_node_autofilled_ = element_.isAutofilled(); > node->setSuggestedValueStart( > start > node->maxLength() ? node->maxLength() : start); > node->setSuggestedValue(value.substr(0, node->maxLength())); > node->setAutofilled(true); > node->setSelectionRange(nodde->suggestedValueStart(), > node->suggestedValue().length()); > } We do not recalculate |start| here, we do it only once in renderer. Instead, on browser-process side we just verify whether suggestions (token) do have substring matching the user inputs (autofill::ContainsTokenThatStartsWith()) & order suggestions prefixes followed by substrings. And on renderer-process side while preview we do this calculation to get |start| index (autofill::GetTextSelectionStart()) just before the text range selection. Please correct me if I'm missing something here. Are you suggesting to get rid of ContainsTokenThatStartsWith() and instead calculate |start| using GetTextSelectionStart() on browser-process side while populating suggestions? IMO, in that case we do need to cache start indices along the suggestions and pass them along over IPC on previews (filed/form etc...) to renderer-process to use them to set selection range. https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:876: size_t start = autofill::GetTextSelectionStart(field->suggestedValue(), On 2015/06/11 19:10:26, Rouslan wrote: > As in previous comment, recalculating the |start| seems like a bad design. The > code here uses field->suggestedValue() to set the selected range. I think that > WebFormControlElement should also contain suggestedValueStart() accessor and > setSuggestedValueStart(int) setter. (Blink uses ints for range offsets by > convention.) This is one of the files that you will need to modify in Blink: > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Unfortunately Blink is still a separate repository, so you would need to create > a separate patch. I can guide you through creating it, but you will need an LG > from a Blink owner, who may have their own requirements. (One important thing to > take care of will be a clear comment explaining the purpose of > suggestedValueStart().) Once your Blink change lands, the code here will become > much simpler. > > filed->setSelectionRange(field->suggestedValueStart(), > field->suggetedValue().length()); As mentioned in my previous comment, blink side modifications will be needed if we calculate & cache |start| indices on browser-process side along the suggestions and pass these over IPC to the renderer for text range selection to preview fields/forms. IPC calls involve: 1. Autocomplete: AutofillMsg_PreviewFieldWithValue 2. Autofill form & CC name: AutofillMsg_PreviewForm 3. Password forms: AutofillMsg_PreviewPasswordSuggestion Please correct my understanding.
Hi Pritam, Sorry for the delay. Please find my explanations below. If I'm not being clear, please email me directly or ping me on IRC, whichever you prefer. Cheers, --Rouslan https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:736: size_t start = autofill::GetTextSelectionStart(value, node->value()); On 2015/06/12 11:09:11, Pritam Nikam wrote: > On 2015/06/11 19:10:25, Rouslan wrote: > > You've calculated the |start| value previously, so it seems a bad design to > > re-calculate it again. I would change this particular function to the > following. > > > > void AutofillAgent::PreviewFieldWithValue(size_t start, > > const base::string16& value, > > WebInputElement* node) { > > was_query_node_autofilled_ = element_.isAutofilled(); > > node->setSuggestedValue(value.substr(0, node->maxLength())); > > node->setAutofilled(true); > > node->setSelectionRange(start, node->suggestedValue().length()); > > } > > > > The next comment suggests that you change WebFormControlElement to have > > setSuggestedValue() setter. After you make that change, this function will > look > > like this: > > > > void AutofillAgent::PreviewFieldWithValue(size_t start, > > const base::string16& value, > > WebInputElement* node) { > > was_query_node_autofilled_ = element_.isAutofilled(); > > node->setSuggestedValueStart( > > start > node->maxLength() ? node->maxLength() : start); > > node->setSuggestedValue(value.substr(0, node->maxLength())); > > node->setAutofilled(true); > > node->setSelectionRange(nodde->suggestedValueStart(), > > node->suggestedValue().length()); > > } > > We do not recalculate |start| here, we do it only once in renderer. > > Instead, on browser-process side we just verify whether suggestions (token) do > have substring matching the user inputs > (autofill::ContainsTokenThatStartsWith()) & order suggestions prefixes followed > by substrings. And on renderer-process side while preview we do this calculation > to get |start| index (autofill::GetTextSelectionStart()) just before the text > range selection. Please correct me if I'm missing something here. > > Are you suggesting to get rid of ContainsTokenThatStartsWith() and instead > calculate |start| using GetTextSelectionStart() on browser-process side while > populating suggestions? > > IMO, in that case we do need to cache start indices along the suggestions and > pass them along over IPC on previews (filed/form etc...) to renderer-process to > use them to set selection range. Ah, I was slightly confused. I thought that you calculate |start| in the browser and also in the renderer. After looking at the code some more, I see that you're actually doing tokenization in the browser and call GetTextSelectionStart() in the renderer. I think this approach is not the best, because there's a slight chance that GetTextSelectionStart() will return base::string16::npos even though ContainsTokenThatStartsWith() returned false. For example,. I see that ContainsTokenThatStartsWith() takes a |case_sensitive| parameter, but |GetTextSelectionStart()| does not. This is one discrepancy that raises alarms. Therefore, I think that you should rework autofill_util.cc so that it has only GetTextSelectionStart(). The caller to GetTextSelectionStart() should check for base::string16::npos. If the return value is not base::string16::npos, then it should be stored in node->setSuggestedValueStart(int). Yes, this will involve changing some IPCs. I would also prefer that you change components/autofill/core/browser/suggestion.cc to contain a "size_t match_start;" instead of MatchMode. MatchMode enum is not necessary, because PREFIX_MATCH is essentially the same as a SUBSBSTRING_MATCH where |match_start| is always 0. This change will make more of your code generic. https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:876: size_t start = autofill::GetTextSelectionStart(field->suggestedValue(), On 2015/06/12 11:09:11, Pritam Nikam wrote: > On 2015/06/11 19:10:26, Rouslan wrote: > > As in previous comment, recalculating the |start| seems like a bad design. The > > code here uses field->suggestedValue() to set the selected range. I think that > > WebFormControlElement should also contain suggestedValueStart() accessor and > > setSuggestedValueStart(int) setter. (Blink uses ints for range offsets by > > convention.) This is one of the files that you will need to modify in Blink: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Unfortunately Blink is still a separate repository, so you would need to > create > > a separate patch. I can guide you through creating it, but you will need an LG > > from a Blink owner, who may have their own requirements. (One important thing > to > > take care of will be a clear comment explaining the purpose of > > suggestedValueStart().) Once your Blink change lands, the code here will > become > > much simpler. > > > > filed->setSelectionRange(field->suggestedValueStart(), > > field->suggetedValue().length()); > > As mentioned in my previous comment, blink side modifications will be needed if > we calculate & cache |start| indices on browser-process side along the > suggestions and pass these over IPC to the renderer for text range selection to > preview fields/forms. > > IPC calls involve: > 1. Autocomplete: AutofillMsg_PreviewFieldWithValue Looks right. > 2. Autofill form & CC name: AutofillMsg_PreviewForm This IPC does not send the actual element over the wire. It's using the member variable |element_|. So no need to modify this particular method. > 3. Password forms: AutofillMsg_PreviewPasswordSuggestion Looks right. > > Please correct my understanding. Your understanding is correct.
Continuing the discussion here is also OK, of course. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thanks Rouslan for your inputs. I've replied below, please have a look. Thanks! https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:736: size_t start = autofill::GetTextSelectionStart(value, node->value()); On 2015/06/22 00:32:59, Rouslan wrote: > On 2015/06/12 11:09:11, Pritam Nikam wrote: > > On 2015/06/11 19:10:25, Rouslan wrote: > > > You've calculated the |start| value previously, so it seems a bad design to > > > re-calculate it again. I would change this particular function to the > > following. > > > > > > void AutofillAgent::PreviewFieldWithValue(size_t start, > > > const base::string16& value, > > > WebInputElement* node) { > > > was_query_node_autofilled_ = element_.isAutofilled(); > > > node->setSuggestedValue(value.substr(0, node->maxLength())); > > > node->setAutofilled(true); > > > node->setSelectionRange(start, node->suggestedValue().length()); > > > } > > > > > > The next comment suggests that you change WebFormControlElement to have > > > setSuggestedValue() setter. After you make that change, this function will > > look > > > like this: > > > > > > void AutofillAgent::PreviewFieldWithValue(size_t start, > > > const base::string16& value, > > > WebInputElement* node) { > > > was_query_node_autofilled_ = element_.isAutofilled(); > > > node->setSuggestedValueStart( > > > start > node->maxLength() ? node->maxLength() : start); > > > node->setSuggestedValue(value.substr(0, node->maxLength())); > > > node->setAutofilled(true); > > > node->setSelectionRange(nodde->suggestedValueStart(), > > > node->suggestedValue().length()); > > > } > > > > We do not recalculate |start| here, we do it only once in renderer. > > > > Instead, on browser-process side we just verify whether suggestions (token) do > > have substring matching the user inputs > > (autofill::ContainsTokenThatStartsWith()) & order suggestions prefixes > followed > > by substrings. And on renderer-process side while preview we do this > calculation > > to get |start| index (autofill::GetTextSelectionStart()) just before the text > > range selection. Please correct me if I'm missing something here. > > > > Are you suggesting to get rid of ContainsTokenThatStartsWith() and instead > > calculate |start| using GetTextSelectionStart() on browser-process side while > > populating suggestions? > > > > IMO, in that case we do need to cache start indices along the suggestions and > > pass them along over IPC on previews (filed/form etc...) to renderer-process > to > > use them to set selection range. > > Ah, I was slightly confused. I thought that you calculate |start| in the browser > and also in the renderer. After looking at the code some more, I see that you're > actually doing tokenization in the browser and call GetTextSelectionStart() in > the renderer. I think this approach is not the best, because there's a slight > chance that GetTextSelectionStart() will return base::string16::npos even though > ContainsTokenThatStartsWith() returned false. Cases where ContainsTokenThatStartsWith() returned false, GetTextSelectionStart() will not get call. Moreover, in GetTextSelectionStart() we have a cushion of NOTREACHED() just to address this case. > For example,. I see that > ContainsTokenThatStartsWith() takes a |case_sensitive| parameter, but > |GetTextSelectionStart()| does not. This is one discrepancy that raises alarms. > Once we have shortlisted suggestions by considering |case_sensitive|, for preview we can safely ignore it. > > Therefore, I think that you should rework autofill_util.cc so that it has only > GetTextSelectionStart(). The caller to GetTextSelectionStart() should check for > base::string16::npos. If the return value is not base::string16::npos, then it > should be stored in node->setSuggestedValueStart(int). > > Yes, this will involve changing some IPCs. > > I would also prefer that you change > components/autofill/core/browser/suggestion.cc to contain a "size_t > match_start;" instead of MatchMode. MatchMode enum is not necessary, because > PREFIX_MATCH is essentially the same as a SUBSBSTRING_MATCH where |match_start| > is always 0. This change will make more of your code generic. If its ok, I'll do these in follow-up CLs: 1. Blink side modifications - adding setSuggestedValueStart 2. Chromium side - Changes in suggestion structure and IPCs. https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:876: size_t start = autofill::GetTextSelectionStart(field->suggestedValue(), On 2015/06/22 00:32:59, Rouslan wrote: > On 2015/06/12 11:09:11, Pritam Nikam wrote: > > On 2015/06/11 19:10:26, Rouslan wrote: > > > As in previous comment, recalculating the |start| seems like a bad design. > The > > > code here uses field->suggestedValue() to set the selected range. I think > that > > > WebFormControlElement should also contain suggestedValueStart() accessor and > > > setSuggestedValueStart(int) setter. (Blink uses ints for range offsets by > > > convention.) This is one of the files that you will need to modify in Blink: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > > > Unfortunately Blink is still a separate repository, so you would need to > > create > > > a separate patch. I can guide you through creating it, but you will need an > LG > > > from a Blink owner, who may have their own requirements. (One important > thing > > to > > > take care of will be a clear comment explaining the purpose of > > > suggestedValueStart().) Once your Blink change lands, the code here will > > become > > > much simpler. > > > > > > filed->setSelectionRange(field->suggestedValueStart(), > > > field->suggetedValue().length()); > > > > As mentioned in my previous comment, blink side modifications will be needed > if > > we calculate & cache |start| indices on browser-process side along the > > suggestions and pass these over IPC to the renderer for text range selection > to > > preview fields/forms. > > > > IPC calls involve: > > 1. Autocomplete: AutofillMsg_PreviewFieldWithValue > > Looks right. > > > 2. Autofill form & CC name: AutofillMsg_PreviewForm > > This IPC does not send the actual element over the wire. It's using the member > variable |element_|. So no need to modify this particular method. > > > 3. Password forms: AutofillMsg_PreviewPasswordSuggestion > > Looks right. > > > > Please correct my understanding. > > Your understanding is correct. Acknowledged.
On 2015/06/22 18:31:40, Pritam Nikam wrote: > If its ok, I'll do these in follow-up CLs: > 1. Blink side modifications - adding setSuggestedValueStart > 2. Chromium side - Changes in suggestion structure and IPCs. I think that this is a design-level change, not a minor implementation detail. Therefore I would rather do the blink and IPC changes first. I'd like to avoid checking in code that we plan to change immediately.
On 2015/06/23 22:09:30, Rouslan wrote: > On 2015/06/22 18:31:40, Pritam Nikam wrote: > > If its ok, I'll do these in follow-up CLs: > > 1. Blink side modifications - adding setSuggestedValueStart Blink side changes: https://codereview.chromium.org/1208063002/ > > 2. Chromium side changes: Changes in suggestion structure and IPCs. > Chromium IPC changes: https://codereview.chromium.org/1208133002/ > I think that this is a design-level change, not a minor implementation detail. > Therefore I would rather do the blink and IPC changes first. I'd like to avoid > checking in code that we plan to change immediately. Thanks!
Pritam, Thank you for creating the other two patches for Blink and IPC changes. After reviewing them, I was convinced that your original approach is better. Let's discard the Blink and IPC patches and continue the review here. I found a bug in your patch: If my street address is "123 Main Street", then "123 main" will correctly suggest my street address, but "main st" does not match my street address. I don't know what's causing it, but I bet that some more testing of autofill_util.cc will reveal the problem cause. Please fix it. Also, here's a few refactoring comments: https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:745: if (IsFeatureSubstringMatchEnabled()) { You can inline this: node->setSelectionRange( IsFeatureSubstringMatchEnabled() ? autofill::GetTextSelectionStart(value, node->value()) : node->value().length(), node->suggestedValue().length()); You use this code in at least two places. It's a good candidate to be placed into autofill_util.h. Please do that. You can call it PreviewSuggestion() or something better you can come up with. https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:875: if (IsFeatureSubstringMatchEnabled()) { Use PreviewSuggestion() here. https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:230: ContainsTokenThatStartsWith(username1, username2, true); 1) Change ContainsTokenThatStartsWith() to use base::StartsWith() internally, when command line switch |kEnableSuggestionsWithSubstringMatch| is off. 2) Use only ContainsTokenThatStartsWith() here. https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:420: if (IsFeatureSubstringMatchEnabled()) { Use PreviewSuggestion() here. https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:803: base::string16 suggested_username = username; No need for these temporary variables on lines 802-803. Is there? https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:809: if (IsFeatureSubstringMatchEnabled()) { Use PreviewSuggestion() here.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:91: std::vector<base::string16> preferred_suggestions; Wouldn't it be better to perform this logic in SQL? https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... After performing the first query, if substring match is enabled, then add a query like this: sql::Statement s2; s2.Assign(db_->GetUniqueStatement( "SELECT value FROM autofill " "WHERE name = ? AND (" " value LIKE ? OR " " value LIKE ? OR " " value LIKE ? OR " " value LIKE ? OR " " value LIKE ? OR " " value LIKE ? ) " "ORDER BY count DESC " "LIMIT ?"); s2.BindString16(0, name); s2.BindString16(1, " " + prefix_lower); s2.BindString16(2, "." + prefix_lower); s2.BindString16(3, "," + prefix_lower); s2.BindString16(4, "-" + prefix_lower); s2.BindString16(5, "_" + prefix_lower); s2.BindString16(6, "@" + prefix_lower); s2.BindInt(7, limit); while (s2.Step()) values->push_back(s2.ColumnString16(0)); retrun s.Succeeded() && s2.Succeeded(); Since you will perform this query second, the results will be at the end of |values|, as is desired.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:20: const base::string16 kSplitCharacters = base::ASCIIToUTF16(" .,-_@"); Wouldn't this statement will require an exit-time distractor, prohibited in Chrome? Use an array instead: static const char kSplitCharacters[] = " .,-_@"; https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:36: Tokenize(suggestion, kSplitCharacters, &suggestion_tokens); Tokenize() is deprecated. Use base::SplitString(). https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.h:13: // |kEnableSuggestionsWithSubstringMatch| is on; otherwise |false|. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... "It is not necessary to say 'returns false otherwise' because this is implied."
Thanks Rouslan for detail review.
I've incorporated most of your inputs. There were few difficulties to address
these and few observations from my side. I've provided my comments for the same.
Please have a look.
Thanks!
On 2015/06/28 00:43:04, Rouslan wrote:
> Pritam,
>
> Thank you for creating the other two patches for Blink and IPC changes. After
> reviewing them, I was convinced that your original approach is better. Let's
> discard the Blink and IPC patches and continue the review here.
>
> I found a bug in your patch: If my street address is "123 Main Street", then
> "123 main" will correctly suggest my street address, but "main st" does not
> match my street address. I don't know what's causing it, but I bet that some
> more testing of autofill_util.cc will reveal the problem cause. Please fix it.
I'd say its an expected behaviour.
For substring match case, we just check whether the |field_contents| prefixes
any of the |suggestion|'s token. And cases where |field_contents| span multiple
|suggestions| text tokens funtion |ContainsTokenThatStartsWith| simply return
false.
Unit test addressing the same:
AutofillUtilTest.ContainsTokenThatStartsWith
38 EXPECT_FALSE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"),
39 base::ASCIIToUTF16("cd.b"),
false));
&
PasswordAutofillManagerTest.MatchingContentsWithSuggestionTokenSeparator
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:745: if
(IsFeatureSubstringMatchEnabled()) {
On 2015/06/28 00:43:03, Rouslan wrote:
> You can inline this:
>
> node->setSelectionRange(
> IsFeatureSubstringMatchEnabled()
> ? autofill::GetTextSelectionStart(value, node->value())
> : node->value().length(),
> node->suggestedValue().length());
>
> You use this code in at least two places. It's a good candidate to be placed
> into autofill_util.h. Please do that. You can call it PreviewSuggestion() or
> something better you can come up with.
Sure, I'll move this code restructuring under |PreviewSuggestion()| API.
However, moving it to |autofill_util.h| will be a problem, as
|PreviewSuggestion()| will have blink dependency i.e. class WebInputElement, and
it'll not fly. Instead I've moved it to |form_autofill_util.h| i.e. under
content/renderer folder.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:875: if
(IsFeatureSubstringMatchEnabled()) {
On 2015/06/28 00:43:03, Rouslan wrote:
> Use PreviewSuggestion() here.
Ditto.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:230:
ContainsTokenThatStartsWith(username1, username2, true);
On 2015/06/28 00:43:03, Rouslan wrote:
> 1) Change ContainsTokenThatStartsWith() to use base::StartsWith() internally,
> when command line switch |kEnableSuggestionsWithSubstringMatch| is off.
>
> 2) Use only ContainsTokenThatStartsWith() here.
We do rank suggestions based on MFU, moving |base::StartsWith()| inside
|ContainsTokenThatStartsWith()| will go for a toss, and we may endup in writing
more code to handle it.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:420: if
(IsFeatureSubstringMatchEnabled()) {
On 2015/06/28 00:43:03, Rouslan wrote:
> Use PreviewSuggestion() here.
Acknowledged.
I'm getting some problem here, shall address it soon.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:803:
base::string16 suggested_username = username;
On 2015/06/28 00:43:03, Rouslan wrote:
> No need for these temporary variables on lines 802-803. Is there?
Done.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:809: if
(IsFeatureSubstringMatchEnabled()) {
On 2015/06/28 00:43:03, Rouslan wrote:
> Use PreviewSuggestion() here.
Done.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
File components/autofill/core/browser/autocomplete_history_manager.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autocomplete_history_manager.cc:91:
std::vector<base::string16> preferred_suggestions;
On 2015/06/28 01:20:07, Rouslan wrote:
> Wouldn't it be better to perform this logic in SQL?
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
>
> After performing the first query, if substring match is enabled, then add a
> query like this:
>
> sql::Statement s2;
> s2.Assign(db_->GetUniqueStatement(
> "SELECT value FROM autofill "
> "WHERE name = ? AND ("
> " value LIKE ? OR "
> " value LIKE ? OR "
> " value LIKE ? OR "
> " value LIKE ? OR "
> " value LIKE ? OR "
> " value LIKE ? ) "
> "ORDER BY count DESC "
> "LIMIT ?");
> s2.BindString16(0, name);
> s2.BindString16(1, " " + prefix_lower);
> s2.BindString16(2, "." + prefix_lower);
> s2.BindString16(3, "," + prefix_lower);
> s2.BindString16(4, "-" + prefix_lower);
> s2.BindString16(5, "_" + prefix_lower);
> s2.BindString16(6, "@" + prefix_lower);
> s2.BindInt(7, limit);
> while (s2.Step())
> values->push_back(s2.ColumnString16(0));
>
We need to pass '%<delimiter> + prefix_lower + %'. But there is one problem:
'_' being keyword, suggestions like "test_user" on user input "use" would not
get populated. Moreover, groups i.e. [] does not seems working (tested on my
local linux setup) with chromium.
> retrun s.Succeeded() && s2.Succeeded();
>
>
did you mean "retrun s.Succeeded() || s2.Succeeded();"
>
> Since you will perform this query second, the results will be at the end of
> |values|, as is desired.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
File components/autofill/core/common/autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/common/autofill_util.cc:20: const base::string16
kSplitCharacters = base::ASCIIToUTF16(" .,-_@");
On 2015/06/28 01:29:39, Rouslan wrote:
> Wouldn't this statement will require an exit-time distractor, prohibited in
> Chrome? Use an array instead:
>
> static const char kSplitCharacters[] = " .,-_@";
Done.
Do I need to set "-Wexit-time-destructors" explicitly? Because I didn't notice
any warning on console.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/common/autofill_util.cc:36: Tokenize(suggestion,
kSplitCharacters, &suggestion_tokens);
On 2015/06/28 01:29:39, Rouslan wrote:
> Tokenize() is deprecated. Use base::SplitString().
Done.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
File components/autofill/core/common/autofill_util.h (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/common/autofill_util.h:13: //
|kEnableSuggestionsWithSubstringMatch| is on; otherwise |false|.
On 2015/06/28 01:29:39, Rouslan wrote:
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com...
>
> "It is not necessary to say 'returns false otherwise' because this is
implied."
Done.
On 2015/06/29 15:38:21, Pritam Nikam wrote:
> On 2015/06/28 00:43:04, Rouslan wrote:
> > I found a bug in your patch: If my street address is "123 Main Street", then
> > "123 main" will correctly suggest my street address, but "main st" does not
> > match my street address. I don't know what's causing it, but I bet that some
> > more testing of autofill_util.cc will reveal the problem cause. Please fix
it.
>
> I'd say its an expected behaviour.
>
> For substring match case, we just check whether the |field_contents| prefixes
> any of the |suggestion|'s token. And cases where |field_contents| span
multiple
> |suggestions| text tokens funtion |ContainsTokenThatStartsWith| simply return
> false.
>
> Unit test addressing the same:
>
> AutofillUtilTest.ContainsTokenThatStartsWith
> 38 EXPECT_FALSE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"),
> 39 base::ASCIIToUTF16("cd.b"),
> false));
>
> &
>
> PasswordAutofillManagerTest.MatchingContentsWithSuggestionTokenSeparator
I understand that it's your code's current behavior and it is well tested.
However, that's not how the code should behave, IMHO. Adding a test case to
verify incorrect behavior does not make the behavior correct :-). Was there a
discussion about it that I missed?
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:745: if
(IsFeatureSubstringMatchEnabled()) {
On 2015/06/29 15:38:21, Pritam Nikam wrote:
> On 2015/06/28 00:43:03, Rouslan wrote:
> > You can inline this:
> >
> > node->setSelectionRange(
> > IsFeatureSubstringMatchEnabled()
> > ? autofill::GetTextSelectionStart(value, node->value())
> > : node->value().length(),
> > node->suggestedValue().length());
> >
> > You use this code in at least two places. It's a good candidate to be placed
> > into autofill_util.h. Please do that. You can call it PreviewSuggestion() or
> > something better you can come up with.
>
> Sure, I'll move this code restructuring under |PreviewSuggestion()| API.
> However, moving it to |autofill_util.h| will be a problem, as
> |PreviewSuggestion()| will have blink dependency i.e. class WebInputElement,
and
> it'll not fly. Instead I've moved it to |form_autofill_util.h| i.e. under
> content/renderer folder.
Sounds good.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
File components/autofill/core/browser/autocomplete_history_manager.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/browser/autocomplete_history_manager.cc:91:
std::vector<base::string16> preferred_suggestions;
On 2015/06/29 15:38:21, Pritam Nikam wrote:
> We need to pass '%<delimiter> + prefix_lower + %'. But there is one problem:
>
> '_' being keyword, suggestions like "test_user" on user input "use" would not
> get populated.
Use '\_' instead?
> Moreover, groups i.e. [] does not seems working (tested on my
> local linux setup) with chromium.
Why do you need []? Do () work?
> > return s.Succeeded() && s2.Succeeded();
> >
> >
>
> did you mean "return s.Succeeded() || s2.Succeeded();"
I don't have a strong preference, as long as you document the behavior in
function comment.
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
File components/autofill/core/common/autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
components/autofill/core/common/autofill_util.cc:20: const base::string16
kSplitCharacters = base::ASCIIToUTF16(" .,-_@");
On 2015/06/29 15:38:21, Pritam Nikam wrote:
> On 2015/06/28 01:29:39, Rouslan wrote:
> > Wouldn't this statement will require an exit-time distractor, prohibited in
> > Chrome? Use an array instead:
> >
> > static const char kSplitCharacters[] = " .,-_@";
>
> Done.
>
> Do I need to set "-Wexit-time-destructors" explicitly? Because I didn't notice
> any warning on console.
There's some bot that runs on your code once it's checked in and reverts your
patch if there's an exit time destructor. I don't recall the name of the bot,
but you don't want to encounter it :-).
Thanks Rouslan again for quick reply.
Please find my in-line replies.
Thanks!
On 2015/06/29 17:06:45, Rouslan wrote:
> On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > On 2015/06/28 00:43:04, Rouslan wrote:
> > > I found a bug in your patch: If my street address is "123 Main Street",
then
> > > "123 main" will correctly suggest my street address, but "main st" does
not
> > > match my street address. I don't know what's causing it, but I bet that
some
> > > more testing of autofill_util.cc will reveal the problem cause. Please fix
> it.
> >
> > I'd say its an expected behaviour.
> >
> > For substring match case, we just check whether the |field_contents|
prefixes
> > any of the |suggestion|'s token. And cases where |field_contents| span
> multiple
> > |suggestions| text tokens funtion |ContainsTokenThatStartsWith| simply
return
> > false.
> >
> > Unit test addressing the same:
> >
> > AutofillUtilTest.ContainsTokenThatStartsWith
> > 38 EXPECT_FALSE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"),
> > 39 base::ASCIIToUTF16("cd.b"),
> > false));
> >
> > &
> >
> > PasswordAutofillManagerTest.MatchingContentsWithSuggestionTokenSeparator
>
> I understand that it's your code's current behavior and it is well tested.
> However, that's not how the code should behave, IMHO. Adding a test case to
> verify incorrect behavior does not make the behavior correct :-). Was there a
> discussion about it that I missed?
>
Yes this behaviour was discussed in past (with Vaclav). You could find related
conversation as well - not able to locate it now :(
Earlier I had kept it like how you've suggested, i.e. user input can span
multiple suggestion tokens.
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
> File components/autofill/content/renderer/autofill_agent.cc (right):
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
> components/autofill/content/renderer/autofill_agent.cc:745: if
> (IsFeatureSubstringMatchEnabled()) {
> On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > On 2015/06/28 00:43:03, Rouslan wrote:
> > > You can inline this:
> > >
> > > node->setSelectionRange(
> > > IsFeatureSubstringMatchEnabled()
> > > ? autofill::GetTextSelectionStart(value, node->value())
> > > : node->value().length(),
> > > node->suggestedValue().length());
> > >
> > > You use this code in at least two places. It's a good candidate to be
placed
> > > into autofill_util.h. Please do that. You can call it PreviewSuggestion()
or
> > > something better you can come up with.
> >
> > Sure, I'll move this code restructuring under |PreviewSuggestion()| API.
> > However, moving it to |autofill_util.h| will be a problem, as
> > |PreviewSuggestion()| will have blink dependency i.e. class WebInputElement,
> and
> > it'll not fly. Instead I've moved it to |form_autofill_util.h| i.e. under
> > content/renderer folder.
>
> Sounds good.
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> File components/autofill/core/browser/autocomplete_history_manager.cc (right):
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> components/autofill/core/browser/autocomplete_history_manager.cc:91:
> std::vector<base::string16> preferred_suggestions;
> On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > We need to pass '%<delimiter> + prefix_lower + %'. But there is one problem:
> >
> > '_' being keyword, suggestions like "test_user" on user input "use" would
not
> > get populated.
>
> Use '\_' instead?
>
> > Moreover, groups i.e. [] does not seems working (tested on my
> > local linux setup) with chromium.
>
>
> Why do you need []? Do () work?
'\_' doesn't seem working.
I thought of using, pattern ('%[_]foo%') as well, but that didn't work.
>
> > > return s.Succeeded() && s2.Succeeded();
> > >
> > >
> >
> > did you mean "return s.Succeeded() || s2.Succeeded();"
>
> I don't have a strong preference, as long as you document the behavior in
> function comment.
Sure will do that.
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> File components/autofill/core/common/autofill_util.cc (right):
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> components/autofill/core/common/autofill_util.cc:20: const base::string16
> kSplitCharacters = base::ASCIIToUTF16(" .,-_@");
> On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > On 2015/06/28 01:29:39, Rouslan wrote:
> > > Wouldn't this statement will require an exit-time distractor, prohibited
in
> > > Chrome? Use an array instead:
> > >
> > > static const char kSplitCharacters[] = " .,-_@";
> >
> > Done.
> >
> > Do I need to set "-Wexit-time-destructors" explicitly? Because I didn't
notice
> > any warning on console.
>
> There's some bot that runs on your code once it's checked in and reverts your
> patch if there's an exit time destructor. I don't recall the name of the bot,
> but you don't want to encounter it :-).
Fair enough :-)
Please let me know when I can continue reviewing your code.
On 2015/06/29 17:19:09, Pritam Nikam wrote:
> '\_' doesn't seem working.
>
> I thought of using, pattern ('%[_]foo%') as well, but that didn't work.
Fair enough. Since you've done your due diligence, it's OK to keep this in
autocomplete_history_manager.cc instead of SQL. Do me a favor though: document
this unavailability of SQL solutions in a comment above the new code:
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
On 2015/06/29 17:31:02, Rouslan wrote:
> Please let me know when I can continue reviewing your code.
>
> On 2015/06/29 17:19:09, Pritam Nikam wrote:
> > '\_' doesn't seem working.
> >
> > I thought of using, pattern ('%[_]foo%') as well, but that didn't work.
>
> Fair enough. Since you've done your due diligence, it's OK to keep this in
> autocomplete_history_manager.cc instead of SQL. Do me a favor though: document
> this unavailability of SQL solutions in a comment above the new code:
>
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
Hi Rouslan,
I'm left with one comment here [1]. Rest of the comments I've addressed already.
[1].
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
Thanks!
Pritam, please let me know when you address all outstanding comments, so I can go in for another pass.
On 2015/06/29 18:31:06, Rouslan wrote: > Pritam, please let me know when you address all outstanding comments, so I can > go in for another pass. Hi Rouslan, I guess you can go ahead with review, as restructuring [1] mentioned above will need more code changes and IMO, thats not worth the effort. Having said that, I will give it another try in morning. But that will be quite late for you. Thanks!
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:230: ContainsTokenThatStartsWith(username1, username2, true); On 2015/06/29 15:38:21, Pritam Nikam wrote: > On 2015/06/28 00:43:03, Rouslan wrote: > > 1) Change ContainsTokenThatStartsWith() to use base::StartsWith() internally, > > when command line switch |kEnableSuggestionsWithSubstringMatch| is off. > > > > 2) Use only ContainsTokenThatStartsWith() here. > > We do rank suggestions based on MFU, moving |base::StartsWith()| inside > |ContainsTokenThatStartsWith()| will go for a toss, and we may endup in writing > more code to handle it. Do we ever use ContainsTokenThatStartsWith() without "base::StartsWith() ||" beforehand? https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:420: if (IsFeatureSubstringMatchEnabled()) { On 2015/06/29 15:38:21, Pritam Nikam wrote: > On 2015/06/28 00:43:03, Rouslan wrote: > > Use PreviewSuggestion() here. > > Acknowledged. > > > I'm getting some problem here, shall address it soon. I can help you resolve the issue if you describe it. https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:803: base::string16 suggested_username = username; On 2015/06/29 15:38:21, Pritam Nikam wrote: > On 2015/06/28 00:43:03, Rouslan wrote: > > No need for these temporary variables on lines 802-803. Is there? > > Done. They're still here. I think you were planning to remove them but forgot. https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.h:13: // |kEnableSuggestionsWithSubstringMatch| is on; otherwise |false|. On 2015/06/29 15:38:21, Pritam Nikam wrote: > On 2015/06/28 01:29:39, Rouslan wrote: > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > > > > "It is not necessary to say 'returns false otherwise' because this is > implied." > > Done. You've removed "; otherwise false" below, but you did not remove "; otherwise |false|" above. Please remove that one too. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1538: DCHECK(input_element); The rest of the code in this file does not DCHECK pointers before dereferencing them. Let's be consistent and not DCHECK in your function either. No worries, both DCHECK and dereference behave the same way (crash with a stack trace). https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1541: ? autofill::GetTextSelectionStart(suggestion, input_element->value()) Either use |suggestion| or input_element->suggestedValue() throughout the function, but not both. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text. Please document that |input_element| should not be null. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: ::autofill::PreviewSuggestion(suggested_username, &username_element); Why "::autofill::"? https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:93: for (base::string16 suggestion : suggestions) { const base::string16& https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:134: // future so that logic can be handled in SQL query instead. No need for "Revisit.." sentence and TODO marker. Instead, please find the sqlite documentation that explains why sqlite filtering will not work for our case and paste the link in the comment. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.h:76: base::string16 pending_query_prefix_; Let's call it "last_query_prefix" and put a newline afterwards. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3076: // Test that suggestion tokens (substrings separated by characters from " Please be more specific. For example, 'Verify that typing "gmail" will match "theking@gmail.com" and "buddy@gmail.com" when substring matching is enabled.' https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3103: // .,-_@") are matched against field contents ignoring their case. Please be more specific. For example, 'Verify that typing "apple" will match "123 Apple St." when substring matching is enabled.' https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3127: // Test that suggestions which do not have a prefix match or prefix-token match Please be more specific. For example, 'Verify that typing "mail" will not match any of the "@gmail.com" email addresses when substring matching is enabled.' You get the idea. The same for the rest of the tests, too, please. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:799: // CanonicalizeProfileString() returns lower-case string with all CanonicalizeProfileString() does not replace "all separators". Please correct your comment. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:803: // preferred passing |values[i]| and |field_contents| instead. This part of the comment is outdated. There's no values[i] anywhere in this file. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:807: ContainsTokenThatStartsWith(value, field_contents, false))) { It's rare in Chromium code to assign a variable inside of an if clause. Is there a simpler way to write this without such assignment? https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:822: }); Is this clang formatted? https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:876: std::list<const CreditCard*> substring_matched_suggestions; s/substring_matched_suggestions/substring_matched_cards https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/suggestion.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/suggestion.h:23: }; newline after }; https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:7: #include <vector> Also <algorithm> for std::search. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:32: std::vector<base::string16> suggestion_tokens = You can probably save one copy operation by using "const std::vector<base::string16>&" here. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:37: // token. s/token/tokens https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:48: size_t offset = 0; You don't need |offset| variable. You can use "std::search(it, suggestion.end(), ...)". Then you can return "it - suggestion.begin() + field_contents.size()". Instead of "++offset", you can put "++it" in the third part of "for(<1>;<2>;<3>)". https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:49: // Loop until we find either the |field_contents| begins with the |suggestion| s/begins with/is a prefix of Right? https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:18: // on and |field_contents| is a prefix of some token. "... of a token in |suggestion|." https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: // Finds if the |field_contents| prefixes any of the |suggestion| token, s/token/tokens https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:24: // separated by characters from " .,-_@". Returns the start position of the text Remove "from". It's a bit confusing IMHO. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:24: EXPECT_TRUE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"), Use an array of structs similar to the next test case, please. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:53: base::string16 field_suggestion; Use "char*" instead of "base::string16". Don't base::ASCIIToUTF16() when building the struct. Do that inside of the loop instead. https://codereview.chromium.org/962673004/diff/920001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:73: (substring_matched_suggestion = autofill::ContainsTokenThatStartsWith( If you can figure out a way to write this without an assignment inside of the if-clause, that we great. :-D https://codereview.chromium.org/962673004/diff/920001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:359: // Test that suggestion tokens (substrings separated by characters from Please be more specific in this comment and the comments below. For example, this comment can be clarified by rewriting it as, 'Verify that typing "foo" into the username field will match usernames "foo.bar@example.com", "bar.foo@example.com", and "example@foo.com".'
Thanks Rouslan for review. I've incorporated review comments in new patch set, ptal. Thanks! https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:230: ContainsTokenThatStartsWith(username1, username2, true); On 2015/06/29 22:06:28, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > On 2015/06/28 00:43:03, Rouslan wrote: > > > 1) Change ContainsTokenThatStartsWith() to use base::StartsWith() > internally, > > > when command line switch |kEnableSuggestionsWithSubstringMatch| is off. > > > > > > 2) Use only ContainsTokenThatStartsWith() here. > > > > We do rank suggestions based on MFU, moving |base::StartsWith()| inside > > |ContainsTokenThatStartsWith()| will go for a toss, and we may endup in > writing > > more code to handle it. > > Do we ever use ContainsTokenThatStartsWith() without "base::StartsWith() ||" > beforehand? Yes. We have to treat both cases seperatly for below cases: 1. components/password_manager/core/browser/password_autofill_manager.cc 2. components/autofill/core/browser/personal_data_manager.cc https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:420: if (IsFeatureSubstringMatchEnabled()) { On 2015/06/29 22:06:28, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > On 2015/06/28 00:43:03, Rouslan wrote: > > > Use PreviewSuggestion() here. > > > > Acknowledged. > > > > > > I'm getting some problem here, shall address it soon. > > I can help you resolve the issue if you describe it. For password forms, we autocomplets the suggestion as we type into username field. Considering this requirement workflow order should be: A. Set the input filed with suggestion text. username_element->setValue(username, true); B. Apply text selection based on suggestion and current input. if (set_selection) { if (IsFeatureSubstringMatchEnabled()) { size_t start = autofill::GetTextSelectionStart(username, current_username); username_element->setSelectionRange(start, username.length()); } else { username_element->setSelectionRange(current_username.length(), username.length()); } This requirement mandates us to pass an extra parameter |current_username| (old field contents) along with |username| (filed suggestion) and |username_element| (text field node) to utility function |PreviewSuggestion()|. I was trying if we can somehow skip doing so. Now I've changed prototype as below: void PreviewSuggestion(WebFormControlElement* input_element, const base::string16& suggestion, const base::string16& user_input); https://codereview.chromium.org/962673004/diff/860001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:803: base::string16 suggested_username = username; On 2015/06/29 22:06:28, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > On 2015/06/28 00:43:03, Rouslan wrote: > > > No need for these temporary variables on lines 802-803. Is there? > > > > Done. > > They're still here. I think you were planning to remove them but forgot. Done. Yes, I forgot to get rid of these. https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:91: std::vector<base::string16> preferred_suggestions; On 2015/06/29 17:06:45, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > We need to pass '%<delimiter> + prefix_lower + %'. But there is one problem: > > > > '_' being keyword, suggestions like "test_user" on user input "use" would not > > get populated. > > Use '\_' instead? > > > Moreover, groups i.e. [] does not seems working (tested on my > > local linux setup) with chromium. > > > Why do you need []? Do () work? > > > > return s.Succeeded() && s2.Succeeded(); > > > > > > > > > > did you mean "return s.Succeeded() || s2.Succeeded();" > > I don't have a strong preference, as long as you document the behavior in > function comment. ESCAPE clause worked for (_) case. SELECT value FROM table WHERE value LIKE '%!_foo%' ESCAPE '!' https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:20: const base::string16 kSplitCharacters = base::ASCIIToUTF16(" .,-_@"); On 2015/06/29 17:06:45, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > On 2015/06/28 01:29:39, Rouslan wrote: > > > Wouldn't this statement will require an exit-time distractor, prohibited in > > > Chrome? Use an array instead: > > > > > > static const char kSplitCharacters[] = " .,-_@"; > > > > Done. > > > > Do I need to set "-Wexit-time-destructors" explicitly? Because I didn't notice > > any warning on console. > > There's some bot that runs on your code once it's checked in and reverts your > patch if there's an exit time destructor. I don't recall the name of the bot, > but you don't want to encounter it :-). Done. https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor... components/autofill/core/common/autofill_util.h:13: // |kEnableSuggestionsWithSubstringMatch| is on; otherwise |false|. On 2015/06/29 22:06:28, Rouslan wrote: > On 2015/06/29 15:38:21, Pritam Nikam wrote: > > On 2015/06/28 01:29:39, Rouslan wrote: > > > > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Com... > > > > > > "It is not necessary to say 'returns false otherwise' because this is > > implied." > > > > Done. > > You've removed "; otherwise false" below, but you did not remove "; otherwise > |false|" above. Please remove that one too. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1538: DCHECK(input_element); On 2015/06/29 22:06:28, Rouslan wrote: > The rest of the code in this file does not DCHECK pointers before dereferencing > them. Let's be consistent and not DCHECK in your function either. No worries, > both DCHECK and dereference behave the same way (crash with a stack trace). Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1541: ? autofill::GetTextSelectionStart(suggestion, input_element->value()) On 2015/06/29 22:06:28, Rouslan wrote: > Either use |suggestion| or input_element->suggestedValue() throughout the > function, but not both. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text. On 2015/06/29 22:06:28, Rouslan wrote: > Please document that |input_element| should not be null. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: ::autofill::PreviewSuggestion(suggested_username, &username_element); On 2015/06/29 22:06:28, Rouslan wrote: > Why "::autofill::"? We are already inside |PasswordAutofillAgent::PreviewSuggestion()|. For compiler, it's confusing. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:93: for (base::string16 suggestion : suggestions) { On 2015/06/29 22:06:28, Rouslan wrote: > const base::string16& Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.cc:134: // future so that logic can be handled in SQL query instead. On 2015/06/29 22:06:28, Rouslan wrote: > No need for "Revisit.." sentence and TODO marker. Instead, please find the > sqlite documentation that explains why sqlite filtering will not work for our > case and paste the link in the comment. With ESCAPE clause its working. I've modified |AutofillTable::GetFormValuesForElementName()|. SELECT value FROM table WHERE value LIKE '%!_foo%' ESCAPE '!' https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autocomplete_history_manager.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autocomplete_history_manager.h:76: base::string16 pending_query_prefix_; On 2015/06/29 22:06:29, Rouslan wrote: > Let's call it "last_query_prefix" and put a newline afterwards. As we can move logic to SQL, removing it. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3076: // Test that suggestion tokens (substrings separated by characters from " On 2015/06/29 22:06:29, Rouslan wrote: > Please be more specific. For example, 'Verify that typing "gmail" will match > mailto:"theking@gmail.com" and mailto:"buddy@gmail.com" when substring matching is enabled.' Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3103: // .,-_@") are matched against field contents ignoring their case. On 2015/06/29 22:06:29, Rouslan wrote: > Please be more specific. For example, 'Verify that typing "apple" will match > "123 Apple St." when substring matching is enabled.' Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3127: // Test that suggestions which do not have a prefix match or prefix-token match On 2015/06/29 22:06:29, Rouslan wrote: > Please be more specific. For example, 'Verify that typing "mail" will not match > any of the mailto:"@gmail.com" email addresses when substring matching is enabled.' > > You get the idea. The same for the rest of the tests, too, please. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:799: // CanonicalizeProfileString() returns lower-case string with all On 2015/06/29 22:06:29, Rouslan wrote: > CanonicalizeProfileString() does not replace "all separators". Please correct > your comment. Done. I reffered it from here [1]. [1]. https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Rephrased it to: // CanonicalizeProfileString() returns lower-case string with separators; // includes space, line and paragraph along with punctuations like dash, // start, end and others {U_DASH_PUNCTUATION, U_START_PUNCTUATION, // U_END_PUNCTUATION, U_CONNECTOR_PUNCTUATION, U_OTHER_PUNCTUATION, // U_SPACE_SEPARATOR, U_LINE_SEPARATOR, U_PARAGRAPH_SEPARATOR}; substituted // by whitespaces and trims off trailing whitespace if any. Passing // |value_canon| and |field_contents_canon| to ContainsTokenThatStartsWith() // may lead to unexpected result, hence preferred passing |value| and // |field_contents| instead. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:803: // preferred passing |values[i]| and |field_contents| instead. On 2015/06/29 22:06:29, Rouslan wrote: > This part of the comment is outdated. There's no values[i] anywhere in this > file. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:807: ContainsTokenThatStartsWith(value, field_contents, false))) { On 2015/06/29 22:06:29, Rouslan wrote: > It's rare in Chromium code to assign a variable inside of an if clause. Is there > a simpler way to write this without such assignment? Done. Changes to: bool prefix_matched_suggestion = base::StartsWith(...); if (prefix_matched_suggestion || ContainsTokenThatStartsWith(...)) { ... suggestions.back().match = prefix_matched_suggestion ? Suggestion::PREFIX_MATCH : Suggestion::SUBSTRING_MATCH; } https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:822: }); On 2015/06/29 22:06:29, Rouslan wrote: > Is this clang formatted? Yes. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:876: std::list<const CreditCard*> substring_matched_suggestions; On 2015/06/29 22:06:29, Rouslan wrote: > s/substring_matched_suggestions/substring_matched_cards Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/suggestion.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/suggestion.h:23: }; On 2015/06/29 22:06:29, Rouslan wrote: > newline after }; Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:7: #include <vector> On 2015/06/29 22:06:29, Rouslan wrote: > Also <algorithm> for std::search. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:32: std::vector<base::string16> suggestion_tokens = On 2015/06/29 22:06:29, Rouslan wrote: > You can probably save one copy operation by using "const > std::vector<base::string16>&" here. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:37: // token. On 2015/06/29 22:06:29, Rouslan wrote: > s/token/tokens Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:48: size_t offset = 0; On 2015/06/29 22:06:29, Rouslan wrote: > You don't need |offset| variable. You can use "std::search(it, suggestion.end(), > ...)". Then you can return "it - suggestion.begin() + field_contents.size()". > Instead of "++offset", you can put "++it" in the third part of > "for(<1>;<2>;<3>)". Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:49: // Loop until we find either the |field_contents| begins with the |suggestion| On 2015/06/29 22:06:29, Rouslan wrote: > s/begins with/is a prefix of > > Right? Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:18: // on and |field_contents| is a prefix of some token. On 2015/06/29 22:06:29, Rouslan wrote: > "... of a token in |suggestion|." Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:23: // Finds if the |field_contents| prefixes any of the |suggestion| token, On 2015/06/29 22:06:29, Rouslan wrote: > s/token/tokens Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util.h:24: // separated by characters from " .,-_@". Returns the start position of the text On 2015/06/29 22:06:29, Rouslan wrote: > Remove "from". It's a bit confusing IMHO. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:24: EXPECT_TRUE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"), On 2015/06/29 22:06:29, Rouslan wrote: > Use an array of structs similar to the next test case, please. Done. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/common/autofill_util_unittest.cc:53: base::string16 field_suggestion; On 2015/06/29 22:06:29, Rouslan wrote: > Use "char*" instead of "base::string16". Don't base::ASCIIToUTF16() when > building the struct. Do that inside of the loop instead. Done. https://codereview.chromium.org/962673004/diff/920001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:73: (substring_matched_suggestion = autofill::ContainsTokenThatStartsWith( On 2015/06/29 22:06:29, Rouslan wrote: > If you can figure out a way to write this without an assignment inside of the > if-clause, that we great. :-D Done. Changes to: bool prefix_matched_suggestion = show_all || base::StartsWith(...); if (prefix_matched_suggestion || ContainsTokenThatStartsWith(...)) { ... suggestions.back().match = prefix_matched_suggestion ? Suggestion::PREFIX_MATCH : Suggestion::SUBSTRING_MATCH; } https://codereview.chromium.org/962673004/diff/920001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:359: // Test that suggestion tokens (substrings separated by characters from On 2015/06/29 22:06:29, Rouslan wrote: > Please be more specific in this comment and the comments below. For example, > this comment can be clarified by rewriting it as, 'Verify that typing "foo" into > the username field will match usernames mailto:"foo.bar@example.com", > mailto:"bar.foo@example.com", and mailto:"example@foo.com". Done.
Hi Pritam!
On 2015/06/29 17:19:09, Pritam Nikam wrote:
> Thanks Rouslan again for quick reply.
> Please find my in-line replies.
>
> Thanks!
>
> On 2015/06/29 17:06:45, Rouslan wrote:
> > On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > > On 2015/06/28 00:43:04, Rouslan wrote:
> > > > I found a bug in your patch: If my street address is "123 Main Street",
> then
> > > > "123 main" will correctly suggest my street address, but "main st" does
> not
> > > > match my street address. I don't know what's causing it, but I bet that
> some
> > > > more testing of autofill_util.cc will reveal the problem cause. Please
fix
> > it.
> > >
> > > I'd say its an expected behaviour.
> > >
> > > For substring match case, we just check whether the |field_contents|
> prefixes
> > > any of the |suggestion|'s token. And cases where |field_contents| span
> > multiple
> > > |suggestions| text tokens funtion |ContainsTokenThatStartsWith| simply
> return
> > > false.
> > >
> > > Unit test addressing the same:
> > >
> > > AutofillUtilTest.ContainsTokenThatStartsWith
> > > 38
EXPECT_FALSE(ContainsTokenThatStartsWith(base::ASCIIToUTF16("ab@cd.b"),
> > > 39 base::ASCIIToUTF16("cd.b"),
> > > false));
> > >
> > > &
> > >
> > > PasswordAutofillManagerTest.MatchingContentsWithSuggestionTokenSeparator
> >
> > I understand that it's your code's current behavior and it is well tested.
> > However, that's not how the code should behave, IMHO. Adding a test case to
> > verify incorrect behavior does not make the behavior correct :-). Was there
a
> > discussion about it that I missed?
> >
>
> Yes this behaviour was discussed in past (with Vaclav). You could find related
> conversation as well - not able to locate it now :(
> Earlier I had kept it like how you've suggested, i.e. user input can span
> multiple suggestion tokens.
I'm no longer remembering all the details, but looking freshly at the proposal,
prefix-matching from token boundaries sounds reasonable to me. In particular,
matching "Main Street" against "123 Main Street" looks good. What should not
match is "23 Main" or "in street".
Sorry if that is a change against our past conversations. Let me know if you'd
like to clarify more with me.
Cheers,
Vaclav
>
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
> > File components/autofill/content/renderer/autofill_agent.cc (right):
> >
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/con...
> > components/autofill/content/renderer/autofill_agent.cc:745: if
> > (IsFeatureSubstringMatchEnabled()) {
> > On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > > On 2015/06/28 00:43:03, Rouslan wrote:
> > > > You can inline this:
> > > >
> > > > node->setSelectionRange(
> > > > IsFeatureSubstringMatchEnabled()
> > > > ? autofill::GetTextSelectionStart(value, node->value())
> > > > : node->value().length(),
> > > > node->suggestedValue().length());
> > > >
> > > > You use this code in at least two places. It's a good candidate to be
> placed
> > > > into autofill_util.h. Please do that. You can call it
PreviewSuggestion()
> or
> > > > something better you can come up with.
> > >
> > > Sure, I'll move this code restructuring under |PreviewSuggestion()| API.
> > > However, moving it to |autofill_util.h| will be a problem, as
> > > |PreviewSuggestion()| will have blink dependency i.e. class
WebInputElement,
> > and
> > > it'll not fly. Instead I've moved it to |form_autofill_util.h| i.e. under
> > > content/renderer folder.
> >
> > Sounds good.
> >
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> > File components/autofill/core/browser/autocomplete_history_manager.cc
(right):
> >
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> > components/autofill/core/browser/autocomplete_history_manager.cc:91:
> > std::vector<base::string16> preferred_suggestions;
> > On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > > We need to pass '%<delimiter> + prefix_lower + %'. But there is one
problem:
> > >
> > > '_' being keyword, suggestions like "test_user" on user input "use" would
> not
> > > get populated.
> >
> > Use '\_' instead?
> >
> > > Moreover, groups i.e. [] does not seems working (tested on my
> > > local linux setup) with chromium.
> >
> >
> > Why do you need []? Do () work?
>
> '\_' doesn't seem working.
>
> I thought of using, pattern ('%[_]foo%') as well, but that didn't work.
>
> >
> > > > return s.Succeeded() && s2.Succeeded();
> > > >
> > > >
> > >
> > > did you mean "return s.Succeeded() || s2.Succeeded();"
> >
> > I don't have a strong preference, as long as you document the behavior in
> > function comment.
>
> Sure will do that.
>
> >
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> > File components/autofill/core/common/autofill_util.cc (right):
> >
> >
>
https://codereview.chromium.org/962673004/diff/860001/components/autofill/cor...
> > components/autofill/core/common/autofill_util.cc:20: const base::string16
> > kSplitCharacters = base::ASCIIToUTF16(" .,-_@");
> > On 2015/06/29 15:38:21, Pritam Nikam wrote:
> > > On 2015/06/28 01:29:39, Rouslan wrote:
> > > > Wouldn't this statement will require an exit-time distractor, prohibited
> in
> > > > Chrome? Use an array instead:
> > > >
> > > > static const char kSplitCharacters[] = " .,-_@";
> > >
> > > Done.
> > >
> > > Do I need to set "-Wexit-time-destructors" explicitly? Because I didn't
> notice
> > > any warning on console.
> >
> > There's some bot that runs on your code once it's checked in and reverts
your
> > patch if there's an exit time destructor. I don't recall the name of the
bot,
> > but you don't want to encounter it :-).
>
> Fair enough :-)
On 2015/06/30 15:06:27, vabr (Chromium) wrote: > I'm no longer remembering all the details, but looking freshly at the proposal, > prefix-matching from token boundaries sounds reasonable to me. In particular, > matching "Main Street" against "123 Main Street" looks good. What should not > match is "23 Main" or "in street". > > Sorry if that is a change against our past conversations. Let me know if you'd > like to clarify more with me. > > > Cheers, > Vaclav Pritam, Let's make "Main Street" match against "123 Main Street". I don't see any reason for this to not be the case. Imagine, for example, that a user has two addresses: "1234 Wide St" and "4567 Wide Ave". If the user types "Wide", both addresses will match. If the user types "Wide A", then your code will match nothing. That would be confusing, IMHO. From what I understand, only ContainsTokenThatStartsWith() and the corresponding tests need to change. Cheers, Rouslan https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:808: ::autofill::PreviewSuggestion(suggested_username, &username_element); On 2015/06/30 15:05:50, Pritam Nikam wrote: > On 2015/06/29 22:06:28, Rouslan wrote: > > Why "::autofill::"? > > We are already inside |PasswordAutofillAgent::PreviewSuggestion()|. For > compiler, it's confusing. Acknowledged. https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:807: ContainsTokenThatStartsWith(value, field_contents, false))) { On 2015/06/30 15:05:50, Pritam Nikam wrote: > On 2015/06/29 22:06:29, Rouslan wrote: > > It's rare in Chromium code to assign a variable inside of an if clause. Is > there > > a simpler way to write this without such assignment? > > Done. > > Changes to: > > bool prefix_matched_suggestion = base::StartsWith(...); > if (prefix_matched_suggestion || ContainsTokenThatStartsWith(...)) { > ... > suggestions.back().match = prefix_matched_suggestion > ? Suggestion::PREFIX_MATCH > : Suggestion::SUBSTRING_MATCH; > } Nice. https://codereview.chromium.org/962673004/diff/920001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/920001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:73: (substring_matched_suggestion = autofill::ContainsTokenThatStartsWith( On 2015/06/30 15:05:51, Pritam Nikam wrote: > On 2015/06/29 22:06:29, Rouslan wrote: > > If you can figure out a way to write this without an assignment inside of the > > if-clause, that we great. :-D > > Done. > > Changes to: > bool prefix_matched_suggestion = show_all || base::StartsWith(...); > if (prefix_matched_suggestion || ContainsTokenThatStartsWith(...)) { > ... > suggestions.back().match = prefix_matched_suggestion > ? Suggestion::PREFIX_MATCH > : Suggestion::SUBSTRING_MATCH; > } Very nice. https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... components/autofill/content/renderer/autofill_agent.cc:743: PreviewSuggestion(node, value, node->value()); Pass in node->suggestedValue() instead of "value", because the former is truncated. https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:875: PreviewSuggestion(field, data.value, field->value()); Pass in input_element->suggestedValue() instead of data.value, because the former is truncated. https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.cc:1543: suggestion.size()); Lines 1542-1543: We usually use length() for strings and size() for vectors. Let's use length() here. https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... components/autofill/content/renderer/form_autofill_util.h:181: void PreviewSuggestion(blink::WebFormControlElement* input_element, You're modifying |input_element|. Modified parameters usually go to the end of the parameter list. https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/con... components/autofill/content/renderer/password_autofill_agent.cc:797: ::autofill::PreviewSuggestion(&username_element, username, You should pass in username_element.suggestedValue() instead of "username" to be consistent with other places where I've suggested to pass in the former. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3091: // displayed in suggestion list. Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3118: // suggestion list. Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3144: // "buddy@gmail.com" has a token with a prefix "mail". Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3175: // list. Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3196: // token with a prefix "lvis". Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:3241: // Smith" placed before the substring matched suggestion "Adam Smith". Redundant comment. Delete. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/personal_data_manager.cc:807: // |field_contents| instead. I would prefer removing this comment entirely, to be honest with you. I think reading and understanding it will take as much time as reading and understanding the code in question. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:461: sql::Statement s2; I am beginning to think that it would be more correct to use && instead of ||. Like this: bool succeeded = false; if (prefix.empty()) { sql::Statement s; ... succeeded = s.Succeeded(); } else { sql::Statement s1; ... succeeded = s1.Succeeded(); if (IsFeatureSubstringMatchEnabled()) { sql::Statement s2; ... succeeded &= s2.Succeeded(); } } return succeeded; https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:510: 1, base::ASCIIToUTF16("% ") + prefix_lower + base::ASCIIToUTF16("%")); If "prefix_lower" contains "_", "%%", or "; DROP TABLE autofill;"... will your code break? Please add some tests for this. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:520: base::ASCIIToUTF16("%")); I am very happy that this works. Good job! Would your code be shorter and easier to understand if you changed lines 509-520 to something like this? std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower); s2.BindString(1, base::StringPrintf("%% %s%%", prefix_lower_utf8)); s2.BindString(2, base::StringPrintf("%%.%s%%", prefix_lower_utf8)); s2.BindString(3, base::StringPrintf("%%,%s%%", prefix_lower_utf8)); s2.BindString(4, base::StringPrintf("%%-%s%%", prefix_lower_utf8)); s2.BindString(5, base::StringPrintf("%%@%s%%", prefix_lower_utf8)); s2.BindString(6, base::StringPrintf("%%!_%s%%", prefix_lower_utf8)); https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/common/autofill_util.cc:59: base::ASCIIToUTF16(kSplitCharacters))) { It should be easier to dereference the pointer (*it) instead of taking a single character substring. base::string16 split_chars = base::ASCIIToUTF16(kSplitCharacters); ... if (it == suggestion.begin() || split_chars.find(*it) != std::string::npos)) { ... } https://codereview.chromium.org/962673004/diff/960001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:394: // "example@foo.com" are displayed. Redundant comment. Please delete. https://codereview.chromium.org/962673004/diff/960001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:441: // "bar.foo@example.com" or "example@foo.com" has a token with a prefix "oo". Redundant comment. Please delete. https://codereview.chromium.org/962673004/diff/960001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:450: // "bar.foo@example.com" as filed containts span accross multiple tokens. s/field contains/field contents https://codereview.chromium.org/962673004/diff/960001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:495: // "foo.bar@ex". That suggestion should be the only match. Redundant comment. Please delete. https://codereview.chromium.org/962673004/diff/960001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:544: // "bar.foo@example.com". Redundant comment. Please delete.
By the way, please fix a crash in AutofillAgent::OnPreviewPasswordSuggestion() after your patch is applied. [1:1:0630/134601:FATAL:autofill_util.cc(67)] Check failed: false. #0 0x7f85fe0cb04e base::debug::StackTrace::StackTrace() #1 0x7f85fe11c012 logging::LogMessage::~LogMessage() #2 0x7f860b8aa919 autofill::GetTextSelectionStart() #3 0x7f860acf7fdb autofill::PreviewSuggestion() #4 0x7f860ad0e504 autofill::PasswordAutofillAgent::PreviewSuggestion() #5 0x7f860acee2ba autofill::AutofillAgent::OnPreviewPasswordSuggestion() Steps to manually reproduce: 1) Go to https://form870.appspot.com/smoke.html. 2) For each of the "NP Fill Form (...)" buttons: 2a) Click on the "NP Fill Form (...)" button. 2b) Click on "Submit" button right above it. 2c) Make sure that password is saved. 2d) Go back to https://form870.appspot.com/smoke.html. 3) Double-click on the username field. 4) Hover over the other usernames in the autofill popup.
Heads-up: Conflicting changes coming in https://codereview.chromium.org/1220653002/.
On 2015/07/01 09:14:18, vabr (Chromium) wrote: > Heads-up: Conflicting changes coming in > https://codereview.chromium.org/1220653002/. Note that the above CL introduces new handling of case sensitivity, but keeps the old API, so please make sure that if you rebase, you don't revert to the old API by accident.
Thanks Vaclav and Rouslan for your inputs.
SQL LIKE clause does not go well with keywords in search string ("_", "%%"
etc...). May be we need to restore C++ code snippet in
|autocomplete_history_manager.cc|. Rest of the inputs addressed in new patch,
please have a look.
Thanks!
On 2015/06/30 19:06:24, Rouslan wrote:
> On 2015/06/30 15:06:27, vabr (Chromium) wrote:
> > I'm no longer remembering all the details, but looking freshly at the
> proposal,
> > prefix-matching from token boundaries sounds reasonable to me. In
particular,
> > matching "Main Street" against "123 Main Street" looks good. What should not
> > match is "23 Main" or "in street".
> >
> > Sorry if that is a change against our past conversations. Let me know if
you'd
> > like to clarify more with me.
> >
> >
> > Cheers,
> > Vaclav
>
> Pritam,
>
> Let's make "Main Street" match against "123 Main Street". I don't see any
reason
> for this to not be the case. Imagine, for example, that a user has two
> addresses: "1234 Wide St" and "4567 Wide Ave". If the user types "Wide", both
> addresses will match. If the user types "Wide A", then your code will match
> nothing. That would be confusing, IMHO. From what I understand, only
> ContainsTokenThatStartsWith() and the corresponding tests need to change.
>
> Cheers,
> Rouslan
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
File components/autofill/content/renderer/autofill_agent.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
components/autofill/content/renderer/autofill_agent.cc:743:
PreviewSuggestion(node, value, node->value());
On 2015/06/30 19:06:23, Rouslan wrote:
> Pass in node->suggestedValue() instead of "value", because the former is
> truncated.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:875:
PreviewSuggestion(field, data.value, field->value());
On 2015/06/30 19:06:23, Rouslan wrote:
> Pass in input_element->suggestedValue() instead of data.value, because the
> former is truncated.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.cc:1543:
suggestion.size());
On 2015/06/30 19:06:23, Rouslan wrote:
> Lines 1542-1543: We usually use length() for strings and size() for vectors.
> Let's use length() here.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
File components/autofill/content/renderer/form_autofill_util.h (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
components/autofill/content/renderer/form_autofill_util.h:181: void
PreviewSuggestion(blink::WebFormControlElement* input_element,
On 2015/06/30 19:06:23, Rouslan wrote:
> You're modifying |input_element|. Modified parameters usually go to the end of
> the parameter list.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:797:
::autofill::PreviewSuggestion(&username_element, username,
On 2015/06/30 19:06:23, Rouslan wrote:
> You should pass in username_element.suggestedValue() instead of "username" to
be
> consistent with other places where I've suggested to pass in the former.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager_unittest.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3091: // displayed
in suggestion list.
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3118: //
suggestion list.
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3144: //
"buddy@gmail.com" has a token with a prefix "mail".
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3175: // list.
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3196: // token
with a prefix "lvis".
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/autofill_manager_unittest.cc:3241: // Smith"
placed before the substring matched suggestion "Adam Smith".
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/browser/personal_data_manager.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/personal_data_manager.cc:807: //
|field_contents| instead.
On 2015/06/30 19:06:23, Rouslan wrote:
> I would prefer removing this comment entirely, to be honest with you. I think
> reading and understanding it will take as much time as reading and
understanding
> the code in question.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:461: sql::Statement
s2;
On 2015/06/30 19:06:23, Rouslan wrote:
> I am beginning to think that it would be more correct to use && instead of ||.
> Like this:
>
> bool succeeded = false;
>
> if (prefix.empty()) {
> sql::Statement s;
> ...
> succeeded = s.Succeeded();
> } else {
> sql::Statement s1;
> ...
> succeeded = s1.Succeeded();
>
> if (IsFeatureSubstringMatchEnabled()) {
> sql::Statement s2;
> ...
> succeeded &= s2.Succeeded();
> }
> }
>
> return succeeded;
In that case, if |succeeded = s1.Succeeded()| is false, irrespective of
|s2.Succeeded()|'s result |succeeded| will be always false.
IMO, || is appropreate here.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:510: 1,
base::ASCIIToUTF16("% ") + prefix_lower + base::ASCIIToUTF16("%"));
On 2015/06/30 19:06:23, Rouslan wrote:
> If "prefix_lower" contains "_", "%%", or "; DROP TABLE autofill;"... will your
> code break? Please add some tests for this.
Yeah, there seems many problems :(
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:520:
base::ASCIIToUTF16("%"));
On 2015/06/30 19:06:23, Rouslan wrote:
> I am very happy that this works. Good job! Would your code be shorter and
easier
> to understand if you changed lines 509-520 to something like this?
>
> std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower);
> s2.BindString(1, base::StringPrintf("%% %s%%", prefix_lower_utf8));
> s2.BindString(2, base::StringPrintf("%%.%s%%", prefix_lower_utf8));
> s2.BindString(3, base::StringPrintf("%%,%s%%", prefix_lower_utf8));
> s2.BindString(4, base::StringPrintf("%%-%s%%", prefix_lower_utf8));
> s2.BindString(5, base::StringPrintf("%%@%s%%", prefix_lower_utf8));
> s2.BindString(6, base::StringPrintf("%%!_%s%%", prefix_lower_utf8));
These doesn't work :(
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/common/autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/common/autofill_util.cc:59:
base::ASCIIToUTF16(kSplitCharacters))) {
On 2015/06/30 19:06:23, Rouslan wrote:
> It should be easier to dereference the pointer (*it) instead of taking a
single
> character substring.
>
> base::string16 split_chars = base::ASCIIToUTF16(kSplitCharacters);
> ...
> if (it == suggestion.begin() || split_chars.find(*it) != std::string::npos)) {
> ...
> }
Done.
Did you mean?
base::string16 split_chars = base::ASCIIToUTF16(kSplitCharacters);
if (it == suggestion.begin() || split_chars.find(*(it-1)) !=
std::string::npos)) {
...
}
*it -> Character where match found.
*(it-1) -> Character right before the match.
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
File
components/password_manager/core/browser/password_autofill_manager_unittest.cc
(right):
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:394:
// "example@foo.com" are displayed.
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Please delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:441:
// "bar.foo@example.com" or "example@foo.com" has a token with a prefix "oo".
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Please delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:450:
// "bar.foo@example.com" as filed containts span accross multiple tokens.
On 2015/06/30 19:06:23, Rouslan wrote:
> s/field contains/field contents
Done.
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:495:
// "foo.bar@ex". That suggestion should be the only match.
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Please delete.
Done.
https://codereview.chromium.org/962673004/diff/960001/components/password_man...
components/password_manager/core/browser/password_autofill_manager_unittest.cc:544:
// "bar.foo@example.com".
On 2015/06/30 19:06:23, Rouslan wrote:
> Redundant comment. Please delete.
Done.
On 2015/07/01 17:26:00, Pritam Nikam wrote:
> SQL LIKE clause does not go well with keywords in search string ("_", "%%"
> etc...). May be we need to restore C++ code snippet in
> |autocomplete_history_manager.cc|. Rest of the inputs addressed in new patch,
> please have a look.
Please take a look at my suggestion to deal with SQL injection when using LIKE
clauses.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:461: sql::Statement
s2;
On 2015/07/01 17:26:00, Pritam Nikam wrote:
> On 2015/06/30 19:06:23, Rouslan wrote:
> > I am beginning to think that it would be more correct to use && instead of
||.
> > Like this:
> >
> > bool succeeded = false;
> >
> > if (prefix.empty()) {
> > sql::Statement s;
> > ...
> > succeeded = s.Succeeded();
> > } else {
> > sql::Statement s1;
> > ...
> > succeeded = s1.Succeeded();
> >
> > if (IsFeatureSubstringMatchEnabled()) {
> > sql::Statement s2;
> > ...
> > succeeded &= s2.Succeeded();
> > }
> > }
> >
> > return succeeded;
>
> In that case, if |succeeded = s1.Succeeded()| is false, irrespective of
> |s2.Succeeded()|'s result |succeeded| will be always false.
>
> IMO, || is appropreate here.
Is there a case when s1.Succeeded() is false that does not indicate an error?
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:510: 1,
base::ASCIIToUTF16("% ") + prefix_lower + base::ASCIIToUTF16("%"));
On 2015/07/01 17:26:00, Pritam Nikam wrote:
> On 2015/06/30 19:06:23, Rouslan wrote:
> > If "prefix_lower" contains "_", "%%", or "; DROP TABLE autofill;"... will
your
> > code break? Please add some tests for this.
>
> Yeah, there seems many problems :(
>
The fix is not to move SQL injection mitigation into SubstringSubstituteText(),
but to do something like this instead:
-------------------------------------------------------------
s2.Assign(db->GetUniqueStatement(
"SELECT value FROM autofill "
"WHERE name = ? AND ("
" value LIKE '%' || ? || '%' OR "
" value LIKE '%' || ? || '%' OR "
" value LIKE '%' || ? || '%' OR "
" value LIKE '%' || ? || '%' OR "
" value LIKE '%' || ? || '%' OR "
" value LIKE '%' || ? || '%') "
" ORDER BY count DESC "
"LIMIT ?"));
s2.BindString16(0, name);
std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower);
s2.BindString(1, base::StringPrintf(" %s", prefix_lower_utf8));
s2.BindString(2, base::StringPrintf(".%s", prefix_lower_utf8));
s2.BindString(3, base::StringPrintf(",%s", prefix_lower_utf8));
s2.BindString(4, base::StringPrintf("-%s", prefix_lower_utf8));
s2.BindString(5, base::StringPrintf("@%s", prefix_lower_utf8));
s2.BindString(6, base::StringPrintf("_%s", prefix_lower_utf8));
s2.BindInt(7, limit);
-------------------------------------------------------------
BindString() will protect you from "_", "%", and "; DROP TABLE autofill;" in
"prefix_lower". If this does not work, I welcome you emailing me or finding me
on IRC to find a better way to achieve our goal here.
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
File components/autofill/core/common/autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor...
components/autofill/core/common/autofill_util.cc:59:
base::ASCIIToUTF16(kSplitCharacters))) {
On 2015/07/01 17:26:00, Pritam Nikam wrote:
> On 2015/06/30 19:06:23, Rouslan wrote:
> > It should be easier to dereference the pointer (*it) instead of taking a
> single
> > character substring.
> >
> > base::string16 split_chars = base::ASCIIToUTF16(kSplitCharacters);
> > ...
> > if (it == suggestion.begin() || split_chars.find(*it) != std::string::npos))
{
> > ...
> > }
>
> Done.
>
> Did you mean?
>
> base::string16 split_chars = base::ASCIIToUTF16(kSplitCharacters);
> if (it == suggestion.begin() || split_chars.find(*(it-1)) !=
> std::string::npos)) {
> ...
> }
>
> *it -> Character where match found.
> *(it-1) -> Character right before the match.
You're right.
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
File components/autofill/content/renderer/form_autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/content/renderer/form_autofill_util.cc:1541: ?
std::min(autofill::GetTextSelectionStart(suggestion, user_input),
GetTextSelectionStart() returns only one thing that's larger than
suggestion.length(): std::string::npos. If you've encountered this, it indicates
a bug. Please fix the bug in GetTextSelectionStart() instead of handling it here
with std::min().
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_table_unittest.cc
(right):
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table_unittest.cc:1867: const
size_t max_count = 2;
We usually format constants kLikeThis.
const size_t kMaxCount = 2;
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table_unittest.cc:1873: }
test_cases[] = {
kTestCases
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/browser/webdata/autofill_table_unittest.cc:1897: for
(size_t k = 0; k < arraysize(test_cases[i].field_suggestion); ++k) {
You can use kMaxCount instead of arraysize(...) here.
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
File components/autofill/core/common/autofill_util.cc (right):
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/common/autofill_util.cc:59:
Compare<base::string16::value_type>(case_sensitive))) !=
case_sensitive ? Compare<...>() : base::CaseInsensitiveCompare<...>()
(IMHO, base::CaseInsensitiveCompare is not doing the right thing, because it's
using to lower(), which works only for the current locale. For example, if your
locale is en-US, then Ё will not be changed to ё. Let's go with it for now..)
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/common/autofill_util.cc:62: static const base::string16
split_chars(
1) No need to make it static.
2) Constants are named kLikeThis (kSplitChars).
3) Put this outside of the loop.
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
File components/autofill/core/common/autofill_util.h (right):
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co...
components/autofill/core/common/autofill_util.h:28: bool case_sensitive =
false);
Google C++ style guide prohibits default parameters in most cases.
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu...
Thanks Rouslan for your inputs. I've addressed these in review comments, ptal. Thanks! https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:461: sql::Statement s2; On 2015/07/03 02:05:43, Rouslan wrote: > On 2015/07/01 17:26:00, Pritam Nikam wrote: > > On 2015/06/30 19:06:23, Rouslan wrote: > > > I am beginning to think that it would be more correct to use && instead of > ||. > > > Like this: > > > > > > bool succeeded = false; > > > > > > if (prefix.empty()) { > > > sql::Statement s; > > > ... > > > succeeded = s.Succeeded(); > > > } else { > > > sql::Statement s1; > > > ... > > > succeeded = s1.Succeeded(); > > > > > > if (IsFeatureSubstringMatchEnabled()) { > > > sql::Statement s2; > > > ... > > > succeeded &= s2.Succeeded(); > > > } > > > } > > > > > > return succeeded; > > > > In that case, if |succeeded = s1.Succeeded()| is false, irrespective of > > |s2.Succeeded()|'s result |succeeded| will be always false. > > > > IMO, || is appropreate here. > > Is there a case when s1.Succeeded() is false that does not indicate an error? Done. I didn't find so far. But logically, if |succeeded = s1.Succeeded();| execution results *false* then |s2.Succeeded()|'s execution results would be immaterial if we *AND* their results. https://codereview.chromium.org/962673004/diff/960001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_table.cc:510: 1, base::ASCIIToUTF16("% ") + prefix_lower + base::ASCIIToUTF16("%")); On 2015/07/03 02:05:43, Rouslan wrote: > On 2015/07/01 17:26:00, Pritam Nikam wrote: > > On 2015/06/30 19:06:23, Rouslan wrote: > > > If "prefix_lower" contains "_", "%%", or "; DROP TABLE autofill;"... will > your > > > code break? Please add some tests for this. > > > > Yeah, there seems many problems :( > > > > The fix is not to move SQL injection mitigation into SubstringSubstituteText(), > but to do something like this instead: > > ------------------------------------------------------------- > s2.Assign(db->GetUniqueStatement( > "SELECT value FROM autofill " > "WHERE name = ? AND (" > " value LIKE '%' || ? || '%' OR " > " value LIKE '%' || ? || '%' OR " > " value LIKE '%' || ? || '%' OR " > " value LIKE '%' || ? || '%' OR " > " value LIKE '%' || ? || '%' OR " > " value LIKE '%' || ? || '%') " > " ORDER BY count DESC " > "LIMIT ?")); This SQL query does not work on my chromium Linux build. I can see it treats (_) & (%) as SQL keywords. Furthermore, as per SQLite documentation (from here [1]) for LIKE operator to handle literal (_), (%) or ESCAPE character as recommended way is to use ESCAPE clause with ESCAPE character to prepend these. > s2.BindString16(0, name); > std::string prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower); > s2.BindString(1, base::StringPrintf(" %s", prefix_lower_utf8)); > s2.BindString(2, base::StringPrintf(".%s", prefix_lower_utf8)); > s2.BindString(3, base::StringPrintf(",%s", prefix_lower_utf8)); > s2.BindString(4, base::StringPrintf("-%s", prefix_lower_utf8)); > s2.BindString(5, base::StringPrintf("@%s", prefix_lower_utf8)); > s2.BindString(6, base::StringPrintf("_%s", prefix_lower_utf8)); > s2.BindInt(7, limit); > ------------------------------------------------------------- const char* const prefix_lower_utf8 = base::UTF16ToUTF8(prefix_lower).c_str(); s2.BindString(1, base::StringPrintf(" %s", prefix_lower_utf8)); We need string16 version i.e. s2.BindString16() API instead. I didn't find string formatting API for |wchar_t| on all platforms. Presently its just for windows platform. Please refer [2]. > > BindString() will protect you from "_", "%", and "; DROP TABLE autofill;" in > "prefix_lower". If this does not work, I welcome you emailing me or finding me > on IRC to find a better way to achieve our goal here. Sent you details in mail. References: [1]. https://www.sqlite.org/lang_expr.html#like [2]. https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:1541: ? std::min(autofill::GetTextSelectionStart(suggestion, user_input), On 2015/07/03 02:05:43, Rouslan wrote: > GetTextSelectionStart() returns only one thing that's larger than > suggestion.length(): std::string::npos. If you've encountered this, it indicates > a bug. Please fix the bug in GetTextSelectionStart() instead of handling it here > with std::min(). For password forms we autocomplete field_contents as user punches in text with the suggestion text. Same goes with password form load completes, username and password fields get autocomplete by MFU suggestion. In suggestions hover case, |GetTextSelectionStart()| returns |std::string::npos| when |field_contents| varies from |field_suggestion| text. something like below: On page load: username_element->setValue(...); username_element->setSuggestedValue(...); input_element->setSelectionRange(suggestedValue, value); and for suggestion hover: username_element->setSuggestedValue(...); input_element->setSelectionRange(value.length(), suggestedValue.length()); i.e. older |value| compared to new |suggestedValue| and that may not be matching (substring/prefix). In such cases |setSelectionRange()| with first parameter |std::string::npos| always set it to "0". If we keep a maximum value cap to |suggestedValue.length()| then it will position cursor it to end of the suggestion text. If we change |GetTextSelectionStart()| to return different value than |std::string::npos|, we may not be able to identify non-matching case. For IsFeatureSubstringMatchEnabled() off case, say if new |suggestedValue.length()| is greater than that of older one, on hover show text selection from |older_suggestedValue.length() - new_suggestedValue.length()|. Hope I gave proper explanation. Please let me know if it sounds confusion. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_unittest.cc:1867: const size_t max_count = 2; On 2015/07/03 02:05:43, Rouslan wrote: > We usually format constants kLikeThis. > > const size_t kMaxCount = 2; Done. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_unittest.cc:1873: } test_cases[] = { On 2015/07/03 02:05:43, Rouslan wrote: > kTestCases Done. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_unittest.cc:1897: for (size_t k = 0; k < arraysize(test_cases[i].field_suggestion); ++k) { On 2015/07/03 02:05:43, Rouslan wrote: > You can use kMaxCount instead of arraysize(...) here. Done. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/common/autofill_util.cc:59: Compare<base::string16::value_type>(case_sensitive))) != On 2015/07/03 02:05:43, Rouslan wrote: > case_sensitive ? Compare<...>() : base::CaseInsensitiveCompare<...>() > > (IMHO, base::CaseInsensitiveCompare is not doing the right thing, because it's > using to lower(), which works only for the current locale. For example, if your > locale is en-US, then Ё will not be changed to ё. Let's go with it for now..) This will not compile. In that case, how about: template <typename Char> struct Compare : public base::CaseInsensitiveCompare<Char> { ... bool operator()(Char x, Char y) const { return case_sensitive_ ? (x == y) : base::CaseInsensitiveCompare<Char>:: operator()(x, y); } } https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/common/autofill_util.cc:62: static const base::string16 split_chars( On 2015/07/03 02:05:43, Rouslan wrote: > 1) No need to make it static. > 2) Constants are named kLikeThis (kSplitChars). > 3) Put this outside of the loop. Done. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/common/autofill_util.h:28: bool case_sensitive = false); On 2015/07/03 02:05:43, Rouslan wrote: > Google C++ style guide prohibits default parameters in most cases. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Default_Argu... Done.
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:1541: ? std::min(autofill::GetTextSelectionStart(suggestion, user_input), On 2015/07/03 16:21:27, Pritam Nikam wrote: > On 2015/07/03 02:05:43, Rouslan wrote: > > GetTextSelectionStart() returns only one thing that's larger than > > suggestion.length(): std::string::npos. If you've encountered this, it > indicates > > a bug. Please fix the bug in GetTextSelectionStart() instead of handling it > here > > with std::min(). > > For password forms we autocomplete field_contents as user punches in text with > the suggestion text. Same goes with password form load completes, username and > password fields get autocomplete by MFU suggestion. > > In suggestions hover case, |GetTextSelectionStart()| returns |std::string::npos| > when |field_contents| varies from |field_suggestion| text. > > something like below: > > On page load: > username_element->setValue(...); > username_element->setSuggestedValue(...); > input_element->setSelectionRange(suggestedValue, value); > > > and for suggestion hover: > username_element->setSuggestedValue(...); > input_element->setSelectionRange(value.length(), suggestedValue.length()); > > i.e. older |value| compared to new |suggestedValue| and that may not be matching > (substring/prefix). > > In such cases |setSelectionRange()| with first parameter |std::string::npos| > always set it to "0". If we keep a maximum value cap to > |suggestedValue.length()| then it will position cursor it to end of the > suggestion text. > > If we change |GetTextSelectionStart()| to return different value than > |std::string::npos|, we may not be able to identify non-matching case. > > For IsFeatureSubstringMatchEnabled() off case, say if new > |suggestedValue.length()| is greater than that of older one, on hover show text > selection from |older_suggestedValue.length() - new_suggestedValue.length()|. > > Hope I gave proper explanation. Please let me know if it sounds confusion. Now it makes sense. Please put a sentence about this use case in the function comment for GetTextSelectionStart(). However, this code now changes the behavior of preview-on-hover feature. Currently it selects the whole text from 0 to suggestion.length(). Your code is changing the behavior to select from suggestion.length() to suggestion.length(). Do something like this instead please: ------------------------------------------------------------------- size_t selection_start = IsFeatureSubstringMatchEnabled() ? autofill::GetTextSelectionStart(suggestion, user_input) : user_input.length(); input_element->setSelectionRange( selection_start == std::string16::npos ? 0 : user_input.length(), suggestion.length()); ------------------------------------------------------------------- This code is a little bit confusing, however. It would be great if you could come up with something better. https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/core/common/autofill_util.cc:59: Compare<base::string16::value_type>(case_sensitive))) != On 2015/07/03 16:21:27, Pritam Nikam wrote: > On 2015/07/03 02:05:43, Rouslan wrote: > > case_sensitive ? Compare<...>() : base::CaseInsensitiveCompare<...>() > > > > (IMHO, base::CaseInsensitiveCompare is not doing the right thing, because it's > > using to lower(), which works only for the current locale. For example, if > your > > locale is en-US, then Ё will not be changed to ё. Let's go with it for now..) > > This will not compile. > > In that case, how about: > > template <typename Char> > struct Compare : public base::CaseInsensitiveCompare<Char> { > ... > bool operator()(Char x, Char y) const { > return case_sensitive_ ? (x == y) : base::CaseInsensitiveCompare<Char>:: > operator()(x, y); > } > } Looks good.
Thanks Rouslan for review. PTAL. Thanks! https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:1541: ? std::min(autofill::GetTextSelectionStart(suggestion, user_input), On 2015/07/03 22:05:47, Rouslan wrote: > On 2015/07/03 16:21:27, Pritam Nikam wrote: > > On 2015/07/03 02:05:43, Rouslan wrote: > > > GetTextSelectionStart() returns only one thing that's larger than > > > suggestion.length(): std::string::npos. If you've encountered this, it > > indicates > > > a bug. Please fix the bug in GetTextSelectionStart() instead of handling it > > here > > > with std::min(). > > > > For password forms we autocomplete field_contents as user punches in text with > > the suggestion text. Same goes with password form load completes, username and > > password fields get autocomplete by MFU suggestion. > > > > In suggestions hover case, |GetTextSelectionStart()| returns > |std::string::npos| > > when |field_contents| varies from |field_suggestion| text. > > > > something like below: > > > > On page load: > > username_element->setValue(...); > > username_element->setSuggestedValue(...); > > input_element->setSelectionRange(suggestedValue, value); > > > > > > and for suggestion hover: > > username_element->setSuggestedValue(...); > > input_element->setSelectionRange(value.length(), suggestedValue.length()); > > > > i.e. older |value| compared to new |suggestedValue| and that may not be > matching > > (substring/prefix). > > > > In such cases |setSelectionRange()| with first parameter |std::string::npos| > > always set it to "0". If we keep a maximum value cap to > > |suggestedValue.length()| then it will position cursor it to end of the > > suggestion text. > > > > If we change |GetTextSelectionStart()| to return different value than > > |std::string::npos|, we may not be able to identify non-matching case. > > > > For IsFeatureSubstringMatchEnabled() off case, say if new > > |suggestedValue.length()| is greater than that of older one, on hover show > text > > selection from |older_suggestedValue.length() - new_suggestedValue.length()|. > > > > Hope I gave proper explanation. Please let me know if it sounds confusion. > > Now it makes sense. Please put a sentence about this use case in the function > comment for GetTextSelectionStart(). > > However, this code now changes the behavior of preview-on-hover feature. > Currently it selects the whole text from 0 to suggestion.length(). Your code is > changing the behavior to select from suggestion.length() to suggestion.length(). > Do something like this instead please: > > > ------------------------------------------------------------------- > size_t selection_start = IsFeatureSubstringMatchEnabled() > ? autofill::GetTextSelectionStart(suggestion, user_input) > : user_input.length(); > input_element->setSelectionRange( > selection_start == std::string16::npos ? 0 : user_input.length(), > suggestion.length()); > ------------------------------------------------------------------- > > > This code is a little bit confusing, however. It would be great if you could > come up with something better. Done. Changed to: -- [snap] -- size_t selection_start = user_input.length(); if (IsFeatureSubstringMatchEnabled()) { size_t offset = autofill::GetTextSelectionStart(suggestion, user_input); selection_start = (offset == base::string16::npos) ? 0 : offset; } input_element->setSelectionRange(selection_start, suggestion.length()); -- [snap] --
Looking good! Remaining suggestions are mostly for code style and comments. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:1542: selection_start = (offset == base::string16::npos) ? 0 : offset; Please add a comment when zero selection start is appropriate. For example: // Zero selection start is for password manager, which can show // usernames that do not begin with the user input value. (Or anything better you can come up with.) https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:180: // |input_element| should not be null. Please add to the comment the reason why you don't use ->value() and ->suggestedValue() directly. For example: // Not using input_element->value() and input_element->suggestedValue() // directly because password manager sometimes does not set them. It's a good idea to file a bug against password manager to always set suggested value and link to it here. For example. // http://crbug.com/XYZ https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:385: for (base::string16::const_iterator it = escape_wildcards.begin(); Please use a c++11 loop, like this: for (base::char16 c : escape_wildcards) { ... } https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:391: } Lines 387-391 are a for loop in disguise. Let's use an actual for loop instead: for (size_t pos = 0; (pos != result.find(c, pos)) != base::string::16::npos; pos += 2) { result.insert(result.begin() + pos, escaper); } https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:536: " value LIKE '%!_' || :prefix || '%'ESCAPE '!' ) " Please add a space between '%' and ESCAPE. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:541: s2.BindString16(1, Substitute(prefix_lower, ASCIIToUTF16("_%"), L'!')); Please use 0x21, which is a base::char16, instead of L'!", which is a wchar_t. base::char16 is wchar_t only on some platforms. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:544: // Append substring matched suggestions. It's clear what's going on without this comment. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_unittest.cc:1862: TEST_F(AutofillTableTest, GetFormValuesForElementName) { Please use a test name that mentions the substring matching feature. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:24: Compare(bool case_sensitive) : case_sensitive_(case_sensitive) {} 1) Mark this constructor "explicit", please. This will prevent automatic conversion of a bool into a Compare object. 2) Please add a destructor with "override" keyword. Legend has it, that you will leak memory otherwise. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:26: return case_sensitive_ ? (x == y) : (tolower(x) == tolower(y)); You forgot to change tolower() to base::CaseInsensitiveCompare<Char>::operator(x, y). https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:35: static const char kSplitCharacters[] = " .,-_@"; Put this into unnamed namespace please. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.h:32: size_t GetTextSelectionStart(const base::string16& suggestion, Let's not overload functions. This opens a can of warms. That is, please delete this function. Use the above function everywhere. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Ove... https://codereview.chromium.org/962673004/diff/1090001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:75: show_all || base::StartsWith(lower_suggestion, lower_contents, true); The last parameter to base::StartsWith() is enum, not boolean. (This has recently changed.)
Thanks again Rouslan. I've addressed your review inputs in new patch-set (#51) PTAL. Thanks! https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.cc:1542: selection_start = (offset == base::string16::npos) ? 0 : offset; On 2015/07/06 20:15:17, Rouslan wrote: > Please add a comment when zero selection start is appropriate. For example: > > // Zero selection start is for password manager, which can show > // usernames that do not begin with the user input value. > > (Or anything better you can come up with.) Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:180: // |input_element| should not be null. On 2015/07/06 20:15:17, Rouslan wrote: > Please add to the comment the reason why you don't use ->value() and > ->suggestedValue() directly. For example: > > // Not using input_element->value() and input_element->suggestedValue() > // directly because password manager sometimes does not set them. > > It's a good idea to file a bug against password manager to always set suggested > value and link to it here. For example. > > // http://crbug.com/XYZ Done. Issue logged: http://crbug.com/507714 https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:385: for (base::string16::const_iterator it = escape_wildcards.begin(); On 2015/07/06 20:15:17, Rouslan wrote: > Please use a c++11 loop, like this: > > for (base::char16 c : escape_wildcards) { > ... > } Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:391: } On 2015/07/06 20:15:17, Rouslan wrote: > Lines 387-391 are a for loop in disguise. Let's use an actual for loop instead: > > for (size_t pos = 0; > (pos != result.find(c, pos)) != base::string::16::npos; > pos += 2) { > result.insert(result.begin() + pos, escaper); > } Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:536: " value LIKE '%!_' || :prefix || '%'ESCAPE '!' ) " On 2015/07/06 20:15:17, Rouslan wrote: > Please add a space between '%' and ESCAPE. Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:541: s2.BindString16(1, Substitute(prefix_lower, ASCIIToUTF16("_%"), L'!')); On 2015/07/06 20:15:17, Rouslan wrote: > Please use 0x21, which is a base::char16, instead of L'!", which is a wchar_t. > base::char16 is wchar_t only on some platforms. Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table.cc:544: // Append substring matched suggestions. On 2015/07/06 20:15:17, Rouslan wrote: > It's clear what's going on without this comment. Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/browser/webdata/autofill_table_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/browser/webdata/autofill_table_unittest.cc:1862: TEST_F(AutofillTableTest, GetFormValuesForElementName) { On 2015/07/06 20:15:18, Rouslan wrote: > Please use a test name that mentions the substring matching feature. Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:24: Compare(bool case_sensitive) : case_sensitive_(case_sensitive) {} On 2015/07/06 20:15:18, Rouslan wrote: > 1) Mark this constructor "explicit", please. This will prevent automatic > conversion of a bool into a Compare object. Done. > > 2) Please add a destructor with "override" keyword. Legend has it, that you will > leak memory otherwise. "override" goes well with virtual functions / dtor. Here, however the |CaseInsensitiveCompare| does not content virtual destructor. Furthermore, as we are not creating any base class pointer here memory leak shall not happen. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:26: return case_sensitive_ ? (x == y) : (tolower(x) == tolower(y)); On 2015/07/06 20:15:18, Rouslan wrote: > You forgot to change tolower() to > base::CaseInsensitiveCompare<Char>::operator(x, y). Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.cc:35: static const char kSplitCharacters[] = " .,-_@"; On 2015/07/06 20:15:18, Rouslan wrote: > Put this into unnamed namespace please. Done. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/core/common/autofill_util.h:32: size_t GetTextSelectionStart(const base::string16& suggestion, On 2015/07/06 20:15:18, Rouslan wrote: > Let's not overload functions. This opens a can of warms. That is, please delete > this function. Use the above function everywhere. > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Ove... Done. https://codereview.chromium.org/962673004/diff/1090001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/962673004/diff/1090001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:75: show_all || base::StartsWith(lower_suggestion, lower_contents, true); On 2015/07/06 20:15:18, Rouslan wrote: > The last parameter to base::StartsWith() is enum, not boolean. (This has > recently changed.) Done.
autofill lgtm % nit. Evan, this looks good to me now. Can you approve it as an OWNER? https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1090001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:180: // |input_element| should not be null. On 2015/07/07 16:18:31, Pritam Nikam wrote: > On 2015/07/06 20:15:17, Rouslan wrote: > > Please add to the comment the reason why you don't use ->value() and > > ->suggestedValue() directly. For example: > > > > // Not using input_element->value() and input_element->suggestedValue() > > // directly because password manager sometimes does not set them. > > > > It's a good idea to file a bug against password manager to always set > suggested > > value and link to it here. For example. > > > > // http://crbug.com/XYZ > > Done. > > Issue logged: http://crbug.com/507714 Acknowledged. https://codereview.chromium.org/962673004/diff/1110001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1110001/components/autofill/co... components/autofill/core/common/autofill_util.cc:29: operator()(x, y); nit: is this clang formatted?
lgtm for OWNERS
Thanks Rouslan and Evan. Hi Ilya, Please review changes in: "tools/metrics/histograms/histograms.xml" Thanks! https://codereview.chromium.org/962673004/diff/1110001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1110001/components/autofill/co... components/autofill/core/common/autofill_util.cc:29: operator()(x, y); On 2015/07/07 18:09:12, Rouslan wrote: > nit: is this clang formatted? Yes, Its a clang formatted.
histograms.xml lgtm
Hi Vaclav, I've modified "components/autofill/content/renderer/password_autofill_agent.cc & .h" - Removed |username_selection_start_| as its not being usefed for "enable-suggestions-with-substring-match" feature. - Instead record username |username_query_prefix_| typed in before suggestions previewed. - Use it to set the username input element's text selection range calculation. Please have a look. Thanks!
Thanks, Pritam Nikam. I left some comments. Also, should the changes in the PasswordAutofillAgent be tested? If the behaviour stays the same, could you please make sure that it is currently tested? Cheers, Vaclav https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:182: // directly because password manager sometimes does not set them. This looks very suspiciously. What does "sometimes" mean? I do not like the idea that there is perhaps a bug in how PasswordAutofillAgent (not password manager) handles value setting, but we just paper it over. Could we please clarify this? https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:230: return base::StartsWith(username1, username2, base::CompareCase::SENSITIVE) || I understand that the StartsWith call is there in case when IsFeatureSubstringMatchEnabled returns false (because otherwise the test is duplicated in ContainsTokenThatStartsWith). I'm not happy about the duplication, and about the need to think about removing it once substring matching becomes 100% launched. So I suggest to change ContainsTokenThatStartsWith to try matching from the begin every time, and from other token boundaries depending on IsFeatureSubstringMatchEnabled. Then you can delete StartsWith here completely. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:290: // Records the username typed before preview the suggestions. nit: preview the suggestions -> suggestions preview https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.cc:60: (it = std::search( nit: Modifying iterator in the exit condition looks like a misuse of a for-cycle, and harms readability. Writing for (A; B; C), the iterator should only be modified in A and C. You can also use the while cycle, if you want to keep just one instance of the std::search call. I'd suggest something like this: it = suggestion.begin(); do { it = std::search(...); if (/*match found*/) { return ...; } } while (it != suggestion.end()); https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.h:16: // Returns true if the |field_contents| prefixes any of the |suggestion| tokens The comment is not correct. This is not about prefixing tokens, this is about substrings starting at token boundaries. To illustrate for a suggestion "a.b.c" and field contents "b.c", the function returns true, but according to the comment it should return false, because "b.c" is not a prefix of any of the 3 tokens "a", "b", and "c". Please correct the comment (also for the method below). Subsequently the name of the method is also incorrect. What about FieldIsSuggestionSubstringStartingOnTokenBoundary? It is long, but it is correct. Feel free to come up with different names, but make sure they are correct. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.h:23: // separated by characters " .,-_@". Returns the start position of the text nit: Please clarify: Returns the start position -> Returns what should be the start position https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:30: {"ab@cd.b", "a", false, true}, You don't include cases spanning more tokens and still with expected_result true. Stealing from your other test, please add, e.g., {"ab@cd.b", "ab@c", false,, true} {"ba.a.ab", "a.a", false, true}
Some updates. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| and |user_input|. nit: This comment is hard to understand. It is not clear what "previews" mean, it is not clear that |user_input| is assumed to be a substring (or prefix, since it only highlights the last part of |suggestion| not included in |user_input|?) of |suggestion|, and that |suggestion| is assumed to be already visible in the field to the user. Could you please improve the comment to clearly say both (A) what the function does, and (B) what exactly are the expectations about the arguments? https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:182: // directly because password manager sometimes does not set them. On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > This looks very suspiciously. What does "sometimes" mean? I do not like the idea > that there is perhaps a bug in how PasswordAutofillAgent (not password manager) > handles value setting, but we just paper it over. Could we please clarify this? This has been resolved in http://crbug.com/507714, but please see the new comments. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:423: username_element->setValue(username, true); Please add: // TODO(vabr): Why not setSuggestedValue? http://crbug.com/507714
Thanks Vaclav for review. I've addressed review inputs along patch set #56, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| and |user_input|. On 2015/07/09 08:27:49, vabr (away until 13 July) wrote: > nit: This comment is hard to understand. It is not clear what "previews" mean, > it is not clear that |user_input| is assumed to be a substring (or prefix, since > it only highlights the last part of |suggestion| not included in |user_input|?) > of |suggestion|, and that |suggestion| is assumed to be already visible in the > field to the user. Could you please improve the comment to clearly say both (A) > what the function does, and (B) what exactly are the expectations about the > arguments? Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:182: // directly because password manager sometimes does not set them. On 2015/07/09 08:27:49, vabr (away until 13 July) wrote: > On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > > This looks very suspiciously. What does "sometimes" mean? I do not like the > idea > > that there is perhaps a bug in how PasswordAutofillAgent (not password > manager) > > handles value setting, but we just paper it over. Could we please clarify > this? > > This has been resolved in http://crbug.com/507714, but please see the new > comments. Acknowledged. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:230: return base::StartsWith(username1, username2, base::CompareCase::SENSITIVE) || On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > I understand that the StartsWith call is there in case when > IsFeatureSubstringMatchEnabled returns false (because otherwise the test is > duplicated in ContainsTokenThatStartsWith). I'm not happy about the duplication, > and about the need to think about removing it once substring matching becomes > 100% launched. So I suggest to change ContainsTokenThatStartsWith to try > matching from the begin every time, and from other token boundaries depending on > IsFeatureSubstringMatchEnabled. Then you can delete StartsWith here completely. For autofill & CC suggestions we are using base::StartsWith(...) and ContainsTokenThatStartsWith(...) to identify suggestions with PREFIX or SUBSTRING matched so that we can order them. IMO, if we move base::StartsWith(...) to ContainsTokenThatStartsWith(...) it would be difficult to do so by just making a single call to ContainsTokenThatStartsWith(...). To keep the API usage consistent across, let this be as it is. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:423: username_element->setValue(username, true); On 2015/07/09 08:27:50, vabr (away until 13 July) wrote: > Please add: > // TODO(vabr): Why not setSuggestedValue? http://crbug.com/507714 Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.h:290: // Records the username typed before preview the suggestions. On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > nit: preview the suggestions -> suggestions preview Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.cc:60: (it = std::search( On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > nit: Modifying iterator in the exit condition looks like a misuse of a > for-cycle, and harms readability. Writing for (A; B; C), the iterator should > only be modified in A and C. You can also use the while cycle, if you want to > keep just one instance of the std::search call. I'd suggest something like this: > > it = suggestion.begin(); > do { > it = std::search(...); > if (/*match found*/) { > return ...; > } > } while (it != suggestion.end()); Done. This was changed to for(...) loop, originally was in while(...). https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... But using | do { ... } while(...); | clause we have to write addtional exit case as std::search(...) can return suggestion.end(). https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.h:16: // Returns true if the |field_contents| prefixes any of the |suggestion| tokens On 2015/07/09 06:17:52, vabr (away until 13 July) wrote: > The comment is not correct. This is not about prefixing tokens, this is about > substrings starting at token boundaries. To illustrate for a suggestion "a.b.c" > and field contents "b.c", the function returns true, but according to the > comment it should return false, because "b.c" is not a prefix of any of the 3 > tokens "a", "b", and "c". > Please correct the comment (also for the method below). > > Subsequently the name of the method is also incorrect. What about > FieldIsSuggestionSubstringStartingOnTokenBoundary? It is long, but it is > correct. Feel free to come up with different names, but make sure they are > correct. Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.h:23: // separated by characters " .,-_@". Returns the start position of the text On 2015/07/09 06:17:52, vabr (away until 13 July) wrote: > nit: Please clarify: > Returns the start position -> Returns what should be the start position Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:30: {"ab@cd.b", "a", false, true}, On 2015/07/09 06:17:52, vabr (away until 13 July) wrote: > You don't include cases spanning more tokens and still with expected_result > true. Stealing from your other test, please add, e.g., > mailto:{"ab@cd.b", "ab@c", false,, true} > {"ba.a.ab", "a.a", false, true} Done. I've already added 2 test-cases: (line: 38) {"ab@cd.b", "cd.b", true, true}, (line: 39) {"ab@cd.b", "b@cd", false, false} I'll include these 2 as well.
Patchset #56 (id:1210001) has been deleted
The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, danakj@chromium.org, rouslan@chromium.org, estade@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/962673004/#ps1230001 (title: "Incorporated Vaclav's review comments.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
Here is the test failure, by the way: [ RUN ] AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens ../../components/autofill/core/browser/autofill_manager_unittest.cc:543: Failure Value of: suggestions_[i].label Actual: Visa ⋯3456 Expected: expected_suggestions[i].label Which is: *3456 Google Test trace: ../../components/autofill/core/browser/autofill_manager_unittest.cc:541: i: 0 [ FAILED ] AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens (18 ms) You should be able to reproduce this via `ninja -Cout/Release -l10 -j10 components_unittests && out/Release/components_unittests --gtest_filter="AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens"`.
On 2015/07/10 20:31:29, Rouslan wrote: > Here is the test failure, by the way: > > [ RUN ] AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens > ../../components/autofill/core/browser/autofill_manager_unittest.cc:543: Failure > Value of: suggestions_[i].label > Actual: Visa ⋯3456 > Expected: expected_suggestions[i].label > Which is: *3456 > Google Test trace: > > ../../components/autofill/core/browser/autofill_manager_unittest.cc:541: i: 0 > [ FAILED ] AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens > (18 ms) > > > You should be able to reproduce this via `ninja -Cout/Release -l10 -j10 > components_unittests && out/Release/components_unittests > --gtest_filter="AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens"`. Thanks Rouslan.
The CQ bit was checked by pritam.nikam@samsung.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, estade@chromium.org, danakj@chromium.org, vabr@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/962673004/#ps1250001 (title: "Fixed unittest for Android bot.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1250001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) (exceeded global retry quota)
Hi Pritam Nikam, I left some more comments. Cheers, Vaclav https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:230: return base::StartsWith(username1, username2, base::CompareCase::SENSITIVE) || On 2015/07/10 16:58:45, Pritam Nikam wrote: > On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > > I understand that the StartsWith call is there in case when > > IsFeatureSubstringMatchEnabled returns false (because otherwise the test is > > duplicated in ContainsTokenThatStartsWith). I'm not happy about the > duplication, > > and about the need to think about removing it once substring matching becomes > > 100% launched. So I suggest to change ContainsTokenThatStartsWith to try > > matching from the begin every time, and from other token boundaries depending > on > > IsFeatureSubstringMatchEnabled. Then you can delete StartsWith here > completely. > > For autofill & CC suggestions we are using base::StartsWith(...) and > ContainsTokenThatStartsWith(...) to identify suggestions with PREFIX or > SUBSTRING matched so that we can order them. IMO, if we move > base::StartsWith(...) to ContainsTokenThatStartsWith(...) it would be difficult > to do so by just making a single call to ContainsTokenThatStartsWith(...). To > keep the API usage consistent across, let this be as it is. I don't understand what you mean. FieldIsSuggestionSubstringStartingOnTokenBoundary already does prefix matching, as a particular case of the substring matching. You can still call StartsWith elsewhere first to check if there is a prefix match, as you do now. Here there is no need to distinguish prefix and non-prefix matches (no ranking based on it), so there is no need to call StartsWith. Except for FieldIsSuggestionSubstringStartingOnTokenBoundary no doing any matching when substring is not allowed. I don't see how this is about consistency in API usage. Consistent API usage does not mean that everybody using the API will compute the same thing. I don't agree with letting this be as it is for the "consistency" argument. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.cc:60: (it = std::search( On 2015/07/10 16:58:46, Pritam Nikam wrote: > On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > > nit: Modifying iterator in the exit condition looks like a misuse of a > > for-cycle, and harms readability. Writing for (A; B; C), the iterator should > > only be modified in A and C. You can also use the while cycle, if you want to > > keep just one instance of the std::search call. I'd suggest something like > this: > > > > it = suggestion.begin(); > > do { > > it = std::search(...); > > if (/*match found*/) { > > return ...; > > } > > } while (it != suggestion.end()); > > Done. > > This was changed to for(...) loop, originally was in while(...). > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > > But using | do { ... } while(...); | clause we have to write addtional exit case > as std::search(...) can return suggestion.end(). Ah, sorry about the back and forth, I did not realise that estade@ told you to use for-cycle. While I personally like the way you wrote this with do-while better (and view the separate exit condition as an improvement), estade@ is an OWNER of this file, so follow his direction and feel free to revert to the for-cycle. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text and highlights grammar: missing definite articles: the |suggestion| text the |input_element| text the end of the |suggestion| text https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text and highlights Wrong valency for preview, please change to: Previews |suggestion| in |input_element| and highlights... https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text and highlights Sorry, I gave up listing the nits for this comment block, and instead supply my own version. Please observe the following objectives when writing comments: * use simple, clear sentences * don't create large blocks of repeated text, nobody will be able to read those * first define what things are, then speak about them, not the other way round * for complicated cases, consider using examples * nouns need almost always articles in front of them; a particular exception are names, including variable names (e.g., "|suggestion|", but "a/the |suggestion| text") My suggestion for the comment (note, that I intentionally left out the discussion of substrings, because it is not essential to understand the main job of this function): Previews |suggestion| in |input_element| and highlights the suffix of |suggestion| not included in the |input_element| text. |input_element| must not be null. |user_input| should be the text typed by the user into |input_element|. Note that |user_input| cannot be easily derived from |input_element| by calling value(), because of http://crbug.com/507714. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:182: // The |input_element| is the text field being considered for preview and should nit: Do not repeat that the preview is done in the input element, just say that it should not be null. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:184: // The |suggestion| text is the field's suggestion to be previewed in the This sentence is completely duplicating what has already been said. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.cc:66: break; nit: Why not just return base::string16::npos; https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.h:16: // Returns true if the |field_contents| is a substring of the field's nit: What is "the field's |suggestion|"? Why not just "|suggestion|"? https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.h:28: // tokens. A token is a sequences of contiguous characters separated by any of Please do not repeat the definition of the token. This makes the comments bloat and very hard to read, and the two copies will get out of sync eventually. Instead, put a separate comment above line 16, explaining what is a token. Then you can refer to it in both declaration comments. That will have multiple benefits: * no duplication * all three comments will be simple, clear and easy to read * definition of "token" will come before using it, which also increases readability https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:41: {"ab@cd.b", "b", false, true}, You duplicated this line (see line 32), and there is still nothing testing a prefix match with the field contents spanning multiple tokens. Please add {"ab@cd.b", "ab@c", false, true} as I asked you to do in my previous comment.
Thanks Vaclav for inputs. I've incorporated these along new patch set, ptal. Thanks! https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/content/renderer/password_autofill_agent.cc:230: return base::StartsWith(username1, username2, base::CompareCase::SENSITIVE) || On 2015/07/13 14:04:25, vabr (Chromium) wrote: > On 2015/07/10 16:58:45, Pritam Nikam wrote: > > On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > > > I understand that the StartsWith call is there in case when > > > IsFeatureSubstringMatchEnabled returns false (because otherwise the test is > > > duplicated in ContainsTokenThatStartsWith). I'm not happy about the > > duplication, > > > and about the need to think about removing it once substring matching > becomes > > > 100% launched. So I suggest to change ContainsTokenThatStartsWith to try > > > matching from the begin every time, and from other token boundaries > depending > > on > > > IsFeatureSubstringMatchEnabled. Then you can delete StartsWith here > > completely. > > > > For autofill & CC suggestions we are using base::StartsWith(...) and > > ContainsTokenThatStartsWith(...) to identify suggestions with PREFIX or > > SUBSTRING matched so that we can order them. IMO, if we move > > base::StartsWith(...) to ContainsTokenThatStartsWith(...) it would be > difficult > > to do so by just making a single call to ContainsTokenThatStartsWith(...). To > > keep the API usage consistent across, let this be as it is. > > I don't understand what you mean. > FieldIsSuggestionSubstringStartingOnTokenBoundary already does prefix matching, > as a particular case of the substring matching. You can still call StartsWith > elsewhere first to check if there is a prefix match, as you do now. Here there > is no need to distinguish prefix and non-prefix matches (no ranking based on > it), so there is no need to call StartsWith. Except for > FieldIsSuggestionSubstringStartingOnTokenBoundary no doing any matching when > substring is not allowed. I don't see how this is about consistency in API > usage. Consistent API usage does not mean that everybody using the API will > compute the same thing. I don't agree with letting this be as it is for the > "consistency" argument. Done. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/co... components/autofill/core/common/autofill_util.cc:60: (it = std::search( On 2015/07/13 14:04:25, vabr (Chromium) wrote: > On 2015/07/10 16:58:46, Pritam Nikam wrote: > > On 2015/07/09 06:17:51, vabr (away until 13 July) wrote: > > > nit: Modifying iterator in the exit condition looks like a misuse of a > > > for-cycle, and harms readability. Writing for (A; B; C), the iterator should > > > only be modified in A and C. You can also use the while cycle, if you want > to > > > keep just one instance of the std::search call. I'd suggest something like > > this: > > > > > > it = suggestion.begin(); > > > do { > > > it = std::search(...); > > > if (/*match found*/) { > > > return ...; > > > } > > > } while (it != suggestion.end()); > > > > Done. > > > > This was changed to for(...) loop, originally was in while(...). > > > https://codereview.chromium.org/962673004/diff/620001/components/autofill/cor... > > > > But using | do { ... } while(...); | clause we have to write addtional exit > case > > as std::search(...) can return suggestion.end(). > > Ah, sorry about the back and forth, I did not realise that estade@ told you to > use for-cycle. While I personally like the way you wrote this with do-while > better (and view the separate exit condition as an improvement), estade@ is an > OWNER of this file, so follow his direction and feel free to revert to the > for-cycle. Done. Restored to for (...) cluase. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| text and highlights On 2015/07/13 14:04:25, vabr (Chromium) wrote: > Sorry, I gave up listing the nits for this comment block, and instead supply my > own version. Please observe the following objectives when writing comments: > * use simple, clear sentences > * don't create large blocks of repeated text, nobody will be able to read those > * first define what things are, then speak about them, not the other way round > * for complicated cases, consider using examples > * nouns need almost always articles in front of them; a particular exception are > names, including variable names (e.g., "|suggestion|", but "a/the |suggestion| > text") > > My suggestion for the comment (note, that I intentionally left out the > discussion of substrings, because it is not essential to understand the main job > of this function): > > Previews |suggestion| in |input_element| and highlights the suffix of > |suggestion| not included in the |input_element| text. |input_element| must not > be null. |user_input| should be the text typed by the user into |input_element|. > Note that |user_input| cannot be easily derived from |input_element| by calling > value(), because of http://crbug.com/507714. > > Done. Thanks. Above points you mentioned will definitely help me to improve drafting comments. I'll take care in future. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.cc:66: break; On 2015/07/13 14:04:26, vabr (Chromium) wrote: > nit: Why not just > > return base::string16::npos; Restored to for(...) instead. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.h:16: // Returns true if the |field_contents| is a substring of the field's On 2015/07/13 14:04:26, vabr (Chromium) wrote: > nit: What is "the field's |suggestion|"? Why not just "|suggestion|"? Done. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util.h:28: // tokens. A token is a sequences of contiguous characters separated by any of On 2015/07/13 14:04:26, vabr (Chromium) wrote: > Please do not repeat the definition of the token. This makes the comments bloat > and very hard to read, and the two copies will get out of sync eventually. > Instead, put a separate comment above line 16, explaining what is a token. Then > you can refer to it in both declaration comments. That will have multiple > benefits: > * no duplication > * all three comments will be simple, clear and easy to read > * definition of "token" will come before using it, which also increases > readability Done. https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... File components/autofill/core/common/autofill_util_unittest.cc (right): https://codereview.chromium.org/962673004/diff/1250001/components/autofill/co... components/autofill/core/common/autofill_util_unittest.cc:41: {"ab@cd.b", "b", false, true}, On 2015/07/13 14:04:26, vabr (Chromium) wrote: > You duplicated this line (see line 32), and there is still nothing testing a > prefix match with the field contents spanning multiple tokens. Please add > mailto:{"ab@cd.b", "ab@c", false, true} > as I asked you to do in my previous comment. Done. I guess I've missed it out of excitement :)
Thanks, Pritam Nikam, the passwords part LGTM. Also, I value your positive attitude! Cheers, Vaclav
On 2015/07/14 11:08:51, vabr (Chromium) wrote: > Thanks, Pritam Nikam, the passwords part LGTM. > > Also, I value your positive attitude! > > Cheers, > Vaclav Appreciated, thanks Vaclav :-) I'll proceed with CQ then. Seems this CL will land sometime soon :-) Thanks all for your help. Cheers!
The CQ bit was checked by pritam.nikam@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, estade@chromium.org, danakj@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/962673004/#ps1270001 (title: "Incorporated Vaclav's review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1270001
Message was sent while issue was closed.
Committed patchset #58 (id:1270001)
Message was sent while issue was closed.
Patchset 58 (id:??) landed as https://crrev.com/2dfdae10eacafadd4103018bde5b9481b3cec9d2 Cr-Commit-Position: refs/heads/master@{#338677} |
