Autocomplete suggestions for bookmark TitleMatch's does not order matching bo...

Autocomplete suggestions for bookmark TitleMatch's does not order matching bookmark results by anything other than their position in the BookmarkIndex. When your search term matches many bookmarks in a particular BookmarkIndex entry, you may not see your most used bookmarks rather rare and obscure bookmarks that may have only been visited once or twice.

BUG=16230
TEST=Use a clean profile and import Firefox bookmarks and history. Set your omnibox-popup-count to say 30 to see more results in the autocomplete. Type in a term to the Firefox location bar and see what results you get back. Firefox uses a "frecency" algorithm that prioritizes the more frequently accessed and more recent matches. Now type in the same term to the Omnibox and you'll probably see some rarely used bookmarks that only show because of their position in the bookmark index.

[pierre.lafayette, 2009-08-13 15:03:33]

Hi Peter, 

The goal of this patch is to sort TitleMatch's retrieved under the same
BookmarkIndex entry by something other than simply the URL's position in the
entry's associated list. For this version of the patch I simply used visit
count. This way bookmarks I use more often that match my location bar input will
be prioritized.

This patch is a rough (but working) draft and I was hoping that you can point me
in the right direction. I'm slightly confused about the synchronous vs
asynchronous usage, in particular the block at line 118 that retrieves history
info for the results but yet doesn't seem to be using the info it retrieves (or
does it?). I'm guessing that my SortMatches() call should probably be in an
asynchronous only block and the history retrieval should be done similarly to
the 118 block(?). When I retrieve the historic info for the matches I could use
it to populate the URLResult's thus removing the need for the final QueryHistory
call.

If I'm completely off and not doing this the right way, let me know. However,
the bug itself does exist. As you'll see if you do the TEST.

Thanks.

[Peter Kasting, 2009-08-13 18:14:01]

Scott needs to look at this too.  Added him to reviewer list.

It looks to me like the existing code doesn't rank the bookmark results _at
all_, which is certainly a bug.  Scott should tell me whether I'm correct. 
However, I don't think this patch solves that how we'd want.

On 2009/08/13 15:03:33, pierre.lafayette wrote:
> The goal of this patch is to sort TitleMatch's retrieved under the same
> BookmarkIndex entry

I don't know what "under the same BookmarkIndex entry" means.

> For this version of the patch I simply used visit
> count. This way bookmarks I use more often that match my location bar input
will
> be prioritized.

This isn't what you want.  You want similar sort criteria to how the history URL
provider ranks, I believe, i.e. typed_count first.  I'm not sure how to work
match_positions into this algorithm.

The reason for valuing typed_count highest is twofold: one, the user is typing
in the box, so they're much more likely to want to type URL's they've typed
before; and two, it allows you to have stability in your results between the
synchronous and asynchronous passes, since that way a match from only the
asynchronous pass will never score above the matches available synchronously.

I wonder if we could factor a the ranking code (probably _not_ the scoring code)
out of HistoryURLProvider into a place both classes can use it.

I also wonder if we could make our scoring here less blocky and more like a
continuous function, so that other than not being eligible for inline
autocompletion, content matches would intermix with URL matches better.  We
could probably do that as a second patch though.

> I'm slightly confused about the synchronous vs
> asynchronous usage, in particular the block at line 118 that retrieves history
> info for the results but yet doesn't seem to be using the info it retrieves
(or
> does it?).

It does, note how it provides the QueryHistory call the member QueryComplete
function to call back when it's finished.

> I'm guessing that my SortMatches() call should probably be in an
> asynchronous only block and the history retrieval should be done similarly to
> the 118 block(?). When I retrieve the historic info for the matches I could
use
> it to populate the URLResult's thus removing the need for the final
QueryHistory
> call.

Yes, your use of the in-memory DB here is only going to find you historical data
on bookmarked URLs that were typed.

Doing the Right Thing here is going to be hairy, I suspect.

[pierre.lafayette, 2009-08-13 19:52:00]

On 2009/08/13 18:14:01, Peter Kasting wrote:
> On 2009/08/13 15:03:33, pierre.lafayette wrote:
> > The goal of this patch is to sort TitleMatch's retrieved under the same
> > BookmarkIndex entry
> 
> I don't know what "under the same BookmarkIndex entry" means.

By BookmarkIndex entry, I meant the "key" term that we use to look up the
associated URLs. So I was referring to all URLs associated with that index key.

E.g.
(goo) -> { Goo<www.goo.com>, World of Goo<www.worldofgoo.com>, ... }
(google) -> { Google<www.google.com>, Google Code<code.google.com>, ...}

At least in the case of the Firefox import, these URLs associated with a
BookmarkIndex are in the order of their position in the moz_bookmarks database
(see Firefox3Importer::GetWholeBookmarkFolder()).

[sky, 2009-08-18 18:16:44]

Sorry for the delay in responding, was on vacation.

I spoke with Peter and others about this. Here's what you should do. Make the
bookmark index query the in memory db and sort based on type count. You can use
the GetRowForURL method and the URLRow has the typed count in it.

  -Scott

On 2009/08/13 15:03:33, pierre.lafayette wrote:
> Hi Peter, 
> 
> The goal of this patch is to sort TitleMatch's retrieved under the same
> BookmarkIndex entry by something other than simply the URL's position in the
> entry's associated list. For this version of the patch I simply used visit
> count. This way bookmarks I use more often that match my location bar input
will
> be prioritized.
> 
> This patch is a rough (but working) draft and I was hoping that you can point
me
> in the right direction. I'm slightly confused about the synchronous vs
> asynchronous usage, in particular the block at line 118 that retrieves history
> info for the results but yet doesn't seem to be using the info it retrieves
(or
> does it?). I'm guessing that my SortMatches() call should probably be in an
> asynchronous only block and the history retrieval should be done similarly to
> the 118 block(?). When I retrieve the historic info for the matches I could
use
> it to populate the URLResult's thus removing the need for the final
QueryHistory
> call.
> 
> If I'm completely off and not doing this the right way, let me know. However,
> the bug itself does exist. As you'll see if you do the TEST.
> 
> Thanks.

[Peter Kasting, 2009-08-18 18:49:20]

On 2009/08/18 18:16:44, sky wrote:
> I spoke with Peter and others about this. Here's what you should do. Make the
> bookmark index query the in memory db and sort based on type count. You can
use
> the GetRowForURL method and the URLRow has the typed count in it.

Clarity: Scott and I suggest this because it is easily doable without mangling
the existing code much.  It's not an optimal fix; untyped bookmarks are still
going to be sorted pseudo-randomly.  But fixing that is much more difficult.  So
we hope to see how well this partial solution works.

[pierre.lafayette, 2009-08-19 00:11:00]

I've moved the sort to the bookmark index. Let me know if this is not what you
meant. I still left the match_positions logic since a search for Google Reader
shouldn't match google.com because it was typed more often. Thoughts?

[Peter Kasting, 2009-08-19 02:01:51]

On Tue, Aug 18, 2009 at 5:11 PM, <pierre.lafayette@gmail.com> wrote:

> I've moved the sort to the bookmark index. Let me know if this is not
> what you meant.


I'm torn between wanting the autocomplete code to be the only consumer of
the in-memory DB (and I forget how that gets initted, we'll need to check to
ensure using it two places doesn't need any magic) and feeling like the
bookmark code should sort its own stuff.

I still left the match_positions logic since a search
> for Google Reader shouldn't match google.com because it was typed more
> often.


I don't really know what the match_positions value means, Scott should check
that.

I think your comparator has way more boilerplate than it needs.  A simple
boolean function would work just as well.  See other files in the
autocomplete system.

http://codereview.chromium.org/165455
>

[pierre.lafayette, 2009-08-19 06:49:36]

On 2009/08/19 02:01:51, Peter Kasting wrote: 
> I don't really know what the match_positions value means, Scott should check
that.

The match_positions is a vector of size_t pairs which represent the [begin, end)
positions of a matched user input term in the bookmark title. More pairs in the
vector means that more input terms were matched (i.e. more words the user typed
match the title of the bookmark match).

> I think your comparator has way more boilerplate than it needs.  A simple
> boolean function would work just as well.  See other files in the
> autocomplete system.

Agreed. I was following the style of bookmark_model.cc and the SortComparator.

[sky, 2009-08-19 14:20:59]

In addition to the following you need to add tests for coverage of this code.

http://codereview.chromium.org/165455/diff/2056/3049
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2056/3049#newcode279
Line 279: DCHECK_EQ(results_.size(), 0);
0 should be first.

http://codereview.chromium.org/165455/diff/2056/3050
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/2056/3050#newcode57
Line 57: 
We should get the typed count here, then sort. That way AddMatchToResults need
only iterate over max_count, rather than having to iterate over all the matches.

http://codereview.chromium.org/165455/diff/2056/3050#newcode71
Line 71: results->erase(results->begin(),
results->resize(max_count);

http://codereview.chromium.org/165455/diff/2056/3050#newcode99
Line 99: return (a.match_positions.size() < b.match_positions.size()) ||
typed_count is the primary thing we care about here. For example, if I'm
searching on 'foo' a bookmark that I've typed 10 times with the title 'foo' is
more relevant then the bookmark I've typed 1 times with the title 'foo foo'. In
fact as the search is an and search, you can nuke the match_positions.size()
constraint.

http://codereview.chromium.org/165455/diff/2056/3051
File chrome/browser/bookmarks/bookmark_index.h (right):

http://codereview.chromium.org/165455/diff/2056/3051#newcode45
Line 45: Profile* profile,
As a BookmarkIndex is tied to one Profile, the Profile should be set in the
constructor rather than taken as an arg.

http://codereview.chromium.org/165455/diff/2056/3053
File chrome/browser/bookmarks/bookmark_model.h (left):

http://codereview.chromium.org/165455/diff/2056/3053#oldcode8
Line 8: #include "build/build_config.h"
This is the standard place we put this include.

[pierre.lafayette, 2009-08-19 20:38:45]

Updated.

http://codereview.chromium.org/165455/diff/2056/3050
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/2056/3050#newcode71
Line 71: results->erase(results->begin(),
On 2009/08/19 14:20:59, sky wrote:
> results->resize(max_count);

The entries at the end of the results have higher typed count. So we want to
remove from the front of the vector since the relevance increments for each
entry and that's what's used for the final ordering of the results.

[Peter Kasting, 2009-08-19 20:40:39]

http://codereview.chromium.org/165455/diff/2056/3050
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/2056/3050#newcode71
Line 71: results->erase(results->begin(),
On 2009/08/19 20:38:45, pierre.lafayette wrote:
> On 2009/08/19 14:20:59, sky wrote:
> > results->resize(max_count);
> 
> The entries at the end of the results have higher typed count. So we want to
> remove from the front of the vector since the relevance increments for each
> entry and that's what's used for the final ordering of the results.

Then reverse your ordering function.  Removing things from the front of a vector
sucks.  And when we get the list back in the history_url_provider code we'll
want it best-first anyway.

[sky, 2009-08-19 21:15:17]

Mostly there. Don't forget unit tests.

http://codereview.chromium.org/165455/diff/3055/2065
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/3055/2065#newcode279
Line 279: DCHECK_EQ(0, results_.size());
Sorry, on looking at this again it should be:

DCHECK(results_.empty());

http://codereview.chromium.org/165455/diff/3055/2066
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/3055/2066#newcode77
Line 77: profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
You'll likely need to deal with a null profile_, which happens during testing.
Deal with this by adding all to node_typed_counts with 0 for the typed count.
You should treat a null history_service and in memory db the same way.

nit: 4 space indent.

http://codereview.chromium.org/165455/diff/3055/2066#newcode108
Line 108: history::URLID url_id = url_db->GetRowForURL((*i)->GetURL(), &url);
There's no point in assigning the return value as you don't use it.

http://codereview.chromium.org/165455/diff/3055/2067
File chrome/browser/bookmarks/bookmark_index.h (right):

http://codereview.chromium.org/165455/diff/3055/2067#newcode84
Line 84: typedef std::pair<const BookmarkNode*, int> NodeTypedCountPair;
Can you add a comment above this as to what this is, especially that the second
value is the typed count.

http://codereview.chromium.org/165455/diff/3055/2067#newcode93
Line 93: // node from the in-memory database. Inserts pairs into a vector to be
sorted.
nit: Inserts pairs containing the node and typed count into the fector
|node_typed_counts|. |node_typed_counts| is sorted based on type count (highest
first).

[pierre.lafayette, 2009-08-24 16:16:20]

http://codereview.chromium.org/165455/diff/3082/2091
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/3082/2091#newcode322
Line 322: SortMatches(&url_db, matches, &node_typed_counts, 4);
I'm trying to mock the call to GetRowForURL but from what I can tell google mock
seems to only have the ability to mock calls that are made directly on the mock
object (I miss aspect oriented mocking). The url_db.GetRowForURL calls in fact
do get intercepted whereas injecting the mock into the URLDatabase* pointer in
the SortMatches call seems to strip out the mocking functionality.

How should I go about testing this?

[sky, 2009-08-24 18:41:10]

Rather than mocking I would suggest creating an InMemoryDB and adding
what you need to it. That'll likely be easier than having to mock out
a bunch of stuff anyway.

  -Scott

On Mon, Aug 24, 2009 at 9:16 AM, <pierre.lafayette@gmail.com> wrote:
>
> http://codereview.chromium.org/165455/diff/3082/2091
> File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):
>
> http://codereview.chromium.org/165455/diff/3082/2091#newcode322
> Line 322: SortMatches(&url_db, matches, &node_typed_counts, 4);
> I'm trying to mock the call to GetRowForURL but from what I can tell
> google mock seems to only have the ability to mock calls that are made
> directly on the mock object (I miss aspect oriented mocking). The
> url_db.GetRowForURL calls in fact do get intercepted whereas injecting
> the mock into the URLDatabase* pointer in the SortMatches call seems to
> strip out the mocking functionality.
>
> How should I go about testing this?
>
> http://codereview.chromium.org/165455
>

[pierre.lafayette, 2009-08-24 19:47:02]

On 2009/08/24 18:41:10, sky wrote:
> Rather than mocking I would suggest creating an InMemoryDB and adding
> what you need to it. That'll likely be easier than having to mock out
> a bunch of stuff anyway.

Okay. Updated.

[sky, 2009-08-24 20:37:53]

http://codereview.chromium.org/165455/diff/3086/2100
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/3086/2100#newcode76
Line 76: // vector is in descending order of typed count, we run the loop
backwards.
Isn't results ordered with those with highest typed count first?

http://codereview.chromium.org/165455/diff/3086/2100#newcode98
Line 98: if (node_typed_counts->size() > max_count)
There is no need to do this as GetBookmarksWithTitlesMatch checks the size.
Additionally if AddMatchToResults ends up not adding the match you might end up
with less than max_count even though you might have more than max_count.

http://codereview.chromium.org/165455/diff/3086/2102
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/3086/2102#newcode288
Line 288: SortMatches(&url_db, matches, &node_typed_counts, 4);
Can't you go through the public API rather than needing a friend? You can use
TestingProfile to populate an history service for you, eg:

TestingProfile profile;
profile.CreateHistoryService(true);
profile.BlockUntilHistoryProcessesPendingRequests();

[pierre.lafayette, 2009-08-25 05:01:10]

On 2009/08/24 20:37:53, sky wrote:
> http://codereview.chromium.org/165455/diff/3086/2100
> File chrome/browser/bookmarks/bookmark_index.cc (right):
> 
> http://codereview.chromium.org/165455/diff/3086/2100#newcode76
> Line 76: // vector is in descending order of typed count, we run the loop
> backwards.
> Isn't results ordered with those with highest typed count first?

HistoryContentsProvider::CombineResults attributes a relevance to each result.
The relevance gets incremented each iteration so we want to sort in increasing
order so that the increment will favor the better matches.

I've made the sort in BookmarkIndex use decreasing order so that the loop that
calls AddMatchToResults will never disregard the best matches (if the results
size reaches max_count). This required reversing the loop in QueryBookmarks to
preserve the proper relevance ordering.

[Peter Kasting, 2009-08-25 05:07:45]

On Mon, Aug 24, 2009 at 10:01 PM, <pierre.lafayette@gmail.com> wrote:

> HistoryContentsProvider::CombineResults attributes a relevance to each
> result. The relevance gets incremented each iteration so we want to sort
> in increasing order so that the increment will favor the better matches.


That code is retarded.  I suggest changing it to do things in descending
order like all the other providers.  :P

http://codereview.chromium.org/165455
>

[pierre.lafayette, 2009-08-25 12:42:11]

On 2009/08/25 05:07:45, Peter Kasting wrote:
> That code is retarded.  I suggest changing it to do things in descending
> order like all the other providers.  :P

Well ConvertResults (typo earlier) does sort into decreasing order after the
relevance is calculated for each result. It's just the relevance calculation
loop that doesn't expect the results to be ordered. I've updated the code so
that it expects descending order for the incoming results.

http://codereview.chromium.org/165455

[sky, 2009-08-25 14:28:53]

LGTM

[pierre.lafayette, 2009-08-25 14:36:53]

On 2009/08/25 14:28:53, sky wrote:
> LGTM

Peter?

[Peter Kasting, 2009-08-25 17:28:31]

Looks pretty good, mostly just nits.

http://codereview.chromium.org/165455/diff/2114/2115
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2114/2115#newcode158
Line 158: for (size_t i = results_.size(); i-- > 0; ) {
I'm still worried about how the scoring works.  The incremented variables in
CalculateRelevance() only get reset in Start(), but ConvertResults() can be
called multiple times for the same result set if we're allowing an asynchronous
query.  This will change the result scoring between the sync and async passes,
which can lead to results moving around in the popup (although the difference
will be small and thus the likelihood of visible effects low).

It seems like instead what we should perhaps do is reset these variables to 0 at
the top of ConvertResults().

Also, a nit: Your loop works fine, but having a condition check with a side
effect is kind of unusual.  Perhaps:
for (size_t i = results_.size() - 1; i < results_.size(); --i) { ...

http://codereview.chromium.org/165455/diff/2114/2115#newcode280
Line 280: // When we get here the results should be empty.
Nit: I'd just remove this comment, it adds nothing to the DCHECK.

http://codereview.chromium.org/165455/diff/2114/2116
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/2114/2116#newcode71
Line 71: for (size_t i = 0;
Nit: I would prefer an iterator here

http://codereview.chromium.org/165455/diff/2114/2116#newcode74
Line 74: node_typed_counts[i], &parser, query_nodes.get(), results);
Nit: Why not modify AddMatchToResults() to take a BookmarkNode*, and pass
node_typed_counts[i].first here?  If the helper function doesn't need the typed
count, we shouldn't bother to supply it.

http://codereview.chromium.org/165455/diff/2114/2116#newcode86
Line 86: for (size_t i = 0; i < matches.size(); ++i) {
Nit: I would prefer an iterator here; also, no { }

http://codereview.chromium.org/165455/diff/2114/2116#newcode91
Line 91: node_typed_counts->end(),
Nit: This could fit on the previous line

http://codereview.chromium.org/165455/diff/2114/2116#newcode103
Line 103: // The URLDatabase may be NULL if |profile_| is NULL.
Nit: This comment doesn't add anything, I'd remove it.

http://codereview.chromium.org/165455/diff/2114/2117
File chrome/browser/bookmarks/bookmark_index.h (right):

http://codereview.chromium.org/165455/diff/2114/2117#newcode84
Line 84: // Pairs BookmarkNodes and the number of times the node's URL was
typed. Used
Nit: "node's URL was" -> "nodes' URLs were"

http://codereview.chromium.org/165455/diff/2114/2117#newcode89
Line 89: // Sorts matching nodes in |match.nodes| by typed count.
Nit: Do you mean |matches.nodes|?

http://codereview.chromium.org/165455/diff/2114/2117#newcode93
Line 93: // Extracts BookmarkNodes from the matches and retrieves typed counts
for each
Nit: "matches" -> "match"?

http://codereview.chromium.org/165455/diff/2114/2117#newcode108
Line 108: // Add all the matching nodes in |match.nodes| to |results|.
Nit: "all the matching nodes in |match.nodes|" -> "the node in |match|"

http://codereview.chromium.org/165455/diff/2114/2118
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2114/2118#newcode20
Line 20: BookmarkIndexTest() : loop_(MessageLoop::TYPE_DEFAULT),
Nit: I don't see this actually used anywhere.  Seems like it and the #include of
message_loop.h could go away?

http://codereview.chromium.org/165455/diff/2114/2118#newcode91
Line 91: void DoneLoading() { model_->DoneLoading(model_->CreateLoadDetails());
}
Nit: Please put the function body on its own line.

http://codereview.chromium.org/165455/diff/2114/2118#newcode229
Line 229: const GURL url1("http://www.google.com/");
Nit: Use an array of data as in test "Tests", above, for readability and less
code duplication.

http://codereview.chromium.org/165455/diff/2114/2119
File chrome/browser/bookmarks/bookmark_model.cc (right):

http://codereview.chromium.org/165455/diff/2114/2119#newcode7
Line 7: #include <algorithm>
Remove these two #includes, they aren't needed

http://codereview.chromium.org/165455/diff/2114/2119#newcode516
Line 516: i != details.changed_urls.end(); ) {
Nit: This line needs one more space at the beginning (you removed it)

[pierre.lafayette, 2009-08-25 22:30:11]

Updated.

http://codereview.chromium.org/165455/diff/2114/2118
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2114/2118#newcode20
Line 20: BookmarkIndexTest() : loop_(MessageLoop::TYPE_DEFAULT),
On 2009/08/25 17:28:31, Peter Kasting wrote:
> Nit: I don't see this actually used anywhere.  Seems like it and the #include
of
> message_loop.h could go away?
In TestingProfile::BlockUntilHistoryProcessesPendingRequests I was failing on
DCHECK(MessageLoop::current()). My resolution is to initialize a message loop so
that current() is set.

[Peter Kasting, 2009-08-25 22:41:02]

LGTM except for the unittest MessageLoop issue.  Looping in Darin in hopes of
finding out what the right fix there is.

http://codereview.chromium.org/165455/diff/2114/2118
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2114/2118#newcode20
Line 20: BookmarkIndexTest() : loop_(MessageLoop::TYPE_DEFAULT),
On 2009/08/25 22:30:11, pierre.lafayette wrote:
> In TestingProfile::BlockUntilHistoryProcessesPendingRequests I was failing on
> DCHECK(MessageLoop::current()). My resolution is to initialize a message loop
so
> that current() is set.

I'm pretty sure that's not the right fix.  Scott or perhaps Darin could perhaps
say what the correct thing is.

(Even if this was right, you'd need comments explaining it.)

http://codereview.chromium.org/165455/diff/2123/3135
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2123/3135#newcode155
Line 155: // Reset the relevance counters so that result relevance will vary on
Nit: "will" -> "won't"

Also, I'd put this block above the one above it.

http://codereview.chromium.org/165455/diff/2123/3136
File chrome/browser/bookmarks/bookmark_index.cc (right):

http://codereview.chromium.org/165455/diff/2123/3136#newcode74
Line 74: AddMatchToResults((*i).first, &parser, query_nodes.get(), results);
Nit: "(*i).first" -> "i->first"

http://codereview.chromium.org/165455/diff/2123/3136#newcode89
Line 89: node_typed_counts->end(), &NodeTypedCountPairSortFunc);
Nit: I meant that node_typed_counts->end() could fit on the previous line...

http://codereview.chromium.org/165455/diff/2123/3137
File chrome/browser/bookmarks/bookmark_index.h (right):

http://codereview.chromium.org/165455/diff/2123/3137#newcode89
Line 89: // Extracts the BookmarkNodes in the underlying Match NodeSets into
Nit: "the underlying Match NodeSets" -> "|matches.nodes|"?  (Seems clearer...)

http://codereview.chromium.org/165455/diff/2123/3137#newcode94
Line 94: // Extracts BookmarkNodes from the Match and retrieves typed counts for
each
Nit: "the Match" -> "|match|"?

http://codereview.chromium.org/165455/diff/2123/3138
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2123/3138#newcode20
Line 20: BookmarkIndexTest() : loop_(MessageLoop::TYPE_DEFAULT),
Nit: Wrong indentation (extra space)

[sky, 2009-08-25 22:50:24]

http://codereview.chromium.org/165455/diff/2114/2118
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2114/2118#newcode20
Line 20: BookmarkIndexTest() : loop_(MessageLoop::TYPE_DEFAULT),
On 2009/08/25 22:41:03, Peter Kasting wrote:
> On 2009/08/25 22:30:11, pierre.lafayette wrote:
> > In TestingProfile::BlockUntilHistoryProcessesPendingRequests I was failing
on
> > DCHECK(MessageLoop::current()). My resolution is to initialize a message
loop
> so
> > that current() is set.
> 
> I'm pretty sure that's not the right fix.  Scott or perhaps Darin could
perhaps
> say what the correct thing is.
> 
> (Even if this was right, you'd need comments explaining it.)

Using TestingProfile like this requires a MessageLoop (loading history is
async). Other tests using TestingProfile do similar things.

[Peter Kasting, 2009-08-25 22:59:09]

On Tue, Aug 25, 2009 at 3:50 PM, <sky@chromium.org> wrote:

> Using TestingProfile like this requires a MessageLoop (loading history
> is async). Other tests using TestingProfile do similar things.


Can the individual test init a local MessageLoop variable to scope it (which
would also make comments about it clearer)?  I'm not familiar with this
technique, obviously.

http://codereview.chromium.org/165455
>

[sky, 2009-08-25 23:04:42]

On 2009/08/25 22:59:09, Peter Kasting wrote:
> On Tue, Aug 25, 2009 at 3:50 PM, <mailto:sky@chromium.org> wrote:
> 
> > Using TestingProfile like this requires a MessageLoop (loading history
> > is async). Other tests using TestingProfile do similar things.
> 
> 
> Can the individual test init a local MessageLoop variable to scope it (which
> would also make comments about it clearer)?  I'm not familiar with this
> technique, obviously.

Yes, it should be possible to keep this as a local variable in the new test
being added.

[pierre.lafayette, 2009-08-25 23:51:48]

On 2009/08/25 23:04:42, sky wrote:
> Yes, it should be possible to keep this as a local variable in the new test
> being added.

Done.

[Peter Kasting, 2009-08-25 23:58:51]

LGTM!

http://codereview.chromium.org/165455/diff/3147/2132
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/3147/2132#newcode217
Line 217: // We need to initialize the current MessageLoop when using
Nit: Might be slightly clearer to say "This ensures MessageLoop::current() will
exist, which is needed by
TestingProfile::BlockUntilHistoryProcessesPendingRequests()."

[pierre.lafayette, 2009-08-26 00:04:03]

On 2009/08/25 23:58:51, Peter Kasting wrote:
> LGTM!
> 
> http://codereview.chromium.org/165455/diff/3147/2132
> File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):
> 
> http://codereview.chromium.org/165455/diff/3147/2132#newcode217
> Line 217: // We need to initialize the current MessageLoop when using
> Nit: Might be slightly clearer to say "This ensures MessageLoop::current()
will
> exist, which is needed by
> TestingProfile::BlockUntilHistoryProcessesPendingRequests()."

Done.

[Peter Kasting, 2009-08-26 04:10:37]

Do you need one of us to commit this for you?

[pierre.lafayette, 2009-08-26 04:52:25]

On 2009/08/26 04:10:37, Peter Kasting wrote:
> Do you need one of us to commit this for you?

Yep. BTW do the 10 required patches to be a committer need to be landed patches?
I believe I should have at least 10 total patches by now (I think). Non-landed
patches still take up precious developer time...

[pierre.lafayette, 2009-08-26 16:36:05]

I'm not sure about these ones:

../chrome/browser/bookmarks/bookmark_model.h: In member function 'void
BookmarkIndexTest::DoneLoading()':
../chrome/browser/bookmarks/bookmark_model.h:401: error:
'BookmarkStorage::LoadDetails* BookmarkModel::CreateLoadDetails()' is
private
/b/slave/mac/build/src/chrome/browser/bookmarks/bookmark_index_unittest.cc:91:
error: within this context
../chrome/browser/bookmarks/bookmark_model.h:331: error: 'void
BookmarkModel::DoneLoading(BookmarkStorage::LoadDetails*)' is private
/b/slave/mac/build/src/chrome/browser/bookmarks/bookmark_index_unittest.cc:91:
error: within this context

It's not recognizing the friend class ?

2009/8/26 Scott Violet <sky@chromium.org>

> Compile failure on mac:
>
>
>
http://build.chromium.org/buildbot/try-server/builders/mac/builds/14762/steps...
>
>  -Scott
>
> On Wed, Aug 26, 2009 at 8:38 AM, Scott Violet<sky@chromium.org> wrote:
> > On Tue, Aug 25, 2009 at 9:52 PM, <pierre.lafayette@gmail.com> wrote:
> >> On 2009/08/26 04:10:37, Peter Kasting wrote:
> >>>
> >>> Do you need one of us to commit this for you?
> >>
> >> Yep.
> >
> > I'll land it for you this morning.
> >
> >> BTW do the 10 required patches to be a committer need to be landed
> >> patches? I believe I should have at least 10 total patches by now (I
> >> think). Non-landed patches still take up precious developer time...
> >
> > I believe the patches need to be committed.
> >
> >  -Scott
> >
> >>
> >> http://codereview.chromium.org/165455
> >>
> >
>



-- 
Pierre.

[sky, 2009-08-26 16:43:02]

On Tue, Aug 25, 2009 at 9:52 PM, <pierre.lafayette@gmail.com> wrote:
> On 2009/08/26 04:10:37, Peter Kasting wrote:
>>
>> Do you need one of us to commit this for you?
>
> Yep.

I'll land it for you this morning.

> BTW do the 10 required patches to be a committer need to be landed
> patches? I believe I should have at least 10 total patches by now (I
> think). Non-landed patches still take up precious developer time...

I believe the patches need to be committed.

  -Scott

>
> http://codereview.chromium.org/165455
>

[sky, 2009-08-26 17:10:44]

Compile failure on mac:

http://build.chromium.org/buildbot/try-server/builders/mac/builds/14762/ste=
ps/compile/logs/stdio

  -Scott

On Wed, Aug 26, 2009 at 8:38 AM, Scott Violet<sky@chromium.org> wrote:
> On Tue, Aug 25, 2009 at 9:52 PM, <pierre.lafayette@gmail.com> wrote:
>> On 2009/08/26 04:10:37, Peter Kasting wrote:
>>>
>>> Do you need one of us to commit this for you?
>>
>> Yep.
>
> I'll land it for you this morning.
>
>> BTW do the 10 required patches to be a committer need to be landed
>> patches? I believe I should have at least 10 total patches by now (I
>> think). Non-landed patches still take up precious developer time...
>
> I believe the patches need to be committed.
>
> =A0-Scott
>
>>
>> http://codereview.chromium.org/165455
>>
>

[pierre.lafayette, 2009-08-26 18:15:09]

On 2009/08/26 17:10:44, sky wrote:
> Compile failure on mac:
> 
> http://build.chromium.org/buildbot/try-server/builders/mac/builds/14762/ste=
> ps/compile/logs/stdio

I've updated the patch. I think this should fix it.

http://codereview.chromium.org/165455

[sky, 2009-08-26 18:20:32]

http://codereview.chromium.org/165455/diff/2162/3192
File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):

http://codereview.chromium.org/165455/diff/2162/3192#newcode220
Line 220: model_.reset(new BookmarkModel(&profile));
Rather than adding a bunch of new friends could you have the testing profile
create the bookmark model and block until loaded?

[pierre.lafayette, 2009-08-26 19:30:56]

On 2009/08/26 18:20:32, sky wrote:
> http://codereview.chromium.org/165455/diff/2162/3192
> File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):
> 
> http://codereview.chromium.org/165455/diff/2162/3192#newcode220
> Line 220: model_.reset(new BookmarkModel(&profile));
> Rather than adding a bunch of new friends could you have the testing profile
> create the bookmark model and block until loaded?

Sure. That works.

[sky, 2009-08-26 20:53:45]

Onto the next error:
http://build.chromium.org/buildbot/try-server/builders/mac/builds/14816/steps...
.

  -Scott

On Wed, Aug 26, 2009 at 12:30 PM, <pierre.lafayette@gmail.com> wrote:
> On 2009/08/26 18:20:32, sky wrote:
>>
>> http://codereview.chromium.org/165455/diff/2162/3192
>> File chrome/browser/bookmarks/bookmark_index_unittest.cc (right):
>
>> http://codereview.chromium.org/165455/diff/2162/3192#newcode220
>> Line 220: model_.reset(new BookmarkModel(&profile));
>> Rather than adding a bunch of new friends could you have the testing
>
> profile
>>
>> create the bookmark model and block until loaded?
>
> Sure. That works.
>
> http://codereview.chromium.org/165455
>

[pierre.lafayette, 2009-08-27 03:21:19]

Okay I've updated history_contents_provider_unittest.cc.

http://codereview.chromium.org/165455

[sky, 2009-08-27 20:32:36]

On the good news your build past all tests on all bots. Just one change and
think you're golden.

http://codereview.chromium.org/165455/diff/2184/3214
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2184/3214#newcode161
Line 161: for (size_t i = results_.size() - 1; i < results_.size(); --i) {
I realize this works, but it just looks wrong, please don't do this. A reverse
iterator is probably the best thing to use here.

[Peter Kasting, 2009-08-27 20:37:49]

http://codereview.chromium.org/165455/diff/2184/3214
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2184/3214#newcode161
Line 161: for (size_t i = results_.size() - 1; i < results_.size(); --i) {
On 2009/08/27 20:32:36, sky wrote:
> I realize this works, but it just looks wrong, please don't do this. A reverse
> iterator is probably the best thing to use here.

I take the blame for recommending this.  I didn't even think of a reverse
iterator, you're right that that would be better.

[pierre.lafayette, 2009-08-27 23:25:54]

Updated.

http://codereview.chromium.org/165455/diff/2184/3214
File chrome/browser/autocomplete/history_contents_provider.cc (right):

http://codereview.chromium.org/165455/diff/2184/3214#newcode161
Line 161: for (size_t i = results_.size() - 1; i < results_.size(); --i) {
On 2009/08/27 20:32:36, sky wrote:
> I realize this works, but it just looks wrong, please don't do this. A reverse
> iterator is probably the best thing to use here.

Sure. I didn't use a reverse iterator cause QueryResults didn't already have
accessors for the iterators.

[Peter Kasting, 2009-08-27 23:34:56]

One last nit for this latest patch (sorry!!!)

http://codereview.chromium.org/165455/diff/5001/5008
File chrome/browser/history/history_types.h (right):

http://codereview.chromium.org/165455/diff/5001/5008#newcode396
Line 396: std::vector<URLResult*>::const_iterator begin() const {
Nit: Move the private typedef of URLResultVector up into the public section and
then use it in these declarations.

[pierre.lafayette, 2009-08-28 00:11:37]

On 2009/08/27 23:34:56, Peter Kasting wrote:
> One last nit for this latest patch (sorry!!!)
> 
> http://codereview.chromium.org/165455/diff/5001/5008
> File chrome/browser/history/history_types.h (right):
> 
> http://codereview.chromium.org/165455/diff/5001/5008#newcode396
> Line 396: std::vector<URLResult*>::const_iterator begin() const {
> Nit: Move the private typedef of URLResultVector up into the public section
and
> then use it in these declarations.

Updated. Thanks.

[Peter Kasting, 2009-08-28 00:18:24]

Tweaked and landed in r24704, let's see if the bots break.

[sky, 2009-08-28 16:00:11]

YAY!

  -Scott

On Thu, Aug 27, 2009 at 5:18 PM, <pkasting@chromium.org> wrote:
> Tweaked and landed in r24704, let's see if the bots break.
>
> http://codereview.chromium.org/165455
>