Componentize ScoredHistoryMatch

Componentize ScoredHistoryMatch

Change ScoredHistoryMatch to be a POD structure and introduce a new
ScoredHistoryMatchBuilder to delegate the initialization of ScoredHistoryMatch
to the embedder (mostly autocomplete code which is the sole client of that
code).

Move the ScoredHistoryMatch construction code into autocomple as it
is the sole client of those values.

Clean up chrome/browser/DEPS.

BUG=374731, 374732
Committed: https://crrev.com/5677607ede1f589bc9bb67632de0542750f0855f
Cr-Commit-Position: refs/heads/master@{#316487}

[sdefresne, 2015-02-04 17:45:01]

sdefresne@chromium.org changed reviewers:
+ mpearson@chromium.org, sky@chromium.org

[sdefresne, 2015-02-04 17:45:02]

sky: can you review chrome/browser/history?
mpearson: can you review chrome/browser/autocomplete?

sky, mpearson: this is based on the solution suggested in my quick design
available at
https://docs.google.com/a/chromium.org/document/d/1MwL30zkxv57i11qnK8kMN-fS-Y...

[Mark P, 2015-02-04 19:32:26]

Can you reupload this in a form (git mv?) so that the code review site shows
proper side-by-sides for the files (i.e., scored_history_match.{h,cc}) that you
mainly moved and only did minor modifications to?  That would make this easier
to review.

thanks,
mark

[Mark P, 2015-02-04 19:34:40]

On 2015/02/04 19:32:26, Mark P wrote:
> Can you reupload this in a form (git mv?) so that the code review site shows
> proper side-by-sides for the files (i.e., scored_history_match.{h,cc}) that
you
> mainly moved and only did minor modifications to?  That would make this easier
> to review.
> 
> thanks,
> mark

and scored_history_match_builder too, if possible

[Mark P, 2015-02-04 19:35:00]

On 2015/02/04 19:34:40, Mark P wrote:
> On 2015/02/04 19:32:26, Mark P wrote:
> > Can you reupload this in a form (git mv?) so that the code review site shows
> > proper side-by-sides for the files (i.e., scored_history_match.{h,cc}) that
> you
> > mainly moved and only did minor modifications to?  That would make this
easier
> > to review.
> > 
> > thanks,
> > mark
> 
> and scored_history_match_builder too, if possible
.h  (I see the .cc already shows a useful diff)

[Mark P, 2015-02-04 19:54:04]

I looked at the interfaces.  I'm okay with this approach for moving
ScoredHistoryMatch to the history component.  That said, I'm not a design expert
and you should have someone else review your approach.  Once that's done, I'll
happily review the nitty-gritty of the code.

--mark

https://codereview.chromium.org/896983003/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/scored_history_match_builder.h (right):

https://codereview.chromium.org/896983003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder.h:39: // being called
multiple time.
nit: time->times

https://codereview.chromium.org/896983003/diff/20001/components/history/core/...
File components/history/core/browser/scored_history_match.h (right):

https://codereview.chromium.org/896983003/diff/20001/components/history/core/...
components/history/core/browser/scored_history_match.h:24: class Builder {
Having a ScoredHistoryMatchBuilder and a ScoredHistoryMatch::Builder be two
different things that both exist is confusing.  Can you come up with a better
naming system?

[sdefresne, 2015-02-05 10:51:28]

I've tried to reupload with "git cl upload --find-copies --similarity=15" and I
see that scored_history_match_builder.{cc,h} is detected as a copy of
scored_history_match.{cc,h} and that the move of the rest of the file from
chrome/browser to components/history/core/browser is also correctly detected as
a move. Does it make the CL easier to read?

re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I also
think that the name are really similar, however I don't really have better name
(naming is IMO one of the hardest problem in CS). Would
AutocompleteScoredHistoryMatchBuilder be clearer?

sky: what do you think about moving the construction responsibility into
chrome/browser/autocomplete?

[Mark P, 2015-02-05 16:47:25]

On 2015/02/05 10:51:28, sdefresne wrote:
> I've tried to reupload with "git cl upload --find-copies --similarity=15" and
I
> see that scored_history_match_builder.{cc,h} is detected as a copy of
> scored_history_match.{cc,h} and that the move of the rest of the file from
> chrome/browser to components/history/core/browser is also correctly detected
as
> a move. Does it make the CL easier to read?

Yes, that's what I wanted, thanks.

(Nevertheless, I won't review this change in more detail until you get signoff
from
someone else that this is a good approach.)

> re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I also
> think that the name are really similar, however I don't really have better
name
> (naming is IMO one of the hardest problem in CS). Would
> AutocompleteScoredHistoryMatchBuilder be clearer?

I understand the trouble.  Both classes are effectively doing the same thing --
that's
why it's hard to give them distinguishing names.  Perhaps the best approach
would be to change  history::ScoredHistoryMatch::Builder to
history::ScoredHistoryMatch::BuilderInterface to make clear the distinction
between
these classes is that one cannot be instantiated (it's not fully implemented);
the
other is actually the proper builder.

> sky: what do you think about moving the construction responsibility into
> chrome/browser/autocomplete?

[sky, 2015-02-05 17:21:54]

On 2015/02/05 10:51:28, sdefresne wrote:
> I've tried to reupload with "git cl upload --find-copies --similarity=15" and
I
> see that scored_history_match_builder.{cc,h} is detected as a copy of
> scored_history_match.{cc,h} and that the move of the rest of the file from
> chrome/browser to components/history/core/browser is also correctly detected
as
> a move. Does it make the CL easier to read?
> 
> re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I also
> think that the name are really similar, however I don't really have better
name
> (naming is IMO one of the hardest problem in CS). Would
> AutocompleteScoredHistoryMatchBuilder be clearer?
> 
> sky: what do you think about moving the construction responsibility into
> chrome/browser/autocomplete?

Did you consider making it possible for the embedder to specify the constants
that are needed
(HistoryURLProvider::kScoreForBestInlineableResult/kBaseScoreForNonInlineableResult)
and some means to do the DCHECK? That seems simpler than more
builders/factories/abstract-classes.

[sdefresne, 2015-02-06 13:35:46]

On 2015/02/05 at 17:21:54, sky wrote:
> On 2015/02/05 10:51:28, sdefresne wrote:
> > I've tried to reupload with "git cl upload --find-copies --similarity=15"
and I
> > see that scored_history_match_builder.{cc,h} is detected as a copy of
> > scored_history_match.{cc,h} and that the move of the rest of the file from
> > chrome/browser to components/history/core/browser is also correctly detected
as
> > a move. Does it make the CL easier to read?
> > 
> > re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I
also
> > think that the name are really similar, however I don't really have better
name
> > (naming is IMO one of the hardest problem in CS). Would
> > AutocompleteScoredHistoryMatchBuilder be clearer?
> > 
> > sky: what do you think about moving the construction responsibility into
> > chrome/browser/autocomplete?
> 
> Did you consider making it possible for the embedder to specify the constants
that are needed
(HistoryURLProvider::kScoreForBestInlineableResult/kBaseScoreForNonInlineableResult)
and some means to do the DCHECK? That seems simpler than more
builders/factories/abstract-classes.

We did made multiple attempts to abstract part of those dependencies.

https://codereview.chromium.org/284183011/ tried to deal with
HistoryURLProvider::* constants. We considered multiple options: duplicating,
injecting them, removing the code has it is protected by a constant always set
to false, abstracting them through the HistoryClient. In the end, the
abstraction felt at the wrong level (why abstract just a few constants).
Injecting the value caused the HistoryService::Init() method to takes too many
parameters (3 for the OmniboxFieldTrials, 4 for the HistoryURLProvider) that
were only used during the first call to that method then ignored.

https://codereview.chromium.org/312493003/ tried to address the URLPrefix usage
by abstracting them through the HistoryClient (sorry, the CL is no longer
available, I don't know why and reported http://crbug.com/456078). The
conclusion was that abstracting just the URLPrefix was the wrong level, and
instead it was necessary to abstract the whole logic computing the value of
"can_inline_" and "innermost_match" fields.

https://codereview.chromium.org/903493002 was a third attempt to abstract all
those dependencies through the HistoryClient (and resolving the BrowserThread
usage too). I think this time the abstraction is at the correct level, but while
implementing that CL, I found the code hard to follow. That's why I tried to go
with the Builder interface (i.e. the CL currently under review).

Note that there are four things to abstract there:
- HistoryURLProvider::*
- OmniboxFieldTrials::*
- URLPrefix::*
- BrowserThread::*

Personally, I think that moving the code into autocomplete makes more sense than
abstracting part of the computation through the HistoryClient (or a new
ScoredHistoryClient as in my other CL, that I was planning to make accessible
through the HistoryClient).

Please take a look at both this CL and the other CL,
https://codereview.chromium.org/903493002 and tell me what approach you prefer.
If this is the abstraction via a client, then I'll finish my other CL to
actually compile and link (there are some missing bits).

Thank you.

[sky, 2015-02-06 16:03:12]

On 2015/02/05 16:47:25, Mark P wrote:
> On 2015/02/05 10:51:28, sdefresne wrote:
> > I've tried to reupload with "git cl upload --find-copies --similarity=15"
and
> I
> > see that scored_history_match_builder.{cc,h} is detected as a copy of
> > scored_history_match.{cc,h} and that the move of the rest of the file from
> > chrome/browser to components/history/core/browser is also correctly detected
> as
> > a move. Does it make the CL easier to read?
> 
> Yes, that's what I wanted, thanks.
> 
> (Nevertheless, I won't review this change in more detail until you get signoff
> from
> someone else that this is a good approach.)
> 
> > re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I
also
> > think that the name are really similar, however I don't really have better
> name
> > (naming is IMO one of the hardest problem in CS). Would
> > AutocompleteScoredHistoryMatchBuilder be clearer?
> 
> I understand the trouble.  Both classes are effectively doing the same thing
--
> that's
> why it's hard to give them distinguishing names.  Perhaps the best approach
> would be to change  history::ScoredHistoryMatch::Builder to
> history::ScoredHistoryMatch::BuilderInterface to make clear the distinction
> between
> these classes is that one cannot be instantiated (it's not fully implemented);
> the
> other is actually the proper builder.

It's pretty rare in chrome code to have Interface in the name of the class. More
often than not we tack on Impl to the name of the class. So we would end up with
ScoredHistoryMatchBuilderImpl. We've also used the prefix Chrome to make it
clear as well. I'm fine with either.

> 
> > sky: what do you think about moving the construction responsibility into
> > chrome/browser/autocomplete?

[sky, 2015-02-06 16:03:34]

On 2015/02/06 13:35:46, sdefresne wrote:
> On 2015/02/05 at 17:21:54, sky wrote:
> > On 2015/02/05 10:51:28, sdefresne wrote:
> > > I've tried to reupload with "git cl upload --find-copies --similarity=15"
> and I
> > > see that scored_history_match_builder.{cc,h} is detected as a copy of
> > > scored_history_match.{cc,h} and that the move of the rest of the file from
> > > chrome/browser to components/history/core/browser is also correctly
detected
> as
> > > a move. Does it make the CL easier to read?
> > > 
> > > re: ScoredHistoryMatchBuilder vs history::ScoredHistoryMatch::Builder, I
> also
> > > think that the name are really similar, however I don't really have better
> name
> > > (naming is IMO one of the hardest problem in CS). Would
> > > AutocompleteScoredHistoryMatchBuilder be clearer?
> > > 
> > > sky: what do you think about moving the construction responsibility into
> > > chrome/browser/autocomplete?
> > 
> > Did you consider making it possible for the embedder to specify the
constants
> that are needed
>
(HistoryURLProvider::kScoreForBestInlineableResult/kBaseScoreForNonInlineableResult)
> and some means to do the DCHECK? That seems simpler than more
> builders/factories/abstract-classes.
> 
> We did made multiple attempts to abstract part of those dependencies.
> 
> https://codereview.chromium.org/284183011/ tried to deal with
> HistoryURLProvider::* constants. We considered multiple options: duplicating,
> injecting them, removing the code has it is protected by a constant always set
> to false, abstracting them through the HistoryClient. In the end, the
> abstraction felt at the wrong level (why abstract just a few constants).
> Injecting the value caused the HistoryService::Init() method to takes too many
> parameters (3 for the OmniboxFieldTrials, 4 for the HistoryURLProvider) that
> were only used during the first call to that method then ignored.
> 
> https://codereview.chromium.org/312493003/ tried to address the URLPrefix
usage
> by abstracting them through the HistoryClient (sorry, the CL is no longer
> available, I don't know why and reported http://crbug.com/456078). The
> conclusion was that abstracting just the URLPrefix was the wrong level, and
> instead it was necessary to abstract the whole logic computing the value of
> "can_inline_" and "innermost_match" fields.
> 
> https://codereview.chromium.org/903493002 was a third attempt to abstract all
> those dependencies through the HistoryClient (and resolving the BrowserThread
> usage too). I think this time the abstraction is at the correct level, but
while
> implementing that CL, I found the code hard to follow. That's why I tried to
go
> with the Builder interface (i.e. the CL currently under review).
> 
> Note that there are four things to abstract there:
> - HistoryURLProvider::*
> - OmniboxFieldTrials::*
> - URLPrefix::*
> - BrowserThread::*
> 
> Personally, I think that moving the code into autocomplete makes more sense
than
> abstracting part of the computation through the HistoryClient (or a new
> ScoredHistoryClient as in my other CL, that I was planning to make accessible
> through the HistoryClient).
> 
> Please take a look at both this CL and the other CL,
> https://codereview.chromium.org/903493002 and tell me what approach you
prefer.
> If this is the abstraction via a client, then I'll finish my other CL to
> actually compile and link (there are some missing bits).

Excellent points. I'm fine with what you have here.

[sdefresne, 2015-02-06 17:11:33]

Thank you. I've renamed ScoredHistoryMatchBuilder to
ScoredHistoryMatchBuilderImpl.

mpearson: can you do a review of the CL now that Scott is okay with the idea?

[Mark P, 2015-02-07 07:00:14]

generally fine; comments below

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:9: #include
<algorithm>
include vector
(because I think we dropped it from the .h)

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:39: //
|days_ago_to_recency_score_| is a simple array mapping how long
extra underscore

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:42: // math. 
This is initialized upon first use of GetRecencyScore(),
This last sentence is wrong now.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:47: // scores. 
|raw_term_score_to_topicality_score_| is a simple array
extra underscore

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:52: // This is
initialized upon first use of GetTopicalityScore(),
This last sentence is wrong now.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:346: ?
(kMaxRawTermScore - 1)
optional nit: I thought operators were supposed to go at the end of lines.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:378: size_t
max_visit_to_score =
nit: const

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:503: if
(!scored_history_match.url_matches.empty() && terms.size() == 1 &&
nit: restore parens around binary operator ==

ditto many other places in this changelist.
(unless you can show me where the style guide has changed recently)

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:547:
scored_history_match.raw_score = std::min<int>(
I don't think this does what you want.  I think it effectively typecasts the
first parameter to an int, then does the min.  I think you want a float point
comparison between the first parameter and kint32max, and only then cast the
first thing to an int (if it's smaller).  Look at the original code.  Maybe
casting an out-of-range float to an int does something sensible, but you'll have
to cite the standard to me in order for me to believe it.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:8: #include
<vector>
nit: needed?

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:19: //
ScoredHistoryMatchBuilderImpl creates new history matches with a raw score
nit: ScoredHistoryMatchBuilderImpl -> ScoredHistoryMatchBuilderImpl's Build()
function

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:22: //
nit: why the paragraph break?

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:23: // First
determines if the row qualifies by seeing if all of the terms in
nit: First -> It first

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:39: // Returns
whether |url| is bookmarked, used to affect the score. Must support
nit: ", used" -> which is used

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:70: //
boundaries).  Revises url_matches and title_matches of
spacing in this whole comment

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:101:
history::ScoredHistoryMatch Build(
This should be public.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index_unittest.cc:141: :
builder_(ScoredHistoryMatchBuilderImpl::IsBookmarkedCallback()),
Does this construct an empty callback to pass to builder_'s constructor?

https://codereview.chromium.org/896983003/diff/80001/components/history/core/...
File components/history/core/browser/scored_history_match.h (right):

https://codereview.chromium.org/896983003/diff/80001/components/history/core/...
components/history/core/browser/scored_history_match.h:22: // Builder allows the
embedder to control how matches are scored (we cannot
nit: Builder -> The Builder inner class
or something, the current phrasing makes it hard to parse at first.

https://codereview.chromium.org/896983003/diff/80001/components/history/core/...
components/history/core/browser/scored_history_match.h:58:
ScoredHistoryMatch(const URLRow& url_info,
Please comment this.  The meaning of some of the variables you're introduced is
not obvious.
Perhaps it should be as simple as x, y, and z are passed to the HistoryMatch()
superclass constructor; a, b, and c are used to initialize local variables.

[sdefresne, 2015-02-09 10:01:14]

Please take another look.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:9: #include
<algorithm>
On 2015/02/07 at 07:00:12, Mark P wrote:
> include vector
> (because I think we dropped it from the .h)

Ack.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:39: //
|days_ago_to_recency_score_| is a simple array mapping how long
On 2015/02/07 at 07:00:12, Mark P wrote:
> extra underscore

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:42: // math. 
This is initialized upon first use of GetRecencyScore(),
On 2015/02/07 at 07:00:12, Mark P wrote:
> This last sentence is wrong now.

Fixed.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:47: // scores. 
|raw_term_score_to_topicality_score_| is a simple array
On 2015/02/07 at 07:00:12, Mark P wrote:
> extra underscore

Ack.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:52: // This is
initialized upon first use of GetTopicalityScore(),
On 2015/02/07 at 07:00:13, Mark P wrote:
> This last sentence is wrong now.

Reworded.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:346: ?
(kMaxRawTermScore - 1)
On 2015/02/07 at 07:00:12, Mark P wrote:
> optional nit: I thought operators were supposed to go at the end of lines.

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:378: size_t
max_visit_to_score =
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: const

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:503: if
(!scored_history_match.url_matches.empty() && terms.size() == 1 &&
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: restore parens around binary operator ==
> 
> ditto many other places in this changelist.
> (unless you can show me where the style guide has changed recently)

Ooops, fixed.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:547:
scored_history_match.raw_score = std::min<int>(
On 2015/02/07 at 07:00:12, Mark P wrote:
> I don't think this does what you want.  I think it effectively typecasts the
first parameter to an int, then does the min.  I think you want a float point
comparison between the first parameter and kint32max, and only then cast the
first thing to an int (if it's smaller).  Look at the original code.  Maybe
casting an out-of-range float to an int does something sensible, but you'll have
to cite the standard to me in order for me to believe it.

Good catch. Changed back to old correct code.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:8: #include
<vector>
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: needed?

Ack.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:19: //
ScoredHistoryMatchBuilderImpl creates new history matches with a raw score
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: ScoredHistoryMatchBuilderImpl -> ScoredHistoryMatchBuilderImpl's Build()
function

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:22: //
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: why the paragraph break?

I wanted the first paragraph to quickly summarize the purpose of the class and
the second to go into detail. Removed the paragraph break since it was not
serving its purpose apparently.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:23: // First
determines if the row qualifies by seeing if all of the terms in
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: First -> It first

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:39: // Returns
whether |url| is bookmarked, used to affect the score. Must support
On 2015/02/07 at 07:00:13, Mark P wrote:
> nit: ", used" -> which is used

Done.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:70: //
boundaries).  Revises url_matches and title_matches of
On 2015/02/07 at 07:00:13, Mark P wrote:
> spacing in this whole comment

Ack.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/autocompl...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:101:
history::ScoredHistoryMatch Build(
On 2015/02/07 at 07:00:13, Mark P wrote:
> This should be public.

I was previously told that unless there is a need to call an overridden method
on a derived class, then overridden method should be private and client code
should use pointers to base class, and that is why I did put this in private.

Moved to public since I don't have strong opinion personally.

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/history/i...
File chrome/browser/history/in_memory_url_index_unittest.cc (right):

https://codereview.chromium.org/896983003/diff/80001/chrome/browser/history/i...
chrome/browser/history/in_memory_url_index_unittest.cc:141: :
builder_(ScoredHistoryMatchBuilderImpl::IsBookmarkedCallback()),
On 2015/02/07 at 07:00:13, Mark P wrote:
> Does this construct an empty callback to pass to builder_'s constructor?

Yes, as those tests does not bookmark any URL. The
ScoredHistoryMatchBuilderImpl::Build supports null callback (see the
is_bookmarked.is_null() check in the implementation). Should I instead provide
another constructor for ScoredHistoryMatchBuilderImpl that does not take any
parameter and use an unbound callback?

https://codereview.chromium.org/896983003/diff/80001/components/history/core/...
File components/history/core/browser/scored_history_match.h (right):

https://codereview.chromium.org/896983003/diff/80001/components/history/core/...
components/history/core/browser/scored_history_match.h:58:
ScoredHistoryMatch(const URLRow& url_info,
On 2015/02/07 at 07:00:14, Mark P wrote:
> Please comment this.  The meaning of some of the variables you're introduced
is not obvious.
> Perhaps it should be as simple as x, y, and z are passed to the HistoryMatch()
superclass constructor; a, b, and c are used to initialize local variables.

Done.

[Mark P, 2015-02-09 19:48:21]

lgtm baring minor comments below

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right):

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:201: if
(!scored_history_match.url_matches.empty() && (terms.size()) == 1 &&
unnecessary parens around terms.size(), missing parens around ==

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:247: if
(scored_history_match.raw_score > kint32max)
Actually, it looks like the original code buggy, taking a float output from a
function, storing it in an int field, the comparing it to kIntMax.
Can you fix it in this changelist GetFinalRelevancyScore, put it in a tmp local
variable, then store it (typecast to an int) or kint32max in raw_score as
appropriate?

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right):

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:20: // if all of
the terms in  |terms_vector| occur in |row|.  If so, calculates a
nit: extra space

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
File components/history/core/browser/scored_history_match.h (right):

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
components/history/core/browser/scored_history_match.h:23: // scored (we cannot/
use a base::Callback<> as base::Bind() is limited to 6
nit: remove extra "/"

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
components/history/core/browser/scored_history_match.h:62: // initialize the
corresponding properties.
nit: please append "of this class." for clarity

[Mark P, 2015-02-13 22:27:28]

Hi,

Can you give me an ETA on when this will be submitted.  I have teammates trying
to change the ScoredHistoryMatch scoring code and I don't know whether they
should do it first and ahve you submit later or have you submit first and they
do it later...

--mark

[sdefresne, 2015-02-14 19:14:47]

On 2015/02/13 at 22:27:28, mpearson wrote:
> Hi,
> 
> Can you give me an ETA on when this will be submitted.  I have teammates
trying to change the ScoredHistoryMatch scoring code and I don't know whether
they should do it first and ahve you submit later or have you submit first and
they do it later...
> 
> --mark

I'm planning to land this Monday (my initial plan was to try to get it by
Friday, but I was delaying by a complex merge downstream).

[sdefresne, 2015-02-16 10:52:57]

New patchsets have been uploaded after l-g-t-m from mpearson@chromium.org

[sdefresne, 2015-02-16 13:26:41]

Thank you for the review.

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/scored_history_match_builder_impl.cc (right):

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:201: if
(!scored_history_match.url_matches.empty() && (terms.size()) == 1 &&
On 2015/02/09 at 19:48:21, Mark P wrote:
> unnecessary parens around terms.size(), missing parens around ==

Done.

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.cc:247: if
(scored_history_match.raw_score > kint32max)
On 2015/02/09 at 19:48:21, Mark P wrote:
> Actually, it looks like the original code buggy, taking a float output from a
function, storing it in an int field, the comparing it to kIntMax.
> Can you fix it in this changelist GetFinalRelevancyScore, put it in a tmp
local variable, then store it (typecast to an int) or kint32max in raw_score as
appropriate?

Done.

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/scored_history_match_builder_impl.h (right):

https://codereview.chromium.org/896983003/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/scored_history_match_builder_impl.h:20: // if all of
the terms in  |terms_vector| occur in |row|.  If so, calculates a
On 2015/02/09 at 19:48:21, Mark P wrote:
> nit: extra space

Done.

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
File components/history/core/browser/scored_history_match.h (right):

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
components/history/core/browser/scored_history_match.h:23: // scored (we cannot/
use a base::Callback<> as base::Bind() is limited to 6
On 2015/02/09 at 19:48:21, Mark P wrote:
> nit: remove extra "/"

Done.

https://codereview.chromium.org/896983003/diff/120001/components/history/core...
components/history/core/browser/scored_history_match.h:62: // initialize the
corresponding properties.
On 2015/02/09 at 19:48:21, Mark P wrote:
> nit: please append "of this class." for clarity

Done.

[sdefresne, 2015-02-16 15:23:45]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2015-02-16 15:24:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/896983003/160001

[commit-bot: I haz the power, 2015-02-16 16:08:15]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-02-16 16:09:01]

Patchset 9 (id:??) landed as
https://crrev.com/5677607ede1f589bc9bb67632de0542750f0855f
Cr-Commit-Position: refs/heads/master@{#316487}

[Mark P, 2015-02-17 16:56:37]

submitted patchset lgtm for the record