Omnibox Field Trial: Make HQP Score Bookmarks More Highly

Omnibox Field Trial: Make HQP Score Bookmarks More Highly

Creates a field trial to experiment with giving a bonus to
untyped visits to pages that are bookmarked.  I imagine counting
each visit as 5 visits.  Typed visits can easily swamp visits to
bookmarks.  (Typed visits count 20x.)

Also cleaned up a few comments in the header to refer to
variables with their _ names, not the earlier names (before I
converted ScoredHistoryMatch from a struct to a class).

TBR=sky

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

[Mark P, 2013-11-09 00:38:07]

Peter,

What do you think of this approach?  If you're okay with experimenting with this
method of boosting bookmarks, I'll continue by testing this (both interactively
and via unit tests).

--mark

[Peter Kasting, 2013-11-09 00:44:55]

Seems worth a shot.

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

https://codereview.chromium.org/67553002/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.cc:30: int
ScoredHistoryMatch::bookmark_value = -1;
Maybe this should be 1.

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

https://codereview.chromium.org/67553002/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:96: // Sets bookmark_value based
on the field trial state.
Nit: Use pipes on member name since it isn't otherwised referenced here

[Mark P, 2013-11-25 21:11:57]

Hi Peter,

I think this is ready for a real review.  It's code complete.

thanks,
mark

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

https://codereview.chromium.org/67553002/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.cc:30: int
ScoredHistoryMatch::bookmark_value = -1;
On 2013/11/09 00:44:55, Peter Kasting wrote:
> Maybe this should be 1.

Right you are.  Done.

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

https://codereview.chromium.org/67553002/diff/1/chrome/browser/history/scored...
chrome/browser/history/scored_history_match.h:96: // Sets bookmark_value based
on the field trial state.
On 2013/11/09 00:44:55, Peter Kasting wrote:
> Nit: Use pipes on member name since it isn't otherwised referenced here

Done.

[Peter Kasting, 2013-11-26 02:33:48]

LGTM

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:36: int
ScoredHistoryMatch::bookmark_value_ = -1;
Thought you were changing this to 1?

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:45: InitializeBookmarkValue();
Nit: Is there a lot of value in having multiple short InitializeXXX() functions?
 It seems like maybe we should just have one Init() function (optionally named
InitIfNecessary):

void Init() {
  if (initialized_)
    return;
  ... perform all necessary static initialization
}

This results in less boilerplate in the two constructors.

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:505: if (bookmarked &&
(bookmark_value_ > value_of_transition))
Nit: Simpler?:

    if (bookmarked)
      value_of_transition = std::max(value_of_transition, bookmark_value_);

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.h:69:
FRIEND_TEST(ScoredHistoryMatchTest, ScoringBookmarks);
Nit: Any way to add accessors to the base test class to avoid this additional
FRIEND_TEST line?

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match_unittest.cc:178: class
BookmarkServiceMock : public BookmarkService {
Nit: I tend to prefer classes to all be at the top of the test file, with
dividers, leaving one section for all the test cases; but up to you

[Mark P, 2013-11-26 19:31:56]

All changes as suggested.  I'll CQ it now, but if you have more comments feel
free to uncheck the box or tell me them anyway and I'll do it in a followup
changelist.

--mark

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:36: int
ScoredHistoryMatch::bookmark_value_ = -1;
On 2013/11/26 02:33:48, Peter Kasting wrote:
> Thought you were changing this to 1?

I did!  I must've lost the change while rebasing.  Changed again.

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:45: InitializeBookmarkValue();
On 2013/11/26 02:33:48, Peter Kasting wrote:
> Nit: Is there a lot of value in having multiple short InitializeXXX()
functions?

No, there's practically no value.

>  It seems like maybe we should just have one Init() function (optionally named
> InitIfNecessary):
> 
> void Init() {
>   if (initialized_)
>     return;
>   ... perform all necessary static initialization
> }
> 
> This results in less boilerplate in the two constructors.

Done.

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:505: if (bookmarked &&
(bookmark_value_ > value_of_transition))
On 2013/11/26 02:33:48, Peter Kasting wrote:
> Nit: Simpler?:
> 
>     if (bookmarked)
>       value_of_transition = std::max(value_of_transition, bookmark_value_);

I don't care either way, so I changed it as you suggested.

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.h (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.h:69:
FRIEND_TEST(ScoredHistoryMatchTest, ScoringBookmarks);
On 2013/11/26 02:33:48, Peter Kasting wrote:
> Nit: Any way to add accessors to the base test class to avoid this additional
> FRIEND_TEST line?

Yes.  Done.  (Not that I think we should bother adding much code in order to
avoid FRIEND_TEST lines?  I don't see much harm in them.)

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://codereview.chromium.org/67553002/diff/150001/chrome/browser/history/s...
chrome/browser/history/scored_history_match_unittest.cc:178: class
BookmarkServiceMock : public BookmarkService {
On 2013/11/26 02:33:48, Peter Kasting wrote:
> Nit: I tend to prefer classes to all be at the top of the test file, with
> dividers, leaving one section for all the test cases; but up to you

Because there is only one test using this Mock, I prefer to keep them together. 
If I add another, I'll do as you suggest (because then the mock becomes more
relevant to the test file as a whole).

[commit-bot: I haz the power, 2013-11-26 19:33:02]

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

[commit-bot: I haz the power, 2013-11-26 19:50:12]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-26 19:55:35]

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

[commit-bot: I haz the power, 2013-11-26 22:34:43]

Change committed as 237417