Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1507)

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
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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+856 lines, -72 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 1 chunk +2 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/autofill_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 1 chunk +1 line, -2 lines 0 comments Download
M components/autofill/content/renderer/form_autofill_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +9 lines, -0 lines 0 comments Download
M components/autofill/content/renderer/form_autofill_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 3 chunks +17 lines, -3 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 1 chunk +2 lines, -3 lines 0 comments Download
M components/autofill/content/renderer/password_autofill_agent.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 7 chunks +13 lines, -13 lines 0 comments Download
M components/autofill/core/browser/autofill_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 1 chunk +172 lines, -0 lines 0 comments Download
M components/autofill/core/browser/personal_data_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 5 chunks +34 lines, -9 lines 0 comments Download
M components/autofill/core/browser/suggestion.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 2 chunks +6 lines, -0 lines 0 comments Download
M components/autofill/core/browser/suggestion.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 3 chunks +8 lines, -4 lines 0 comments Download
M components/autofill/core/browser/webdata/autofill_table.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 5 chunks +69 lines, -10 lines 0 comments Download
M components/autofill/core/browser/webdata/autofill_table_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 3 chunks +59 lines, -0 lines 0 comments Download
M components/autofill/core/common/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 2 chunks +3 lines, -0 lines 0 comments Download
M components/autofill/core/common/autofill_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 1 chunk +1 line, -0 lines 0 comments Download
M components/autofill/core/common/autofill_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 1 chunk +4 lines, -0 lines 0 comments Download
A components/autofill/core/common/autofill_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +38 lines, -0 lines 0 comments Download
A components/autofill/core/common/autofill_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +85 lines, -0 lines 0 comments Download
A components/autofill/core/common/autofill_util_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +99 lines, -0 lines 0 comments Download
M components/components_tests.gyp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 3 chunks +41 lines, -28 lines 0 comments Download
M components/password_manager/core/browser/password_autofill_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 3 chunks +178 lines, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 124 (22 generated)
Pritam Nikam
Hi Vaclav, Ilya & Evan, Please have a look. Thanks!
5 years, 9 months ago (2015-03-23 12:34:46 UTC) #4
vabr (Chromium)
Hi Pritam Nikam, I explicitly only had a look at components/password_manager/*, and expect that you'll ...
5 years, 9 months ago (2015-03-23 14:17:34 UTC) #5
Ilya Sherman
Thanks, Pritam. I'll defer to Evan for the initial review passes.
5 years, 9 months ago (2015-03-24 00:25:06 UTC) #6
Evan Stade
tests? https://codereview.chromium.org/962673004/diff/180001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/180001/components/autofill/content/renderer/password_autofill_agent.cc#newcode241 components/autofill/content/renderer/password_autofill_agent.cc:241: ? HasTokoneStartsWith(username1, username2, true) spelling of this function ...
5 years, 9 months ago (2015-03-24 00:48:26 UTC) #7
Pritam Nikam
Thanks Vaclav & Evan for review inputs. I've addresses inputs along new patch set, PTAL. ...
5 years, 9 months ago (2015-03-24 11:39:37 UTC) #8
vabr (Chromium)
Hi Pritam Nikam, I left some more comments. I only looked at components/password_manager/* + components/autofill/content/renderer/password_autofill_agent.cc ...
5 years, 9 months ago (2015-03-24 13:32:29 UTC) #9
Pritam Nikam
Thanks Vaclav for review comments. I've integrated most of your inputs along new patch set, ...
5 years, 9 months ago (2015-03-25 14:25:27 UTC) #10
Evan Stade
On 2015/03/25 14:25:27, Pritam Nikam wrote: > Thanks Vaclav for review comments. > > I've ...
5 years, 9 months ago (2015-03-25 21:28:17 UTC) #11
Pritam Nikam
On 2015/03/25 21:28:17, Evan Stade wrote: > On 2015/03/25 14:25:27, Pritam Nikam wrote: > > ...
5 years, 9 months ago (2015-03-26 14:00:24 UTC) #13
Evan Stade
+jshin --- question below https://codereview.chromium.org/962673004/diff/280001/components/autofill/core/common/autofill_util.cc File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/core/common/autofill_util.cc#newcode39 components/autofill/core/common/autofill_util.cc:39: Tokenize(suggestion_string, base::ASCIIToUTF16(" .,-_@"), can we ...
5 years, 9 months ago (2015-03-26 18:54:38 UTC) #15
vabr (Chromium)
Hi Pritam Nikam, Thanks for the update! I added some more comments, mostly nits, but ...
5 years, 9 months ago (2015-03-27 10:13:35 UTC) #16
Pritam Nikam
Thanks Evan & Vaclav, Patch set #13 addresses your inputs, please have a look. Thanks! ...
5 years, 9 months ago (2015-03-27 14:57:43 UTC) #17
vabr (Chromium)
Hi Pritam, I had a look in autofill_util.cc as well, because I sensed a misunderstanding ...
5 years, 8 months ago (2015-03-30 14:51:04 UTC) #18
Pritam Nikam
Thanks Vaclav once again. I've addresses your inputs with new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/340001/components/autofill/core/common/autofill_util.cc ...
5 years, 8 months ago (2015-03-30 17:35:48 UTC) #20
Evan Stade
https://codereview.chromium.org/962673004/diff/280001/components/autofill/core/common/autofill_util.h File components/autofill/core/common/autofill_util.h (right): https://codereview.chromium.org/962673004/diff/280001/components/autofill/core/common/autofill_util.h#newcode30 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 ...
5 years, 8 months ago (2015-03-30 17:44:58 UTC) #21
vabr (Chromium)
Thanks, Pritam Nikam! The IsContentsPrefixOfSuggestionToken method is now much clearer (the name now matches exactly ...
5 years, 8 months ago (2015-03-31 09:38:07 UTC) #22
Pritam Nikam
Thanks Vaclav and Evan for review inputs. I've incorporated all inputs in new patch set, ...
5 years, 8 months ago (2015-03-31 14:26:12 UTC) #24
Evan Stade
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_util.h#newcode505 base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, I think you want ...
5 years, 8 months ago (2015-03-31 21:41:51 UTC) #25
Evan Stade
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_util.h#newcode505 base/strings/string_util.h:505: BASE_EXPORT base::string16::size_type StartsAt(const base::string16& str, I think you want ...
5 years, 8 months ago (2015-03-31 21:41:52 UTC) #26
Pritam Nikam
+ 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 ...
5 years, 8 months ago (2015-04-01 09:10:37 UTC) #28
vabr (Chromium)
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 ...
5 years, 8 months ago (2015-04-01 11:33:00 UTC) #29
Pritam Nikam
Thanks Vaclav once again for your inputs. I've incorporated these along new patch set, please ...
5 years, 8 months ago (2015-04-01 12:59:38 UTC) #30
vabr (Chromium)
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 ...
5 years, 8 months ago (2015-04-01 13:02:13 UTC) #31
jam
On 2015/04/01 09:10:37, Pritam Nikam wrote: > + mailto:jam@chromium.org: Please review changes in > base/strings/string_util.cc ...
5 years, 8 months ago (2015-04-01 18:19:39 UTC) #32
Pritam Nikam
On 2015/04/01 18:19:39, jam wrote: > On 2015/04/01 09:10:37, Pritam Nikam wrote: > > + ...
5 years, 8 months ago (2015-04-02 03:59:32 UTC) #34
danakj
https://codereview.chromium.org/962673004/diff/460001/base/strings/string_util.cc File base/strings/string_util.cc (right): https://codereview.chromium.org/962673004/diff/460001/base/strings/string_util.cc#newcode1021 base/strings/string_util.cc:1021: return (it != str.end()) ? std::distance(str.begin(), it) : string16::npos; ...
5 years, 8 months ago (2015-04-02 18:31:43 UTC) #35
Pritam Nikam
Thanks danakj for inputs. I've incorporated these in new patch set, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/460001/base/strings/string_util.cc File ...
5 years, 8 months ago (2015-04-06 06:13:10 UTC) #36
Pritam Nikam
Hi all, I've removed changes from "base/strings/" in latest patch set, PTAL. Thanks!
5 years, 8 months ago (2015-04-13 13:40:16 UTC) #37
Pritam Nikam
Hi all, PTAL, Thanks!
5 years, 8 months ago (2015-04-22 12:09:28 UTC) #38
vabr (Chromium)
On 2015/04/22 12:09:28, Pritam Nikam wrote: > Hi all, > > PTAL, Thanks! Hi Pritam ...
5 years, 8 months ago (2015-04-23 08:36:55 UTC) #40
Pritam Nikam
On 2015/04/23 08:36:55, vabr (Chromium) wrote: > On 2015/04/22 12:09:28, Pritam Nikam wrote: > > ...
5 years, 8 months ago (2015-04-23 09:02:58 UTC) #41
danakj
On 2015/04/23 09:02:58, Pritam Nikam wrote: > On 2015/04/23 08:36:55, vabr (Chromium) wrote: > > ...
5 years, 8 months ago (2015-04-23 16:07:52 UTC) #42
Evan Stade
https://codereview.chromium.org/962673004/diff/520001/components/autofill/core/common/autofill_util.cc File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/520001/components/autofill/core/common/autofill_util.cc#newcode49 components/autofill/core/common/autofill_util.cc:49: for (auto token : suggestion_tokens) { nit: don't use ...
5 years, 8 months ago (2015-04-23 20:21:18 UTC) #43
Pritam Nikam
Thanks Evan and danakj. I've addressed Evan's review inputs in new patch set, ptal. Thanks! ...
5 years, 8 months ago (2015-04-27 07:53:41 UTC) #44
Pritam Nikam
Thanks Evan and danakj. I've addressed Evan's review inputs in new patch set, ptal. Thanks!
5 years, 8 months ago (2015-04-27 07:53:58 UTC) #45
Evan Stade
https://codereview.chromium.org/962673004/diff/560001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/560001/components/autofill/content/renderer/autofill_agent.cc#newcode730 components/autofill/content/renderer/autofill_agent.cc:730: if (start != base::string16::npos) { no curlies https://codereview.chromium.org/962673004/diff/560001/components/autofill/content/renderer/form_autofill_util.cc File ...
5 years, 8 months ago (2015-04-27 19:25:21 UTC) #46
Evan Stade
this patch also seems to get case sensitivity wrong. For example, if I have an ...
5 years, 8 months ago (2015-04-28 00:37:22 UTC) #47
Pritam Nikam
Thanks Evan for review inputs. I've addressed your inputs in new patch. Credit card name ...
5 years, 7 months ago (2015-04-28 14:45:55 UTC) #48
Pritam Nikam
Hi Evan, PTAL. Thanks!
5 years, 7 months ago (2015-05-11 11:08:24 UTC) #49
Evan Stade
having patched this into my local machine and tried it out, I think my previous ...
5 years, 7 months ago (2015-05-11 19:07:12 UTC) #50
Pritam Nikam
Thanks Evan for review. I'll address these on monday. Thanks! On 2015/05/11 19:07:12, Evan Stade ...
5 years, 7 months ago (2015-05-13 22:52:50 UTC) #51
Pritam Nikam
Thanks Evan, I've incorporated reviews in new patch, & may need to discuss this further: ...
5 years, 7 months ago (2015-05-21 10:58:22 UTC) #52
Evan Stade
> having patched this into my local machine and tried it out, I think my ...
5 years, 7 months ago (2015-05-22 18:26:48 UTC) #53
Pritam Nikam
Thanks Evan for review comments. I've tried incorporating your review inputs, ptal. Thanks! On 2015/05/22 ...
5 years, 7 months ago (2015-05-25 18:14:52 UTC) #55
Pritam Nikam
Hi Evan, As suggested, with this patch I've ordered prefix followed by sub-string match suggestions, ...
5 years, 6 months ago (2015-06-04 05:50:03 UTC) #57
Evan Stade
Hi Pritam, can you rebase? This patch doesn't apply cleanly on my local machine.
5 years, 6 months ago (2015-06-08 23:14:54 UTC) #58
Evan Stade
https://codereview.chromium.org/962673004/diff/790001/components/autofill/core/browser/personal_data_manager.cc File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/962673004/diff/790001/components/autofill/core/browser/personal_data_manager.cc#newcode899 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 ...
5 years, 6 months ago (2015-06-08 23:23:38 UTC) #59
Evan Stade
+rouslan to take over this review for me as I'll be on vacation soon. https://codereview.chromium.org/962673004/diff/790001/components/autofill/core/browser/suggestion.cc ...
5 years, 6 months ago (2015-06-08 23:24:35 UTC) #61
please use gerrit instead
On 2015/06/08 23:24:35, Evan Stade wrote: > +rouslan to take over this review for me ...
5 years, 6 months ago (2015-06-08 23:36:23 UTC) #62
Pritam Nikam
Thanks Evan and Rouslan for review. I've incorporated review inputs in new patch set, PTAL. ...
5 years, 6 months ago (2015-06-09 11:39:06 UTC) #63
please use gerrit instead
https://codereview.chromium.org/962673004/diff/830001/components/autofill/content/renderer/autofill_agent.cc File components/autofill/content/renderer/autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/830001/components/autofill/content/renderer/autofill_agent.cc#newcode736 components/autofill/content/renderer/autofill_agent.cc:736: size_t start = autofill::GetTextSelectionStart(value, node->value()); You've calculated the |start| ...
5 years, 6 months ago (2015-06-11 19:10:26 UTC) #64
Pritam Nikam
Thanks Rouslan for review inputs. Currently we are using autofill::ContainsTokenThatStartsWith() verify substring matching check to ...
5 years, 6 months ago (2015-06-12 11:09:11 UTC) #65
please use gerrit instead
Hi Pritam, Sorry for the delay. Please find my explanations below. If I'm not being ...
5 years, 6 months ago (2015-06-22 00:32:59 UTC) #66
please use gerrit instead
Continuing the discussion here is also OK, of course. To unsubscribe from this group and ...
5 years, 6 months ago (2015-06-22 01:00:31 UTC) #67
Pritam Nikam
Thanks Rouslan for your inputs. I've replied below, please have a look. Thanks! https://codereview.chromium.org/962673004/diff/830001/components/autofill/content/renderer/autofill_agent.cc File ...
5 years, 6 months ago (2015-06-22 18:31:40 UTC) #68
please use gerrit instead
On 2015/06/22 18:31:40, Pritam Nikam wrote: > If its ok, I'll do these in follow-up ...
5 years, 6 months ago (2015-06-23 22:09:30 UTC) #69
Pritam Nikam
On 2015/06/23 22:09:30, Rouslan wrote: > On 2015/06/22 18:31:40, Pritam Nikam wrote: > > If ...
5 years, 5 months ago (2015-06-27 10:16:56 UTC) #70
please use gerrit instead
Pritam, Thank you for creating the other two patches for Blink and IPC changes. After ...
5 years, 5 months ago (2015-06-28 00:43:04 UTC) #71
please use gerrit instead
https://codereview.chromium.org/962673004/diff/860001/components/autofill/core/browser/autocomplete_history_manager.cc File components/autofill/core/browser/autocomplete_history_manager.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/core/browser/autocomplete_history_manager.cc#newcode91 components/autofill/core/browser/autocomplete_history_manager.cc:91: std::vector<base::string16> preferred_suggestions; Wouldn't it be better to perform this ...
5 years, 5 months ago (2015-06-28 01:20:07 UTC) #72
please use gerrit instead
https://codereview.chromium.org/962673004/diff/860001/components/autofill/core/common/autofill_util.cc File components/autofill/core/common/autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/core/common/autofill_util.cc#newcode20 components/autofill/core/common/autofill_util.cc:20: const base::string16 kSplitCharacters = base::ASCIIToUTF16(" .,-_@"); Wouldn't this statement ...
5 years, 5 months ago (2015-06-28 01:29:39 UTC) #73
Pritam Nikam
Thanks Rouslan for detail review. I've incorporated most of your inputs. There were few difficulties ...
5 years, 5 months ago (2015-06-29 15:38:21 UTC) #74
please use gerrit instead
On 2015/06/29 15:38:21, Pritam Nikam wrote: > On 2015/06/28 00:43:04, Rouslan wrote: > > I ...
5 years, 5 months ago (2015-06-29 17:06:45 UTC) #75
Pritam Nikam
Thanks Rouslan again for quick reply. Please find my in-line replies. Thanks! On 2015/06/29 17:06:45, ...
5 years, 5 months ago (2015-06-29 17:19:09 UTC) #76
please use gerrit instead
Please let me know when I can continue reviewing your code. On 2015/06/29 17:19:09, Pritam ...
5 years, 5 months ago (2015-06-29 17:31:02 UTC) #77
Pritam Nikam
On 2015/06/29 17:31:02, Rouslan wrote: > Please let me know when I can continue reviewing ...
5 years, 5 months ago (2015-06-29 18:09:53 UTC) #78
please use gerrit instead
Pritam, please let me know when you address all outstanding comments, so I can go ...
5 years, 5 months ago (2015-06-29 18:31:06 UTC) #79
Pritam Nikam
On 2015/06/29 18:31:06, Rouslan wrote: > Pritam, please let me know when you address all ...
5 years, 5 months ago (2015-06-29 19:00:22 UTC) #80
please use gerrit instead
https://codereview.chromium.org/962673004/diff/860001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/860001/components/autofill/content/renderer/password_autofill_agent.cc#newcode230 components/autofill/content/renderer/password_autofill_agent.cc:230: ContainsTokenThatStartsWith(username1, username2, true); On 2015/06/29 15:38:21, Pritam Nikam wrote: ...
5 years, 5 months ago (2015-06-29 22:06:30 UTC) #81
Pritam Nikam
Thanks Rouslan for review. I've incorporated review comments in new patch set, ptal. Thanks! https://codereview.chromium.org/962673004/diff/860001/components/autofill/content/renderer/password_autofill_agent.cc ...
5 years, 5 months ago (2015-06-30 15:05:51 UTC) #82
vabr (Chromium)
Hi Pritam! On 2015/06/29 17:19:09, Pritam Nikam wrote: > Thanks Rouslan again for quick reply. ...
5 years, 5 months ago (2015-06-30 15:06:27 UTC) #83
please use gerrit instead
On 2015/06/30 15:06:27, vabr (Chromium) wrote: > I'm no longer remembering all the details, but ...
5 years, 5 months ago (2015-06-30 19:06:24 UTC) #84
please use gerrit instead
By the way, please fix a crash in AutofillAgent::OnPreviewPasswordSuggestion() after your patch is applied. [1:1:0630/134601:FATAL:autofill_util.cc(67)] ...
5 years, 5 months ago (2015-06-30 20:53:07 UTC) #85
vabr (Chromium)
Heads-up: Conflicting changes coming in https://codereview.chromium.org/1220653002/.
5 years, 5 months ago (2015-07-01 09:14:18 UTC) #86
vabr (Chromium)
On 2015/07/01 09:14:18, vabr (Chromium) wrote: > Heads-up: Conflicting changes coming in > https://codereview.chromium.org/1220653002/. Note ...
5 years, 5 months ago (2015-07-01 09:15:55 UTC) #87
Pritam Nikam
Thanks Vaclav and Rouslan for your inputs. SQL LIKE clause does not go well with ...
5 years, 5 months ago (2015-07-01 17:26:00 UTC) #88
please use gerrit instead
On 2015/07/01 17:26:00, Pritam Nikam wrote: > SQL LIKE clause does not go well with ...
5 years, 5 months ago (2015-07-03 02:05:43 UTC) #89
Pritam Nikam
Thanks Rouslan for your inputs. I've addressed these in review comments, ptal. Thanks! https://codereview.chromium.org/962673004/diff/960001/components/autofill/core/browser/webdata/autofill_table.cc File ...
5 years, 5 months ago (2015-07-03 16:21:28 UTC) #90
please use gerrit instead
https://codereview.chromium.org/962673004/diff/1010024/components/autofill/content/renderer/form_autofill_util.cc File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/content/renderer/form_autofill_util.cc#newcode1541 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: ...
5 years, 5 months ago (2015-07-03 22:05:47 UTC) #91
Pritam Nikam
Thanks Rouslan for review. PTAL. Thanks! https://codereview.chromium.org/962673004/diff/1010024/components/autofill/content/renderer/form_autofill_util.cc File components/autofill/content/renderer/form_autofill_util.cc (right): https://codereview.chromium.org/962673004/diff/1010024/components/autofill/content/renderer/form_autofill_util.cc#newcode1541 components/autofill/content/renderer/form_autofill_util.cc:1541: ? std::min(autofill::GetTextSelectionStart(suggestion, user_input), ...
5 years, 5 months ago (2015-07-06 15:29:28 UTC) #92
please use gerrit instead
Looking good! Remaining suggestions are mostly for code style and comments. https://codereview.chromium.org/962673004/diff/1090001/components/autofill/content/renderer/form_autofill_util.cc File components/autofill/content/renderer/form_autofill_util.cc (right): ...
5 years, 5 months ago (2015-07-06 20:15:18 UTC) #93
Pritam Nikam
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/content/renderer/form_autofill_util.cc ...
5 years, 5 months ago (2015-07-07 16:18:32 UTC) #94
please use gerrit instead
autofill lgtm % nit. Evan, this looks good to me now. Can you approve it ...
5 years, 5 months ago (2015-07-07 18:09:12 UTC) #95
Evan Stade
lgtm for OWNERS
5 years, 5 months ago (2015-07-07 19:17:50 UTC) #96
Pritam Nikam
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/core/common/autofill_util.cc File components/autofill/core/common/autofill_util.cc ...
5 years, 5 months ago (2015-07-08 06:23:06 UTC) #97
Ilya Sherman
histograms.xml lgtm
5 years, 5 months ago (2015-07-08 06:41:43 UTC) #98
Pritam Nikam
Hi Vaclav, I've modified "components/autofill/content/renderer/password_autofill_agent.cc & .h" - Removed |username_selection_start_| as its not being usefed ...
5 years, 5 months ago (2015-07-08 15:28:32 UTC) #99
vabr (Chromium)
Thanks, Pritam Nikam. I left some comments. Also, should the changes in the PasswordAutofillAgent be ...
5 years, 5 months ago (2015-07-09 06:17:52 UTC) #100
vabr (Chromium)
Some updates. https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/form_autofill_util.h File components/autofill/content/renderer/form_autofill_util.h (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/form_autofill_util.h#newcode179 components/autofill/content/renderer/form_autofill_util.h:179: // Previews the |input_element| with supplied |suggestion| ...
5 years, 5 months ago (2015-07-09 08:27:50 UTC) #101
Pritam Nikam
Thanks Vaclav for review. I've addressed review inputs along patch set #56, PTAL. Thanks! https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/form_autofill_util.h ...
5 years, 5 months ago (2015-07-10 16:58:46 UTC) #102
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1230001
5 years, 5 months ago (2015-07-10 17:25:09 UTC) #106
commit-bot: I haz the power
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_rel_ng/builds/43156)
5 years, 5 months ago (2015-07-10 20:08:55 UTC) #108
please use gerrit instead
Here is the test failure, by the way: [ RUN ] AutofillManagerTest.DisplayCreditCardSuggestionsWithMatchingTokens ../../components/autofill/core/browser/autofill_manager_unittest.cc:543: Failure Value ...
5 years, 5 months ago (2015-07-10 20:31:29 UTC) #109
Pritam Nikam
On 2015/07/10 20:31:29, Rouslan wrote: > Here is the test failure, by the way: > ...
5 years, 5 months ago (2015-07-13 08:14:16 UTC) #110
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1250001
5 years, 5 months ago (2015-07-13 08:18:56 UTC) #113
commit-bot: I haz the power
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_asan_rel_ng/builds/27045) (exceeded global ...
5 years, 5 months ago (2015-07-13 08:34:53 UTC) #115
vabr (Chromium)
Hi Pritam Nikam, I left some more comments. Cheers, Vaclav https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/password_autofill_agent.cc File components/autofill/content/renderer/password_autofill_agent.cc (right): https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/password_autofill_agent.cc#newcode230 ...
5 years, 5 months ago (2015-07-13 14:04:26 UTC) #116
Pritam Nikam
Thanks Vaclav for inputs. I've incorporated these along new patch set, ptal. Thanks! https://codereview.chromium.org/962673004/diff/1170001/components/autofill/content/renderer/password_autofill_agent.cc File ...
5 years, 5 months ago (2015-07-14 10:30:08 UTC) #117
vabr (Chromium)
Thanks, Pritam Nikam, the passwords part LGTM. Also, I value your positive attitude! Cheers, Vaclav
5 years, 5 months ago (2015-07-14 11:08:51 UTC) #118
Pritam Nikam
On 2015/07/14 11:08:51, vabr (Chromium) wrote: > Thanks, Pritam Nikam, the passwords part LGTM. > ...
5 years, 5 months ago (2015-07-14 12:40:25 UTC) #119
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/962673004/1270001
5 years, 5 months ago (2015-07-14 12:40:55 UTC) #122
commit-bot: I haz the power
Committed patchset #58 (id:1270001)
5 years, 5 months ago (2015-07-14 13:51:27 UTC) #123
commit-bot: I haz the power
5 years, 5 months ago (2015-07-14 13:52:17 UTC) #124
Message was sent while issue was closed.
Patchset 58 (id:??) landed as
https://crrev.com/2dfdae10eacafadd4103018bde5b9481b3cec9d2
Cr-Commit-Position: refs/heads/master@{#338677}

Powered by Google App Engine
This is Rietveld 408576698