Boost HQP Scores for Bookmarks

Boost HQP Scores for Bookmarks

During the final scoring process, in the ScoredHistoryMatch ctor, determine how many bookmarks reference a match's URL and boost the score if any.

BUG=109533
TEST=New unit tests added.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=155106

[mrossetti, 2012-09-01 02:30:21]

Mark, would you like to propose an initial score adjustment based on the
bookmark_count as shown in the ScoredHistoryMatch constructor in
scored_history_match.cc:114-121?

[Mark P, 2012-09-04 17:07:40]

On 2012/09/01 02:30:21, mrossetti wrote:
> Mark, would you like to propose an initial score adjustment based on the
> bookmark_count as shown in the ScoredHistoryMatch constructor in
> scored_history_match.cc:114-121?

Without having had a chance to experiment with this, I stand by my original
guess:
add
+ ((bookmark_count > 0) ? 10 : 0)
to the typed count
(lines 129 and 164)

--mark

[mrossetti, 2012-09-05 01:45:17]

This gives a boost to an HQP result if the result's URL is also referenced by a
bookmark.

[mrossetti, 2012-09-05 18:28:55]

Okay, this is now ready for review.

[Peter Kasting, 2012-09-05 19:06:30]

LGTM

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
chrome/browser/history/url_index_private_data.cc:24: // TODO(mrossetti): Remove
these two includes one BookmarkService is available.
Nit: one -> once

I'm not actually sure what this comment means, or the similar one below.

[mrossetti, 2012-09-05 20:15:43]

- mpearson, pkasting
+ sky

I need an owner's LG please Scott.

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
File chrome/browser/history/url_index_private_data.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
chrome/browser/history/url_index_private_data.cc:24: // TODO(mrossetti): Remove
these two includes one BookmarkService is available.
Done. At some point, there will be an accessor to retrieve the BookmarkService
from the Profile. This note will remind me to fetch the BookmarkService instead
of the BookmarkModel once that happens.

On 2012/09/05 19:06:30, Peter Kasting wrote:
> Nit: one -> once
> 
> I'm not actually sure what this comment means, or the similar one below.

[sky, 2012-09-05 20:57:49]

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
File chrome/browser/history/in_memory_url_index.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.cc:61: Profile* profile,
Are you using this on the db thread?

[mrossetti, 2012-09-05 22:35:40]

Perfect comment Scott! Made me realize there was a vastly simpler approach.
Thanks!

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
File chrome/browser/history/in_memory_url_index.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/4002/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.cc:61: Profile* profile,
On 2012/09/05 20:57:49, sky wrote:
> Are you using this on the db thread?

No, it was not referenced on the db thread, however I simplified things
dramatically to remove even this dubious usage. I think you'll be pleased.

[Peter Kasting, 2012-09-05 22:50:06]

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
File chrome/browser/history/in_memory_url_index.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.cc:168:
BookmarkModelFactory::GetForProfile(profile_);
Nit: Or inline into next statement

[sky, 2012-09-05 23:00:18]

LGTM

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:131:
~BookmarkServiceMock() {}
virtual

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:134: bool
IsBookmarked(const GURL& url);
virtual and OVERRIDE on these.

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:142: };
DISALLOW_...

[mrossetti, 2012-09-06 00:33:44]

Thanks for the reviews!

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
File chrome/browser/history/in_memory_url_index.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/in_memory_url_index.cc:168:
BookmarkModelFactory::GetForProfile(profile_);
On 2012/09/05 22:50:06, Peter Kasting wrote:
> Nit: Or inline into next statement

Done.

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
File chrome/browser/history/scored_history_match_unittest.cc (right):

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:131:
~BookmarkServiceMock() {}
On 2012/09/05 23:00:18, sky wrote:
> virtual

Done.

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:134: bool
IsBookmarked(const GURL& url);
On 2012/09/05 23:00:18, sky wrote:
> virtual and OVERRIDE on these.

Done.

https://chromiumcodereview.appspot.com/10909046/diff/9003/chrome/browser/hist...
chrome/browser/history/scored_history_match_unittest.cc:142: };
On 2012/09/05 23:00:18, sky wrote:
> DISALLOW_...

Done.