Omnibox: Create Field Trial for HQP to Ignore Mid-Word Matches

Omnibox: Create Field Trial for HQP to Ignore Mid-Word Matches

Adds code to HistoryQuick provider to make it optionally only consider terms when they match at the beginning of a word boundary in the URL or titles.  The current behavior allows terms to match anywhere regardless of word boundaries.

Then, creates a field trial to control this behavior.  Enables the trial for 25% of users.

BUG=161911
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170784

[Mark P, 2012-11-29 00:46:59]

Hi Peter,

I've heard a couple of complaints about this style matching recently.  Let's see
if it's a net positive or not.

I'm hoping that "new scoring" has a better solution to this (a better measuring
of the quality of a match), but this might be a good stop-gap measure until I
manage to tune new scoring so it's actually better than current HQP scoring.

Please take a look.

thanks,
mark

[Bart N., 2012-11-29 17:32:26]

FYI

A few minor comments. Also, I wonder if you have some idea about impact of this
particular change, I'd be interested to know?

https://codereview.chromium.org/11421139/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_field_trial.cc (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_field_trial.cc:211: trial =
base::FieldTrialList::FactoryGetFieldTrial(
Out of curiosity, why isn't important to make sure that this particular field
trial does not overlap with your new scoring field trial created above? Wouldn't
be better to add a new group (or reuse part of its control) to the existing
field trial?

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.cc:370: void
ScoredHistoryMatch::MakeTermMatchesOnlyAtWordBoundaries(
I think this particular method is worth a unit test, as it's non trivial.

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:45: static int
ScoreComponentForMatches(const TermMatches& matches,
matches -> provided_matches to make it consistent with your implementation.

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:150: // Allows us to determing
setting for use_new_scoring_ and other startup
-> |use_new_scoring| (extra _)?

[Mark P, 2012-11-29 19:36:48]

> A few minor comments.

Made changes as requested.

> Also, I wonder if you have some idea about impact of this
> particular change, I'd be interested to know?

I tried it on a bunch of example queries.  I found it hard it to find
differences.  The difference in scores is minimal on both single and multiple
word inputs.  The only time I notice a qualitative difference is when a result
is dropped.  This only happens when every term has only mid-word matches.  In
other words, a URL is kept when at least one term has at least one match at the
beginning of a word.

--mark

https://codereview.chromium.org/11421139/diff/1/chrome/browser/autocomplete/a...
File chrome/browser/autocomplete/autocomplete_field_trial.cc (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/autocomplete/a...
chrome/browser/autocomplete/autocomplete_field_trial.cc:211: trial =
base::FieldTrialList::FactoryGetFieldTrial(
On 2012/11/29 17:32:26, Bart N. wrote:
> Out of curiosity, why isn't important to make sure that this particular field
> trial does not overlap with your new scoring field trial created above?
Wouldn't
> be better to add a new group (or reuse part of its control) to the existing
> field trial?

The new scoring field trial is currently disabled.  (0% in the experiment
group.)

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.cc:370: void
ScoredHistoryMatch::MakeTermMatchesOnlyAtWordBoundaries(
On 2012/11/29 17:32:26, Bart N. wrote:
> I think this particular method is worth a unit test, as it's non trivial.

Done.

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:45: static int
ScoreComponentForMatches(const TermMatches& matches,
On 2012/11/29 17:32:26, Bart N. wrote:
> matches -> provided_matches to make it consistent with your implementation.

Done.

https://codereview.chromium.org/11421139/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:150: // Allows us to determing
setting for use_new_scoring_ and other startup
On 2012/11/29 17:32:26, Bart N. wrote:
> -> |use_new_scoring| (extra _)?

Rephrased entirely.

[Bart N., 2012-11-29 20:14:08]

lgtm

Thanks for adding the tests.

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:371: const TermMatches& matches,
Looking at the unit tests, I've realized that this method relies on sorted
matches by position. If so, mind mentioning it in the header?

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:378: end_word_starts =
word_starts.end();
I wouldn't cache end(), since this is all template based and will be nicely
optimized by the compiler. Also, perhaps it makes sense to return early here in
case word_starts.empty() returns true?

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match_unittest.cc:80:
TEST_F(ScoredHistoryMatchTest, MakeTermMatchesOnlyAtWordBoundaries) {
Thank you!

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match_unittest.cc:130: EXPECT_EQ(2u,
matches_at_word_boundaries.size());
Would it work something like:
EXPECT_EQ(TermMatch(1, 6, 3), matches_at_word_boundaries[0])?

[Mark P, 2012-11-29 22:45:00]

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:371: const TermMatches& matches,
On 2012/11/29 20:14:08, Bart N. wrote:
> Looking at the unit tests, I've realized that this method relies on sorted
> matches by position. If so, mind mentioning it in the header?

Added a comment to this header by this function and some others in this class
that have the same unmentioned assumption.

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:378: end_word_starts =
word_starts.end();
On 2012/11/29 20:14:08, Bart N. wrote:
> I wouldn't cache end(), since this is all template based and will be nicely
> optimized by the compiler.

Okay.

> Also, perhaps it makes sense to return early here in
> case word_starts.empty() returns true?

Not worth bothering.  It's not a case that'll happen.  We will never have an
empty word_starts when we have term_matches.  (and this loop terminates
immediately when we have no term matches.)

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/11421139/diff/5001/chrome/browser/history/sco...
chrome/browser/history/scored_history_match_unittest.cc:130: EXPECT_EQ(2u,
matches_at_word_boundaries.size());
On 2012/11/29 20:14:08, Bart N. wrote:
> Would it work something like:
> EXPECT_EQ(TermMatch(1, 6, 3), matches_at_word_boundaries[0])?

Not without adding an == operator to that class.  It's straightforward to do. 
It makes me wonder if there's a reason it was left out.  Let's wait for Peter's
opinion.

[Peter Kasting, 2012-12-01 01:21:44]

Completely dropping mid-word matches seems drastic.  Wouldn't it be better to
give them lower scores?  Perhaps we should be running some sort of multi-bucket
trial that tries giving mid-word matches scores ranging from 0 (equivalent to
this patch) up to their current value (which it looks like is equal to
beginning-of-word matches, but I didn't look closely).

LGTM on the implementation.

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:306: provided_matches,
word_starts, &matches_at_word_boundaries);
Nit: I'd slightly prefer to wrap after arg 2 and indent even

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:377:
std::vector<size_t>::const_iterator next_word_starts = word_starts.begin();
Nit: Is there a typedef we should be using here?

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:45: // |provided_matches| should
already be sorted and de-dupped, and
Nit: dupped -> duped (multiple places)

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:47: static int
ScoreComponentForMatches(const TermMatches& provided_matches,
Nit: |provided_matches| here versus |matches| in the next function should be
named identically

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:54: // |matches| should already be
sorted and de-dupped, and ||word_starts|
Nit: || -> |

[Mark P, 2012-12-01 01:54:53]

On 2012/12/01 01:21:44, Peter Kasting wrote:
> Completely dropping mid-word matches seems drastic.  Wouldn't it be better to
> give them lower scores?  Perhaps we should be running some sort of
multi-bucket
> trial that tries giving mid-word matches scores ranging from 0 (equivalent to
> this patch) up to their current value (which it looks like is equal to
> beginning-of-word matches, but I didn't look closely).

I guess you weren't on the bug thread that discussed this at length.  (I can't
find it now; crbug seems to be down for me.)  Here's the quick summary:

I agree with you that this seems drastic and is likely to be negative.  Yet, it
keeps coming up, sometimes by important people (e.g., a VP at Google).  I'd like
a better answer proving it's bad than "I think it will be bad."

I entirely agree a gentler approach would be good.  However, I'm not going to
invest time in tuning old HQP scoring while I'm trying to replace it with new
scoring.  New scoring already has a gentler approach.  I'd rather spend energy
tuning new scoring, which has a more solid conceptual foundation, then tinkering
with something I'm trying to get rid of anyway.

--mark



> 
> LGTM on the implementation.
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> File chrome/browser/history/scored_history_match.cc (right):
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> chrome/browser/history/scored_history_match.cc:306: provided_matches,
> word_starts, &matches_at_word_boundaries);
> Nit: I'd slightly prefer to wrap after arg 2 and indent even
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> chrome/browser/history/scored_history_match.cc:377:
> std::vector<size_t>::const_iterator next_word_starts = word_starts.begin();
> Nit: Is there a typedef we should be using here?
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> File chrome/browser/history/scored_history_match.h (right):
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> chrome/browser/history/scored_history_match.h:45: // |provided_matches| should
> already be sorted and de-dupped, and
> Nit: dupped -> duped (multiple places)
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> chrome/browser/history/scored_history_match.h:47: static int
> ScoreComponentForMatches(const TermMatches& provided_matches,
> Nit: |provided_matches| here versus |matches| in the next function should be
> named identically
> 
>
https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
> chrome/browser/history/scored_history_match.h:54: // |matches| should already
be
> sorted and de-dupped, and ||word_starts|
> Nit: || -> |

[Mark P, 2012-12-01 01:55:32]

Oops, forgot to publish comments to Peter.

--mark

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:306: provided_matches,
word_starts, &matches_at_word_boundaries);
On 2012/12/01 01:21:44, Peter Kasting wrote:
> Nit: I'd slightly prefer to wrap after arg 2 and indent even

Done.

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.cc:377:
std::vector<size_t>::const_iterator next_word_starts = word_starts.begin();
On 2012/12/01 01:21:44, Peter Kasting wrote:
> Nit: Is there a typedef we should be using here?

Done.

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:45: // |provided_matches| should
already be sorted and de-dupped, and
On 2012/12/01 01:21:44, Peter Kasting wrote:
> Nit: dupped -> duped (multiple places)

Fixed three locations.

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:47: static int
ScoreComponentForMatches(const TermMatches& provided_matches,
On 2012/12/01 01:21:44, Peter Kasting wrote:
> Nit: |provided_matches| here versus |matches| in the next function should be
> named identically

Done.

Also rename variable in the implementation.

https://codereview.chromium.org/11421139/diff/4003/chrome/browser/history/sco...
chrome/browser/history/scored_history_match.h:54: // |matches| should already be
sorted and de-dupped, and ||word_starts|
On 2012/12/01 01:21:44, Peter Kasting wrote:
> Nit: || -> |

Done.

[Mark P, 2012-12-01 01:55:49]

Scott,

Can I get your OWNERS stamp?

thanks,
mark

[Peter Kasting, 2012-12-01 02:43:00]

On 2012/12/01 01:54:53, Mark P wrote:
> I agree with you that this seems drastic and is likely to be negative.  Yet,
it
> keeps coming up, sometimes by important people (e.g., a VP at Google).  I'd
like
> a better answer proving it's bad than "I think it will be bad."
> 
> I entirely agree a gentler approach would be good.  However, I'm not going to
> invest time in tuning old HQP scoring while I'm trying to replace it with new
> scoring.  New scoring already has a gentler approach.  I'd rather spend energy
> tuning new scoring, which has a more solid conceptual foundation, then
tinkering
> with something I'm trying to get rid of anyway.

If said VP is Linus/Sundar, he should understand either "I think this will suck"
or "I'm not going to bother testing this yet since I'm working on a new scoring
model that's better anyway".

If it's somebody else, then I kindly wonder why they are sticking their noses in
Chrome's omnibox scoring model.  Suggestions of improvements are welcome, but if
we actually have to spend time digging up data just to make some random person
happy we're wasting our time.

[sky, 2012-12-03 15:36:11]

LGTM

[commit-bot: I haz the power, 2012-12-03 17:15:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11421139/2003

[commit-bot: I haz the power, 2012-12-03 18:56:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mpearson@chromium.org/11421139/2003

[commit-bot: I haz the power, 2012-12-03 19:16:02]

Change committed as 170784