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

Issue 6652008: Implemented substring matching within page titles. Fixed bug where URL was be... (Closed)

Created:
9 years, 9 months ago by mrossetti
Modified:
9 years, 7 months ago
Reviewers:
mrossetti, Peter Kasting, Paweł Hajdan Jr.
CC:
chromium-reviews, brettw-cc_chromium.org
Visibility:
Public.

Description

Implemented substring matching within page titles. Fixed bug where URL was being lower-cased. Added unit tests for page title matching as well as unit tests for various static functions and other areas not previously unit tested. BUG=58958, 57853, 58559, 60107, 75245 TEST=Many unit tests added. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77646

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 40

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+604 lines, -228 lines) Patch
M chrome/browser/autocomplete/history_quick_provider.h View 1 2 3 4 3 chunks +12 lines, -7 lines 0 comments Download
M chrome/browser/autocomplete/history_quick_provider.cc View 1 2 3 4 8 chunks +56 lines, -52 lines 0 comments Download
M chrome/browser/autocomplete/history_quick_provider_unittest.cc View 1 2 3 4 2 chunks +50 lines, -0 lines 0 comments Download
M chrome/browser/history/in_memory_url_index.h View 1 2 3 4 6 chunks +58 lines, -20 lines 0 comments Download
M chrome/browser/history/in_memory_url_index.cc View 1 2 3 4 13 chunks +209 lines, -115 lines 0 comments Download
M chrome/browser/history/in_memory_url_index_unittest.cc View 1 2 3 4 11 chunks +210 lines, -29 lines 0 comments Download
M chrome/test/data/History/url_history_provider_test.db.txt View 1 2 3 4 2 chunks +5 lines, -5 lines 0 comments Download
A chrome/test/data/History/url_history_provider_test_limited.db.txt View 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
mrossetti
9 years, 9 months ago (2011-03-09 01:52:52 UTC) #1
Peter Kasting
You don't mean bug 58753.
9 years, 9 months ago (2011-03-09 01:57:53 UTC) #2
Peter Kasting
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/history_quick_provider.cc File chrome/browser/autocomplete/history_quick_provider.cc (right): http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/history_quick_provider.cc#newcode110 chrome/browser/autocomplete/history_quick_provider.cc:110: AutocompleteMatch::Type match_type = history_match.url_matches.empty() ? Nit: Just inline this ...
9 years, 9 months ago (2011-03-09 02:31:48 UTC) #3
Paweł Hajdan Jr.
Drive-by with testing comments. This is mostly about the gyp change, I added the second ...
9 years, 9 months ago (2011-03-09 10:16:35 UTC) #4
mrossetti_google.com
Fixed. Should have been 57853. On Tue, Mar 8, 2011 at 6:57 PM, <pkasting@chromium.org> wrote: ...
9 years, 9 months ago (2011-03-09 17:01:11 UTC) #5
mrossetti
Ready for another go! Thanks for that great catch! I changed how the 'http://' prefix ...
9 years, 9 months ago (2011-03-10 01:31:14 UTC) #6
Peter Kasting
LGTM http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/history_quick_provider.cc File chrome/browser/autocomplete/history_quick_provider.cc (right): http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/history_quick_provider.cc#newcode190 chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset = matches[i].offset - adjust; On 2011/03/10 ...
9 years, 9 months ago (2011-03-10 02:13:43 UTC) #7
mrossetti
9 years, 9 months ago (2011-03-10 16:10:26 UTC) #8
On 2011/03/10 02:13:43, Peter Kasting wrote:
> LGTM
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
> File chrome/browser/autocomplete/history_quick_provider.cc (right):
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/autocomplete/...
> chrome/browser/autocomplete/history_quick_provider.cc:190: size_t offset =
> matches[i].offset - adjust;
> On 2011/03/10 01:31:14, mrossetti wrote:
> > On 2011/03/09 02:31:48, Peter Kasting wrote:
> > > It's not obvious from the surrounding code that |adjust| must not be
greater
> > > than matches[i].offset.  e.g. What happens when the user types "h", "t",
or
> > "p"?
> > 
> > Ah! Excellent catch! I've modified the scoring function to eliminate
> > consideration of a prefix of 'http://'. Thank you.
> 
> BTW, something you should know about scoring.
> 
> Prefixes like the scheme or an initial "ftp." or "www." matter to scoring.  We
> want to score matches that don't match in those sections better than ones that
> do.  For example, if I type "w", "washingtonmutual.com" is a better match than
> "www.foo.com".
> 
> This detail is important because it prevents "w" or "h" from always
> inline-autocompleting to the most-commonly visited URL (which is generally
> "http://www....").  With Instant on, this is really noticeable.  There's
> currently a bug against the HistoryURLProvider where we're not rigorous enough
> about taking that into account.
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
> File chrome/browser/history/in_memory_url_index.cc (right):
> 
>
http://codereview.chromium.org/6652008/diff/6001/chrome/browser/history/in_me...
> chrome/browser/history/in_memory_url_index.cc:458:
> words.push_back(iter.GetString());
> On 2011/03/10 01:31:14, mrossetti wrote:
> > On 2011/03/09 02:31:48, Peter Kasting wrote:
> > > I confess, I'm a little confused why one arm of the conditional does this
> but
> > > the other doesn't.
> > 
> > The iterator behaves differently depending on the breaking strategy, which
was
> a
> > surprise to me, too. The BreakIterator unit tests do not properly test this
> case
> > as its basic word and space tests always use a test string starting with a
> > space; I verified this behavior through experimentation and implementing
> > specific unit tests in in_memory_url_index_unittest.cc. And I was loathe to
> > modify the behavior of BreakIterator as it would touch a lot of code.
> 
> If BreakIterator should be changed, can you file a bug for that so we can at
> least do it separately?
> 
> If not, maybe add some comments in here?

I'm going to jostle the BreakIterator unit tests this morning to see what ought
to be done, write up a bug from that effort, and also add a TODO in here to come
back and update this if changes are made to the iterator.

Powered by Google App Engine
This is Rietveld 408576698