Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2187343002:
Generating autocomplete results with and without word breaks in the Omnibox. (Closed)
|
Created:
4 years, 4 months ago by Lavar Askew Modified:
4 years, 3 months ago Reviewers:
Mark P CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionGenerating autocomplete results with and without word breaks in the Omnibox.
The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes".
BUG=591979
TEST=0. Clear browser history.
1. Visit the following link: https://twitter.com/fungoodtimes
2. Open a new browser tab.
3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL.
4. Insert the cursor between the "n" and "t" in "funtime" and type "good".
5. The above URL should show in the autocomplete list.
Committed: https://crrev.com/e3235816f23e9eb733d11bdecdfd5f8ca67cec9f
Cr-Commit-Position: refs/heads/master@{#418054}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Code review fixes for patch entitled "Generating autocomplete results with and without word breaks … #
Total comments: 20
Patch Set 3 : I tried to remove duplicates and improve effciency. I appreciated the comments. #
Total comments: 17
Patch Set 4 : corrected formatting and improved comments. Also added unit test MatchWithAndWithoutCursorWordBreak #
Total comments: 20
Patch Set 5 : Unit test added. Grammatical errors corrected. #
Total comments: 8
Patch Set 6 : Added the fungoodtimes twitter URL to the end of the list. Placed ExtractIndividualWordVector, Get… #
Total comments: 4
Patch Set 7 : Removed large comment. Added the 'a' back in. #Patch Set 8 : Test seems to work now. I just checked out the most recent commit of history_quick_provider_unitte… #Patch Set 9 : Correct code causing tests in HistoryQuickProviderTest to fail. #
Total comments: 11
Patch Set 10 : Made changes to bring code inline with best practices. #Patch Set 11 : Put common code for handling multiple search strings inside for loop. This removes the copy paste … #Patch Set 12 : Removed new line after defining lower_words. #
Total comments: 17
Patch Set 13 : Made recommended changes. #
Total comments: 1
Messages
Total messages: 62 (24 generated)
The CQ bit was checked by open.hyperion@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
open.hyperion@gmail.com changed reviewers: + pkasting@chromium.org
Hi, can you please review my code for issue 591979? I've added the ability to build the history id set for the search term with and without the imaginary space inserted. Thanks, Lavar
Description was changed from ========== Created search_string_without_break variable to store original search_string. Built history_id_set_without_break based off search_string_without_break and merged with history_id_set. BUG=591979 TEST=Type at least a 2 character string into the omnibox. Set the cursor anywhere within the string that is not the start of the string and not the end. A white space should be added to the string internally and the history should be searched for both the search term with the white space inserted and without. ========== to ========== Created search_string_without_break variable to store original search_string. Built history_id_set_without_break based off search_string_without_break and merged with history_id_set. BUG=591979 TEST=Type at least a 2 character string into the omnibox. Set the cursor anywhere within the string that is not the start of the string and not the end. A white space should be added to the string internally and the history should be searched for both the search term with the white space inserted and without. ==========
open.hyperion@gmail.com changed reviewers: + peter kasting;mpearson@chromium.org - pkasting@chromium.org
open.hyperion@gmail.com changed reviewers: + mpearson@chromium.org, pkasting@chromium.org - peter kasting;mpearson@chromium.org
Thanks for contributing this CL. You'll want to change the CL title, and the first line of the description, into a short, one-sentence summary of the change, then use the rest of the description to describe why you're making the change (as opposed to describing in detail what you're doing). You'll also want to make the TEST= line something the QA folks can test without a debugger. Ideally, this would look something like "on a clean profile, use the omnibox to navigate to _____, then do _____ in the omnibox and ensure _____ is the first result" or similar. Ultimately Mark is probably a better reviewer than me, so removing myself, but I did take a first pass through. Mark: Please double-check the CLA status. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:168: base::string16 search_string_without_break; It would be nice to avoid the need for this temp and keep all the added logic here together. One way might be: move the conditional that was here down to where you added your other block, so we unconditionally search for the original input string. Then combine this block and the other new block, so that we check if we should insert a space, and if so, go ahead and look in the history DB for its words and merge in the results. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:177: (search_string.compare(cursor_position - 1, 1, blank_space) != 0) || I suspect that rather than comparing against " " specifically, you want to check if this is somewhere ICU would line-break, to match how String16VectorFromString16() splits into words. The dumbest way to do this would be to simply call String16VectorFromString16() twice, once with the space and once without, and throw away one set if it's the same as the other. There may be a better way, though. I would check with mpearson and jshin. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:178: (search_string.compare(cursor_position + 1, 1, blank_space) != 0)) { I think you don't want to compare cursor_position + 1. That would be the next character after whatever is to the right of the cursor. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:220: search_string.compare(search_string_without_break) != 0) { It looks to me as if this copy-and-pastes some of the code above, and perhaps the duplicated parts should be pulled out into a helper function instead.
pkasting@chromium.org changed reviewers: - pkasting@chromium.org
Description was changed from ========== Created search_string_without_break variable to store original search_string. Built history_id_set_without_break based off search_string_without_break and merged with history_id_set. BUG=591979 TEST=Type at least a 2 character string into the omnibox. Set the cursor anywhere within the string that is not the start of the string and not the end. A white space should be added to the string internally and the history should be searched for both the search term with the white space inserted and without. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Visit the following 2 links: www.kare11.com/life/playground-fun-good-times-park/261522192 https://twitter.com/fungoodtimes 1.Open a new browser tab. 2.Type into the Omnibox "funtimes". 3.Insert the cursor between the "n" and "t" in "funtime" and type "good". 4.The first and the second link should show in the autocomplete list. ==========
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Visit the following 2 links: www.kare11.com/life/playground-fun-good-times-park/261522192 https://twitter.com/fungoodtimes 1.Open a new browser tab. 2.Type into the Omnibox "funtimes". 3.Insert the cursor between the "n" and "t" in "funtime" and type "good". 4.The first and the second link should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0.Clear browser history. 1. Visit the following 2 links: www.kare11.com/life/playground-fun-good-times-park/261522192 https://twitter.com/fungoodtimes 2.Open a new browser tab. 3.Type into the Omnibox "funtimes". 4.Insert the cursor between the "n" and "t" in "funtime" and type "good". 5.The first and the second link should show in the autocomplete list. ==========
Hi, Thanks for putting together a patch. I have a question about how it's supposed to work. In your changelist description--thanks for filling this in by the way--you write: >>> Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". >>> Did you mean: "the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes".'? After all, the cursor will be at the end of the word "good", not the beginning of the word, and we split the word where the cursor is. Or am I misunderstanding what you want the user to do? Likewise, in your test instructions, I think you can simply change this to: >>> 0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. >>> The reason I make this suggestion is that I don't think the first URL you listed originally should show up in step 5 under either the current or new behavior (unless the suggestion is cached somewhere). The current behavior would look up the search string "fungood times", and I don't see "fungood" on the first URL. The new behavior would also look up the search string "fungoodtimes", and I don't see that in the first URL either. As for the content of the changelist, I will wait for you to reply to pkasting@'s initial comments before I take a look. Let me know when you made the improvements. thanks, mark
Hi Mark, Thanks for reviewing my patch. In regards to your first comment you are correct. I should have written "fungood times". So we want to search for both "fungood times" and "fungoodtimes". I also have a question though. How about if we searched the history for "fun good times", "fungood times", "fun goodtimes" and "fungoodtimes"? Speaking to the second comment again you are correct. I was wondering if we should attempt to search for "fungoodtimes" in the 4 ways that I mentioned above because I am not sure exactly which side of the cursor the user would prefer to insert the blank space. Let me know which you would prefer, either: "fungoodtimes", "fungood times" or "fun good times", "fungood times", "fun goodtimes" and "fungoodtimes" In the second case the URL would show, which seemed like a nice touch to me. However, I did incorrectly state the test case. Anyways, let me know which one you prefer and I will take care of that.
On Thu, Aug 4, 2016 at 11:49 AM, <open.hyperion@gmail.com> wrote: > Hi Mark, > > Thanks for reviewing my patch. > > In regards to your first comment you are correct. I should have written > "fungood times". So we want to search for both "fungood times" and > "fungoodtimes". I also have a question though. > > How about if we searched the history for "fun good times", "fungood > times", "fun > goodtimes" and "fungoodtimes"? > I don't think this is will work. First, for it to work, you'd have to remember "the user put the cursor HERE before typing more" and then adding a space (splitting the string) at HERE. I think getting Chrome to remember this would take a chunk of work. Second, I don't think the user will expect this behavior--I don't think users would expect Chrome to remember where the cursor was before and treat those as word breaks. Let's stick with fixing your original motivating example, not more. If we come up with a plausible motivating example for the latter case, let's consider it later. --mark > Speaking to the second comment again you are correct. I was wondering if we > should attempt to search for "fungoodtimes" in the 4 ways that I mentioned > above > because I am not sure exactly which side of the cursor the user would > prefer to > insert the blank space. Let me know which you would prefer, either: > > "fungoodtimes", "fungood times" > > or > > "fun good times", "fungood times", "fun goodtimes" and "fungoodtimes" > > In the second case the URL would show, which seemed like a nice touch to > me. > However, I did incorrectly state the test case. > > Anyways, let me know which one you prefer and I will take care of that. > > https://codereview.chromium.org/2187343002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/04 19:50:58, Mark P wrote: > On Thu, Aug 4, 2016 at 11:49 AM, <mailto:open.hyperion@gmail.com> wrote: > > > Hi Mark, > > > > Thanks for reviewing my patch. > > > > In regards to your first comment you are correct. I should have written > > "fungood times". So we want to search for both "fungood times" and > > "fungoodtimes". I also have a question though. > > > > How about if we searched the history for "fun good times", "fungood > > times", "fun > > goodtimes" and "fungoodtimes"? > > > > I don't think this is will work. First, for it to work, you'd have to > remember "the user put the cursor HERE before typing more" and then adding > a space (splitting the string) at HERE. I think getting Chrome to remember > this would take a chunk of work. Second, I don't think the user will > expect this behavior--I don't think users would expect Chrome to remember > where the cursor was before and treat those as word breaks. Let's stick > with fixing your original motivating example, not more. > > If we come up with a plausible motivating example for the latter case, > let's consider it later. > > --mark > > > > Speaking to the second comment again you are correct. I was wondering if we > > should attempt to search for "fungoodtimes" in the 4 ways that I mentioned > > above > > because I am not sure exactly which side of the cursor the user would > > prefer to > > insert the blank space. Let me know which you would prefer, either: > > > > "fungoodtimes", "fungood times" > > > > or > > > > "fun good times", "fungood times", "fun goodtimes" and "fungoodtimes" > > > > In the second case the URL would show, which seemed like a nice touch to > > me. > > However, I did incorrectly state the test case. > > > > Anyways, let me know which one you prefer and I will take care of that. > > > > https://codereview.chromium.org/2187343002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi Mark, I have made the changes recommended by Peter. Everything was not an exact one to one change, but just working through the removal of the "copy-and-pastes" and creating 3 new methods I hope that the spirit of what Peter asked for has been met. Thanks, Lavar
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0.Clear browser history. 1. Visit the following 2 links: www.kare11.com/life/playground-fun-good-times-park/261522192 https://twitter.com/fungoodtimes 2.Open a new browser tab. 3.Type into the Omnibox "funtimes". 4.Insert the cursor between the "n" and "t" in "funtime" and type "good". 5.The first and the second link should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ==========
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fun goodtimes" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ==========
Hi Mark, would you mind reviewing my code? Thanks, Lavar https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:168: base::string16 search_string_without_break; On 2016/07/28 20:32:20, Peter Kasting wrote: > It would be nice to avoid the need for this temp and keep all the added logic > here together. One way might be: move the conditional that was here down to > where you added your other block, so we unconditionally search for the original > input string. Then combine this block and the other new block, so that we check > if we should insert a space, and if so, go ahead and look in the history DB for > its words and merge in the results. Done. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:177: (search_string.compare(cursor_position - 1, 1, blank_space) != 0) || On 2016/07/28 20:32:20, Peter Kasting wrote: > I suspect that rather than comparing against " " specifically, you want to check > if this is somewhere ICU would line-break, to match how > String16VectorFromString16() splits into words. > > The dumbest way to do this would be to simply call String16VectorFromString16() > twice, once with the space and once without, and throw away one set if it's the > same as the other. There may be a better way, though. I would check with > mpearson and jshin. Done. I have removed this code after following your "copy-and-pastes" refactoring suggestion below. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:178: (search_string.compare(cursor_position + 1, 1, blank_space) != 0)) { On 2016/07/28 20:32:20, Peter Kasting wrote: > I think you don't want to compare cursor_position + 1. That would be the next > character after whatever is to the right of the cursor. Done. I have removed this code. https://codereview.chromium.org/2187343002/diff/1/components/omnibox/browser/... components/omnibox/browser/url_index_private_data.cc:220: search_string.compare(search_string_without_break) != 0) { On 2016/07/28 20:32:20, Peter Kasting wrote: > It looks to me as if this copy-and-pastes some of the code above, and perhaps > the duplicated parts should be pulled out into a helper function instead. Done.
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ==========
Sorry for the delay in reviewing. Generally the approach seems fine, though you've at times overlooked some situations (e.g., duplicates, efficiency). A number of comments relate to style nitpicks, which is normal for first-time code submissions. Please don't be put off by those comments! --mark https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:158: HistoryIDSet history_id_set; This variable isn't needed; simply return HistoryIDSet() at the first return below and return HistoryIDSetFromWords(lower_words) at the second return, no need to create an intermediate copy within the function. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:252: // Trim the candidate pool if it is large. Note that we do not filter out I assume this whole block was copied with no modifications. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:331: HistoryItemsForWords(search_string_with_word_break); This might do unnecessary work if the new search string breaks into the same words as the old one. The search term cache may help avoid some of this unnecessary work, but I think we can do better here. Can you please improve this? Let's not slow down users unnecessarily. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:362: //ScoredHistoryMatches for the search string with and without the word break. The code below can be made more efficient and easier to read. Approximately: if (without_word_break.empty()) return with_word_break with_word_break.insert(with_word_break.end(), without.begin(), without.end()) return with_word_break; ( http://stackoverflow.com/a/2208313 is good reading by the way about what I skipped the reserve. Also this technique doesn't need another variable, no need to copy all those entries to a third variable.) https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:385: I could imagine situations where the items overlap. You should be removing duplicates (smartly) here. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:386: return all_scored_items; Also, the number of items we're supposed to return should be no more than max_matches. You need to truncate the list if necessary. Right now you may be returning twice that number. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: bool TrimCandidatePool ( This function and the one below should be commented. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: bool TrimCandidatePool ( Here and throughout the change, you introduce a number of style violations. (Indeed, at times the only thing you did on some lines is to change the spacing to a way that's not consistent with chromium style.) Can you run "git cl format" and use the changes it suggests for the parts of the code that you touch or at least look over your code and change the spacing back? https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: bool TrimCandidatePool ( All of these function should be private. They're helper function that no one external to this class needs to know about. Also, when you move their declarations / make them private, remember to correct the .cc. Our style guide requires that the functions in the .cc are implemented in the same order as they are declared in the header file. You may to do this when the review is almost over; otherwise the "diff" between you change and the original code will look large. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:56: ScoredHistoryMatches GetScoredItemsForSearchString( Most of this class calls this parameter a "term_string"; please be consistent with naming the parameter and teh function. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:65: HistoryIDSet HistoryItemsForWords( This function is confusingly named. It sounds a like like "HistoryItemsForTerms" but (i) function HistoryItemsForTerms returns ScoredHistoryMatches; this returns HistoryIDSets. Having them both called HistoryItems... is confusing. (ii) this function takes a term string; not words. (Indeed, in the middle of the function a comment explains the difference in terminology. This function should not be claiming to take "words".) Having this function called "ForWords" is misleading.
Hi Mark, I believed I have made the changes you recommended. Would you mind reviewing my work again. Thanks, Lavar https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:158: HistoryIDSet history_id_set; On 2016/08/10 17:46:19, Mark P wrote: > This variable isn't needed; simply return HistoryIDSet() at the first return > below and return HistoryIDSetFromWords(lower_words) at the second return, no > need to create an intermediate copy within the function. Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:252: // Trim the candidate pool if it is large. Note that we do not filter out On 2016/08/10 17:46:19, Mark P wrote: > I assume this whole block was copied with no modifications. Yes, you are correct. Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:362: //ScoredHistoryMatches for the search string with and without the word break. On 2016/08/10 17:46:19, Mark P wrote: > The code below can be made more efficient and easier to read. Approximately: > if (without_word_break.empty()) > return with_word_break > with_word_break.insert(with_word_break.end(), without.begin(), without.end()) > return with_word_break; > > ( http://stackoverflow.com/a/2208313 is good reading by the way about what I > skipped the reserve. Also this technique doesn't need another variable, no need > to copy all those entries to a third variable.) Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:385: On 2016/08/10 17:46:19, Mark P wrote: > I could imagine situations where the items overlap. You should be removing > duplicates (smartly) here. Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:386: return all_scored_items; On 2016/08/10 17:46:19, Mark P wrote: > Also, the number of items we're supposed to return should be no more than > max_matches. You need to truncate the list if necessary. Right now you may be > returning twice that number. Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: bool TrimCandidatePool ( On 2016/08/10 17:46:20, Mark P wrote: > Here and throughout the change, you introduce a number of style violations. > (Indeed, at times the only thing you did on some lines is to change the spacing > to a way that's not consistent with chromium style.) Can you run "git cl > format" and use the changes it suggests for the parts of the code that you touch > or at least look over your code and change the spacing back? "git cl format" did not make any suggestions for me. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: bool TrimCandidatePool ( On 2016/08/10 17:46:20, Mark P wrote: > This function and the one below should be commented. Done. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:56: ScoredHistoryMatches GetScoredItemsForSearchString( On 2016/08/10 17:46:19, Mark P wrote: > Most of this class calls this parameter a "term_string"; please be consistent > with naming the parameter and teh function. search_string was the name of the variable before I started working on it. It made things clear to me in this method. If you insist that I do that then I will. https://codereview.chromium.org/2187343002/diff/20001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:65: HistoryIDSet HistoryItemsForWords( On 2016/08/10 17:46:19, Mark P wrote: > This function is confusingly named. It sounds a like like > "HistoryItemsForTerms" but > (i) function HistoryItemsForTerms returns ScoredHistoryMatches; this returns > HistoryIDSets. Having them both called HistoryItems... is confusing. > (ii) this function takes a term string; not words. (Indeed, in the middle of > the function a comment explains the difference in terminology. This function > should not be claiming to take "words".) Having this function called "ForWords" > is misleading. > Done.
This is generally very nice. I see you've gotten chromium style down pretty well. :-) I have a few minor comments, mostly about comments and spacing. I think we're almost done here. Can you please add a simple test for this to history_quick_provider_unittest.cc? Probably it should go right next to SingleMatchWithCursor() and check something similar to what you have in your TEST line in the changelist description. thanks, mark https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:229: nit: don't add a blank line https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:268: HistoryIDSet history_id_set_with_word_break; This declaration doesn't need to be at this level; let it live inside the inner if statement. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:278: // check to see if lower_words and lower_words_with_word_break nit: start sentence with capital letter ("Check"...) nit: please wrap the comment closer to 80 characters https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:285: nit: no need for this blank line I think https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:289: nit: no need for this blank line I think https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: // Returns history URL matches This comment doesn't say anything useful. You want a comment that describes what the function does. I'd suggest something in the same style as the comment by HistoryItemsForTerms or something that says, This helper function is much like HistoryItemsForTerms() except ... https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:57: const size_t& max_matches, nit: please remove the & here; it's not useful https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:63: // in the search_string. nit: shorter and clearer: // Returns a vector of individual words from |search_string|, in order of appearance, duplicates allowed. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:67: // Given a base::string16 in |term_string|, scans the history index and nit: omit "base::string16" -- this is obvious from the function declaration. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:68: // returns the set of history item IDs. Will return an emptyHistoryIDSet after IDs, add "containing all the terms in |term_string|" (or something similar)
Hi Mark, I made the formatting changes you asked for, improved the comments, and added the test case. Let me know if there is anything else. Thanks, Lavar https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:229: On 2016/08/18 19:36:53, Mark P wrote: > nit: don't add a blank line Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:268: HistoryIDSet history_id_set_with_word_break; On 2016/08/18 19:36:53, Mark P wrote: > This declaration doesn't need to be at this level; let it live inside the inner > if statement. Can't believe I missed that...Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:278: // check to see if lower_words and lower_words_with_word_break On 2016/08/18 19:36:53, Mark P wrote: > nit: start sentence with capital letter ("Check"...) > nit: please wrap the comment closer to 80 characters Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:53: // Returns history URL matches On 2016/08/18 19:36:53, Mark P wrote: > This comment doesn't say anything useful. You want a comment that describes > what the function does. I'd suggest something in the same style as the comment > by HistoryItemsForTerms or something that says, This helper function is much > like HistoryItemsForTerms() except ... Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:57: const size_t& max_matches, On 2016/08/18 19:36:53, Mark P wrote: > nit: please remove the & here; it's not useful Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:67: // Given a base::string16 in |term_string|, scans the history index and On 2016/08/18 19:36:53, Mark P wrote: > nit: omit "base::string16" -- this is obvious from the function declaration. Done. https://codereview.chromium.org/2187343002/diff/40001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:68: // returns the set of history item IDs. Will return an emptyHistoryIDSet On 2016/08/18 19:36:53, Mark P wrote: > after IDs, add "containing all the terms in |term_string|" > (or something similar) Done.
Again, no big comments here, mostly things I observed where your header file comment doesn't reflect the current function declaration and code. --mark https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:499: TEST_F(HistoryQuickProviderTest, MatchWithAndWithoutCursorWordBreak) { I'm surprised this test passes as-is, as it doesn't look like you added the twitter URL to the list of pages the URL has visited. (See TestURLInfo above.) Did you actually run this test? (build components_unittests, run it with --gtest_filter=HistoryQuick*.*) Also, in addition to making sure this test passes, please double-check that this new test fails without your change. (One easy way to check is to make a copy of the .h and .cc you've changed, remove those changes, build and run the test to see if it passes or fails, and then replace the .h and .cc with your copied files to restore your changes.) https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:509: // Runs an autocomplete query on |text| and checks to see that the returned This function declaration and the one below shouldn't be here. Did you accidentally copy-and-paste them here? https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:244: ScoredHistoryMatches URLIndexPrivateData::HistoryItemsForTerms( Please move this function to where it used to be in this file. (Chromium style says the .h and .cc should have the functions in the same order.) https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:331: // Support functions for HistoryItemsForTerms This whole block (comment and functions) should go above the Cache of search terms declaration. (In chromium style, all functions are listed before all member fields.) https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:336: // 'terms': "colspec=id%20mstone" and "release". Each URL in the users nit: users -> user's https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:337: // history is scored against the a union of the vector of terms. The scored nit: omit "the a union of " https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:338: // hisory matches are returned unordered. This last sentence is wrong. They are returned ordered, though possibly truncated for length. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:351: // Given a |term_string|, scans the history index and returns the set of nit: a |term_string| -> |lower_words| https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:352: // history item IDs. Will return an emptyHistoryIDSet containing all the nit: insert missing word "matching" before "history item IDs" https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:353: // terms in |term_string|. For the last sentence, did you mean: Will return an emptyHistoryIDSet if |lower_words| is empty. ?
Hi Mark, I've added the test and made the minor updates that you've mentioned. Can you please review? Thanks, Lavar https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:499: TEST_F(HistoryQuickProviderTest, MatchWithAndWithoutCursorWordBreak) { On 2016/08/22 18:24:11, Mark P wrote: > I'm surprised this test passes as-is, as it doesn't look like you added the > twitter URL to the list of pages the URL has visited. (See TestURLInfo above.) > Did you actually run this test? (build components_unittests, run it with > --gtest_filter=HistoryQuick*.*) > > Also, in addition to making sure this test passes, please double-check that this > new test fails without your change. (One easy way to check is to make a copy of > the .h and .cc you've changed, remove those changes, build and run the test to > see if it passes or fails, and then replace the .h and .cc with your copied > files to restore your changes.) Can't I just start up the latest release instance of Chrome (on any machine), enter the twitter URL and go through my test sequence of "funtimes" then "fungoodtimes"? When I do this it fails. When I run an instance of Chrome with my new code it works....Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:509: // Runs an autocomplete query on |text| and checks to see that the returned On 2016/08/22 18:24:11, Mark P wrote: > This function declaration and the one below shouldn't be here. Did you > accidentally copy-and-paste them here? Deleted those lines....Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:244: ScoredHistoryMatches URLIndexPrivateData::HistoryItemsForTerms( On 2016/08/22 18:24:11, Mark P wrote: > Please move this function to where it used to be in this file. > (Chromium style says the .h and .cc should have the functions in the same > order.) Done. I hope I got the order right. I moved the signatures down to the private section of the .h file like you asked though. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:331: // Support functions for HistoryItemsForTerms On 2016/08/22 18:24:11, Mark P wrote: > This whole block (comment and functions) should go above the Cache of search > terms declaration. (In chromium style, all functions are listed before all > member fields.) Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:336: // 'terms': "colspec=id%20mstone" and "release". Each URL in the users On 2016/08/22 18:24:11, Mark P wrote: > nit: users -> user's Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:337: // history is scored against the a union of the vector of terms. The scored On 2016/08/22 18:24:11, Mark P wrote: > nit: omit "the a union of " Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:338: // hisory matches are returned unordered. On 2016/08/22 18:24:11, Mark P wrote: > This last sentence is wrong. They are returned ordered, though possibly > truncated for length. Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:351: // Given a |term_string|, scans the history index and returns the set of On 2016/08/22 18:24:11, Mark P wrote: > nit: > a |term_string| > -> > |lower_words| > Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:352: // history item IDs. Will return an emptyHistoryIDSet containing all the On 2016/08/22 18:24:11, Mark P wrote: > nit: insert missing word "matching" before "history item IDs" Done. https://codereview.chromium.org/2187343002/diff/60001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:353: // terms in |term_string|. On 2016/08/22 18:24:11, Mark P wrote: > For the last sentence, did you mean: > Will return an emptyHistoryIDSet if |lower_words| is empty. > ? Done.
On 2016/08/23 20:38:20, Lavar Askew wrote: > Hi Mark, > > I've added the test and made the minor updates that you've mentioned. Can you > please review? Can you please upload the new patchset? It appears you forgot. thanks, mark
Patchset uploaded.
Almost done! Congrats. Just a few comments below. I recommend you run "git cl try" (assuming you're using the git workflow) to have the testing bots run everything. Then once you make the final changes and I approve, you can check the "Commit:" box on the code review page and it will be sent to a queue to be committed when the bots are happy. --mark https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:60: {"https://twitter.com/fungoodtimes", "relatable!", 1, 1, 0}, nit: Please put this at the end of the list. That's just a habit reflecting how the list has grown. https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:500: TEST_F(HistoryQuickProviderTest, MatchWithAndWithoutCursorWordBreak) { On 2016/08/23 20:38:19, Lavar Askew wrote: > On 2016/08/22 18:24:11, Mark P wrote: > > I'm surprised this test passes as-is, as it doesn't look like you added the > > twitter URL to the list of pages the URL has visited. (See TestURLInfo > above.) > > Did you actually run this test? (build components_unittests, run it with > > --gtest_filter=HistoryQuick*.*) > > > > Also, in addition to making sure this test passes, please double-check that > this > > new test fails without your change. (One easy way to check is to make a copy > of > > the .h and .cc you've changed, remove those changes, build and run the test to > > see if it passes or fails, and then replace the .h and .cc with your copied > > files to restore your changes.) > > Can't I just start up the latest release instance of Chrome (on any machine), > enter the twitter URL and go through my test sequence of "funtimes" then > "fungoodtimes"? When I do this it fails. When I run an instance of Chrome with > my new code it works....Done. It's good practice to make sure a test case you write fails before the bug is fixed and passes after it is. (If it doesn't do that, then it might be passing simply because it was written incorrectly.) That said, you're reluctant to try this, I won't hold your feet to the fire (so to speak) over it. https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:187: ScoredHistoryMatches URLIndexPrivateData::GetScoredItemsForSearchString( On 2016/08/23 20:38:19, Lavar Askew wrote: > On 2016/08/22 18:24:11, Mark P wrote: > > Please move this function to where it used to be in this file. > > (Chromium style says the .h and .cc should have the functions in the same > > order.) > > Done. > > I hope I got the order right. I moved the signatures down to the private > section of the .h file like you asked though. Not quite right. The .h file order is fine. Now the .cc needs to correspond with that the .h is. This means putting ExtractIndividualWordVector(), GetHistoryIDSet(), and GetScoredItemsForSearchString() (in that order) immediately after the implementation of URLSchemeIsWhitelisted(). (Right before the line // SearchTermCacheItem -------------------------- .) https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:335: // Given a |lower_words|, scans the history index and returns the set of nit: omit "a" (it reads strange)
Hi Mark, all ready for your discerning eye. Thanks for your help. -Lavar https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/history_quick_provider_unittest.cc (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:60: {"https://twitter.com/fungoodtimes", "relatable!", 1, 1, 0}, On 2016/08/23 22:14:46, Mark P wrote: > nit: Please put this at the end of the list. That's just a habit reflecting how > the list has grown. Done. https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/history_quick_provider_unittest.cc:500: TEST_F(HistoryQuickProviderTest, MatchWithAndWithoutCursorWordBreak) { On 2016/08/23 22:14:46, Mark P wrote: > On 2016/08/23 20:38:19, Lavar Askew wrote: > > On 2016/08/22 18:24:11, Mark P wrote: > > > I'm surprised this test passes as-is, as it doesn't look like you added the > > > twitter URL to the list of pages the URL has visited. (See TestURLInfo > > above.) > > > Did you actually run this test? (build components_unittests, run it with > > > --gtest_filter=HistoryQuick*.*) > > > > > > Also, in addition to making sure this test passes, please double-check that > > this > > > new test fails without your change. (One easy way to check is to make a > copy > > of > > > the .h and .cc you've changed, remove those changes, build and run the test > to > > > see if it passes or fails, and then replace the .h and .cc with your copied > > > files to restore your changes.) > > > > Can't I just start up the latest release instance of Chrome (on any machine), > > enter the twitter URL and go through my test sequence of "funtimes" then > > "fungoodtimes"? When I do this it fails. When I run an instance of Chrome > with > > my new code it works....Done. > > It's good practice to make sure a test case you write fails before the bug is > fixed and passes after it is. (If it doesn't do that, then it might be passing > simply because it was written incorrectly.) > > That said, you're reluctant to try this, I won't hold your feet to the fire (so > to speak) over it. Ah I see...I will add the old code and run again. It failed. Done. https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.cc:187: ScoredHistoryMatches URLIndexPrivateData::GetScoredItemsForSearchString( On 2016/08/23 22:14:46, Mark P wrote: > On 2016/08/23 20:38:19, Lavar Askew wrote: > > On 2016/08/22 18:24:11, Mark P wrote: > > > Please move this function to where it used to be in this file. > > > (Chromium style says the .h and .cc should have the functions in the same > > > order.) > > > > Done. > > > > I hope I got the order right. I moved the signatures down to the private > > section of the .h file like you asked though. > > Not quite right. The .h file order is fine. Now the .cc needs to correspond > with that the .h is. This means putting ExtractIndividualWordVector(), > GetHistoryIDSet(), and GetScoredItemsForSearchString() (in that order) > immediately after the implementation of URLSchemeIsWhitelisted(). > (Right before the line > // SearchTermCacheItem -------------------------- > .) Done. https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/80001/components/omnibox/brow... components/omnibox/browser/url_index_private_data.h:335: // Given a |lower_words|, scans the history index and returns the set of On 2016/08/23 22:14:46, Mark P wrote: > nit: omit "a" (it reads strange) Done.
lgtm looks good to me. Please make the two fixes I suggest in the comments below, upload a new patchset, then click the "Commit" box and cross your fingers. :-) It'll probably take a few hours before the code gets actually committed. --mark https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:157: /* Please remove this large commented out set of functions. You've moved them below. https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.h:53: // Given |term_string|, scans the history index and returns a vector with nit: the "a" here that was before "|term_string|" reads fine. Please put it back. (I think the difference is that |term_string| sounds singular whereas in the other place you put a similar comment, the variable is |lower_words|, which sound plural.)
The CQ bit was checked by open.hyperion@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2187343002/#ps120001 (title: "Removed large comment. Added the 'a' back in.")
The CQ bit was unchecked by open.hyperion@gmail.com
The CQ bit was checked by open.hyperion@gmail.com
Done...and the commit box has been clicked! https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:157: /* On 2016/08/24 20:15:03, Mark P wrote: > Please remove this large commented out set of functions. You've moved them > below. Done. https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.h (right): https://codereview.chromium.org/2187343002/diff/100001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.h:53: // Given |term_string|, scans the history index and returns a vector with On 2016/08/24 20:15:03, Mark P wrote: > nit: the "a" here that was before "|term_string|" reads fine. Please put it > back. > (I think the difference is that |term_string| sounds singular whereas in the > other place you put a similar comment, the variable is |lower_words|, which > sound plural.) Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Looks like there are some other test cases that now have different answers and need to be revised. --mark On Wed, Aug 24, 2016 at 1:46 PM, commit-bot@chromium.org via codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote: > Try jobs failed on following builders: > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/ > builders/ios-simulator/builds/57860) > > https://codereview.chromium.org/2187343002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/08/24 20:49:44, Mark P wrote: > Looks like there are some other test cases that now have different answers > and need to be revised. > > --mark > > > On Wed, Aug 24, 2016 at 1:46 PM, mailto:commit-bot@chromium.org via > http://codereview.chromium.org <mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote: > > > Try jobs failed on following builders: > > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > http://build.chromium.org/p/tryserver.chromium.mac/ > > builders/ios-simulator/builds/57860) > > > > https://codereview.chromium.org/2187343002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Okay, I am not sure how to do that. My guess is that I need to run the iOS simulator? Do you mind giving me some guidance on this? -Lavar
On 2016/08/24 20:54:17, Lavar Askew wrote: > On 2016/08/24 20:49:44, Mark P wrote: > > Looks like there are some other test cases that now have different answers > > and need to be revised. > > > > --mark > > > > > > On Wed, Aug 24, 2016 at 1:46 PM, mailto:commit-bot@chromium.org via > > http://codereview.chromium.org > <mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote: > > > > > Try jobs failed on following builders: > > > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/ > > > builders/ios-simulator/builds/57860) > > > > > > https://codereview.chromium.org/2187343002/ > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "Chromium-reviews" group. > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Okay, I am not sure how to do that. My guess is that I need to run the iOS > simulator? Do you mind giving me some guidance on this? If the test is failing there, it's probably failing on all platforms. I can see from the red things listed on the code review page that it's also failed on linux too. I expect it to fail on Mac and Windows when those bots get around to running it. I think those test cases will fail locally to you too. Please try running those parts of components_unittests on your local machine to investigate. --mark
The CQ bit was checked by open.hyperion@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2187343002/#ps160001 (title: "Correct code causing tests in HistoryQuickProviderTest to fail.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Mark,
I fixed the errors that were causing the tests in HistoryQuickProviderTest to
fail.
However, when I run all 6649 tests I get the following:
[6649/6649] DataReductionProxyIODataTest.TestConstruction (5 ms)
1 test crashed:
DataReductionProxyInterceptorEndToEndTest.RedirectChainToHttps
(../../components/data_reduction_proxy/core/browser/data_reduction_proxy_interceptor_unittest.cc:484)
Any clue on how I should deal with this?
-Lavar
Message was sent while issue was closed.
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. Committed: https://crrev.com/e3235816f23e9eb733d11bdecdfd5f8ca67cec9f Cr-Commit-Position: refs/heads/master@{#418054} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e3235816f23e9eb733d11bdecdfd5f8ca67cec9f Cr-Commit-Position: refs/heads/master@{#418054}
Message was sent while issue was closed.
On Mon, Sep 12, 2016 at 1:54 PM, <open.hyperion@gmail.com> wrote: > Hi Mark, > > I fixed the errors that were causing the tests in HistoryQuickProviderTest > to > fail. > > However, when I run all 6649 tests I get the following: > > [6649/6649] DataReductionProxyIODataTest.TestConstruction (5 ms) > 1 test crashed: > DataReductionProxyInterceptorEndToEndTest.RedirectChainToHttps > (../../components/data_reduction_proxy/core/browser/data_reduction_proxy_ > interceptor_unittest.cc:484) > > Any clue on how I should deal with this? > As you've no doubt guessed, this was a flake. --mark > -Lavar > > https://codereview.chromium.org/2187343002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I started looking at the changelist to see what happened between the reviewed version of the code (patchset 7) and the submitted one. (You'll see a few comments at the bottom of this message.) I thought it likely the problem was with the unit tests. I guess I was wrong; it's clear you did a lot of revisions to the code since I last reviewed it. In general, if the changes are substantial since the last review, it's typical that you go back to the reviewer to ask whether the code is still okay. I'm not sure if many open source projects work this way, but this typical for chromium. In this case, the code is not still okay. In the final version of your code, you've gone back to a version where you have a lot of code duplication between the space and no-space versions, which is something we specifically advised against early in this review. This current version is likely to cause trouble in the future because, if someone updates code here, they're likely to only update it in one place. If you can give me a changelist soon (in a day or two) that fixes these code structure issue, that'd be great. Otherwise I'll revert this change and wait for a new patchset that implements this feature in a way that follows coding best practices. --mark https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (left): https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:84: style comment: if you're trying to change the style by using only one blank line before section breaks in this file, you should do so consistently. Instead, used one here but two in the section (// URLIndexPrivateData) immediately below. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:1254: ditto style comment about single versus double blank lines. Consistency. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:172: The lines above naturally go with the lines below. I wouldn't add a blank line here. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:235: ditto https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:256: // pretending there's a space where the cursor is. Please make this comment normal paragraph style, i.e., wrap closer to 80 characters instead of its current ragged state. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:257: // This phenomena will be referred to as space ghosting. Don't introduce a term ("space ghosting") only to use it once. I think you can remove this sentence and simply revise the last sentence (the single place you use the term).
Message was sent while issue was closed.
https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (left): https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:84: On 2016/09/13 03:58:19, Mark P wrote: > style comment: if you're trying to change the style by using only one blank line > before section breaks in this file, you should do so consistently. Instead, > used one here but two in the section (// URLIndexPrivateData) immediately below. Done. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:172: On 2016/09/13 03:58:19, Mark P wrote: > The lines above naturally go with the lines below. I wouldn't add a blank line > here. Done. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:235: On 2016/09/13 03:58:19, Mark P wrote: > ditto Done. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:256: // pretending there's a space where the cursor is. On 2016/09/13 03:58:19, Mark P wrote: > Please make this comment normal paragraph style, i.e., wrap closer to 80 > characters instead of its current ragged state. Done. https://codereview.chromium.org/2187343002/diff/160001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:257: // This phenomena will be referred to as space ghosting. On 2016/09/13 03:58:19, Mark P wrote: > Don't introduce a term ("space ghosting") only to use it once. I think you can > remove this sentence and simply revise the last sentence (the single place you > use the term). Done.
Message was sent while issue was closed.
Instead of making new methods I put the common code for the search strings into a for loop.
Message was sent while issue was closed.
Hi Mark, were you able to see the latest changes where I've consolidated things down to a single for loop? -Lavar
Message was sent while issue was closed.
This generally looks nice, maybe nicer than any previous version. And, yes, I chuckled at the Space Ghost remark. Reminds me of an old animated television show I haven't thought about in years. --mark https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (left): https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:188: // Do nothing if we have indexed no words (probably because we've not been I think we need to keep this block (except for the lower_word.empty() part of the test. I don't see search_term_cache_ being cleared if word_list_ is empty in your new code. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:244: // Don't score matches when there are no terms to score against. (It's Please keep this comment in the new code. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:166: search_strings.insert(search_strings.end(), original_search_string); nit: push_back is more idiomatic and easier to read. ditto below https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:196: if (!lower_words.empty()) { nit: Instead of indenting this huge block below, I suggest: if (lower_words.empty()) continue; https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:200: // doing nit: formatting https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:204: pre_filter_item_count_ = pre_filter_item_count_ + history_id_set.size(); nit: prefer += https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:226: // that nit: formatting here and in the below paragraph as well https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:252: // Select and sort only the top |max_matches| results. I think this selecting and sorting should go outside the foreach string loop. Also, the post_scoring_item_count_ setting should go with it.
Message was sent while issue was closed.
On 2016/09/15 22:55:54, Mark P wrote: > And, yes, I chuckled at the Space Ghost remark. Reminds me of an old animated > television show I haven't thought about in years. https://youtu.be/qQbt7gnU1HE?t=321
Message was sent while issue was closed.
Recommended changes made. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (left): https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:188: // Do nothing if we have indexed no words (probably because we've not been On 2016/09/15 22:55:54, Mark P wrote: > I think we need to keep this block (except for the lower_word.empty() part of > the test. I don't see search_term_cache_ being cleared if word_list_ is empty > in your new code. Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:244: // Don't score matches when there are no terms to score against. (It's On 2016/09/15 22:55:54, Mark P wrote: > Please keep this comment in the new code. Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:166: search_strings.insert(search_strings.end(), original_search_string); On 2016/09/15 22:55:54, Mark P wrote: > nit: push_back is more idiomatic and easier to read. > ditto below Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:196: if (!lower_words.empty()) { On 2016/09/15 22:55:53, Mark P wrote: > nit: Instead of indenting this huge block below, I suggest: > if (lower_words.empty()) > continue; Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:200: // doing On 2016/09/15 22:55:53, Mark P wrote: > nit: formatting That git cl format command causes more trouble than is solves. Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:204: pre_filter_item_count_ = pre_filter_item_count_ + history_id_set.size(); On 2016/09/15 22:55:54, Mark P wrote: > nit: prefer += Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:226: // that On 2016/09/15 22:55:53, Mark P wrote: > nit: formatting > here and in the below paragraph as well Done. https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:252: // Select and sort only the top |max_matches| results. On 2016/09/15 22:55:53, Mark P wrote: > I think this selecting and sorting should go outside the foreach string loop. > Also, the post_scoring_item_count_ setting should go with it. Done.
Message was sent while issue was closed.
looks good to me once you make the change below You will have to upload this patch as a new issue. This current issue is closed because the change originally reviewed here has been submitted. You might be able to associated your branch with a new issue using a command like "git cl issue". Something like that... I never quite remember what it is exactly and have to look it up. Alternately, you can simply create a new branch on your client and patch this change into the new branch. Once you do that, I'll be happy to approve the change (with the minor revision suggested below) on the new codereview issue. --mark https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/220001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:200: // doing On 2016/09/16 00:37:51, Lavar Askew wrote: > On 2016/09/15 22:55:53, Mark P wrote: > > nit: formatting > That git cl format command causes more trouble than is solves. I agree. https://codereview.chromium.org/2187343002/diff/240001/components/omnibox/bro... File components/omnibox/browser/url_index_private_data.cc (right): https://codereview.chromium.org/2187343002/diff/240001/components/omnibox/bro... components/omnibox/browser/url_index_private_data.cc:179: word_list_.empty() ? search_term_cache_.clear() : ResetSearchTermCache(); 1. Please return early if word_list_ is empty (as in the old behavior), thus skipping lots of the splitting / parsing and (no-op) searching code below. 2. Use a standard if else structure; only use the ternary operator when the return value matters. I guess I'm suggesting: ScoredHistoryMatches scored_items; // Invalidate the term cache and return if we have indexed no words (probably because we've not been initialized yet). if (word_list_.empty()) { search_term_cache_.clear(); return scored_items; } // Reset used_ flags for search_term_cache_. We use a basic mark-and-sweep approach. ResetSearchTermCache();
Message was sent while issue was closed.
Description was changed from ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. Committed: https://crrev.com/e3235816f23e9eb733d11bdecdfd5f8ca67cec9f Cr-Commit-Position: refs/heads/master@{#418054} ========== to ========== Generating autocomplete results with and without word breaks in the Omnibox. The goal is to support mid-word autocomplete in the Omnibox. Currently, if the user types "funtimes", inserts the cursor between the "n" and the "t" and begins typing the word "good" the Ominbox will search for URL results that match "fungood times" only. We want to also search for "fungoodtimes". BUG=591979 TEST=0. Clear browser history. 1. Visit the following link: https://twitter.com/fungoodtimes 2. Open a new browser tab. 3.Type into the Omnibox "funtimes". Note the lack of the suggestion for the above URL. 4. Insert the cursor between the "n" and "t" in "funtime" and type "good". 5. The above URL should show in the autocomplete list. Committed: https://crrev.com/e3235816f23e9eb733d11bdecdfd5f8ca67cec9f Cr-Commit-Position: refs/heads/master@{#418054} ==========
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:240001) has been created in https://codereview.chromium.org/2364523003/ by mpearson@chromium.org. The reason for reverting is: Reverting. Will be replaced by cleaner implementation: https://codereview.chromium.org/2363463002/ --mark. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
