Code clean up for ScoredHistoryMatch

Code clean up for ScoredHistoryMatch

Clean up the dead code using also_do_hup_like_scoring_ and
max_assigned_score_for_non_inlineable_matches_
    
BUG=None
TEST=unit_tests --gtest_filter=ScoredHistory*
R=sdefresne,blundell

[sdefresne, 2014-05-22 12:39:23]

No need to put me in TBR. Just put me in reviewer.

In general, you do use TBR when you've made some refactoring, the refactoring
has been approved by the OWNER of the feature, and you don't want to wait for
all OWNERS of the client code to review your code. In that case, you TBR the
OWNERS of the client code.

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

https://codereview.chromium.org/284183011/diff/1/chrome/browser/history/score...
chrome/browser/history/scored_history_match.h:33: //These values are duplicated
from the HistoryURLProvider
Those constants are only used on scored_history_match.cc so there is no need to
make them public. Instead, put them in an anonymous namespace in
scored_history_match.cc.

[nshaik, 2014-05-22 16:30:26]

On 2014/05/22 12:39:23, sdefresne wrote:
> No need to put me in TBR. Just put me in reviewer.
> 
> In general, you do use TBR when you've made some refactoring, the refactoring
> has been approved by the OWNER of the feature, and you don't want to wait for
> all OWNERS of the client code to review your code. In that case, you TBR the
> OWNERS of the client code.
> 
>
https://codereview.chromium.org/284183011/diff/1/chrome/browser/history/score...
> File chrome/browser/history/scored_history_match.h (right):
> 
>
https://codereview.chromium.org/284183011/diff/1/chrome/browser/history/score...
> chrome/browser/history/scored_history_match.h:33: //These values are
duplicated
> from the HistoryURLProvider
> Those constants are only used on scored_history_match.cc so there is no need
to
> make them public. Instead, put them in an anonymous namespace in
> scored_history_match.cc.

Done

[sdefresne, 2014-05-22 16:41:06]

LGTM

Please add blundell@ as a reviewer and send a message with something like
"+blundell@ for OWNERS".

[nshaik, 2014-05-22 17:29:58]

"+blundell@ for OWNERS"

[blundell, 2014-05-26 06:58:35]

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

https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:176: const bool promote_to_inline
= (row.typed_count() > 1) ||
This whole function feels like it should be abstracted through the client.
Sylvain, had you thought about that?

https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:607: if
(also_do_hup_like_scoring_) {
Am I missing something, or is this dead code? Line 597 sets this variable to
false.

[nshaik, 2014-05-30 04:52:03]

Hi Sylvian, can you please let me know, if I need to work on addressing Colin's
comments.

Thanks.

[sdefresne, 2014-05-30 15:22:25]

Hi Naiem,

Yes, I think that Colin's comment makes sense.

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

https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:176: const bool promote_to_inline
= (row.typed_count() > 1) ||
On 2014/05/26 06:58:35, blundell wrote:
> This whole function feels like it should be abstracted through the client.
> Sylvain, had you thought about that?

No, I had not thought of that, but it makes sense to abstract through the client
those few lines (not the whole function since it is the constructor for
ScoredHistoryMatch, but the line computing the hup_like_score).

Once https://codereview.chromium.org/305443004/ lands, then you should be able
to add a HUPLikeScore method to HistoryClient, implement it in
ChromeHistoryClient and call it from here (note that the HistoryClient may be
NULL during testing).

[blundell, 2014-06-02 11:11:48]

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

https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:176: const bool promote_to_inline
= (row.typed_count() > 1) ||
Note: I think that we should come up with a more abstract name than
HUPLikeScore. Is it about giving the client a chance to adjust the score?

On 2014/05/30 15:22:25, sdefresne wrote:
> On 2014/05/26 06:58:35, blundell wrote:
> > This whole function feels like it should be abstracted through the client.
> > Sylvain, had you thought about that?
> 
> No, I had not thought of that, but it makes sense to abstract through the
client
> those few lines (not the whole function since it is the constructor for
> ScoredHistoryMatch, but the line computing the hup_like_score).
> 
> Once https://codereview.chromium.org/305443004/ lands, then you should be able
> to add a HUPLikeScore method to HistoryClient, implement it in
> ChromeHistoryClient and call it from here (note that the HistoryClient may be
> NULL during testing).

[sdefresne, 2014-06-06 07:17:03]

On 2014/06/02 11:11:48, blundell wrote:
>
https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
> File chrome/browser/history/scored_history_match.cc (right):
> 
>
https://codereview.chromium.org/284183011/diff/20001/chrome/browser/history/s...
> chrome/browser/history/scored_history_match.cc:176: const bool
promote_to_inline
> = (row.typed_count() > 1) ||
> Note: I think that we should come up with a more abstract name than
> HUPLikeScore. Is it about giving the client a chance to adjust the score?
> 
> On 2014/05/30 15:22:25, sdefresne wrote:
> > On 2014/05/26 06:58:35, blundell wrote:
> > > This whole function feels like it should be abstracted through the client.
> > > Sylvain, had you thought about that?
> > 
> > No, I had not thought of that, but it makes sense to abstract through the
> client
> > those few lines (not the whole function since it is the constructor for
> > ScoredHistoryMatch, but the line computing the hup_like_score).
> > 
> > Once https://codereview.chromium.org/305443004/ lands, then you should be
able
> > to add a HUPLikeScore method to HistoryClient, implement it in
> > ChromeHistoryClient and call it from here (note that the HistoryClient may
be
> > NULL during testing).

Just a ping to let you know that the HistoryClient is now accessible from the
constructor of ScoredHistoryMatch, so it should be relatively straightforward to
update your CL to abstract the scoring through it.

Cheers,
-- Sylvain

[nshaik, 2014-06-10 08:28:34]

Sylvain,
I have cleaned up the code and for moving the constructor code,
I was thinking I can do it as part of https://codereview.chromium.org/312493003/
or
a different CL.
If this code change and whatever clean up I did is fine. I was hoping we can get
this code in.
Please review.

Thanks,
Naiem

[nshaik, 2014-06-11 08:08:28]

Sylvain,
Please review if the clean up I did is fine.

[sdefresne, 2014-06-11 08:20:14]

Code cleanup looks fine.

[nshaik, 2014-06-11 17:43:35]

The CQ bit was checked by naiem.shaik@gmail.com

[commit-bot: I haz the power, 2014-06-11 17:46:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/naiem.shaik@gmail.com/284183011/60001

[blundell, 2014-06-11 18:16:29]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-06-11 18:17:36]

Not LGTM

I thought the approach we decided on was to have this functionality go through
the embedder?

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (left):

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:205: // If this match is not
inlineable and there's a cap on the maximum
Why did this code go away?

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:588: also_do_hup_like_scoring_ =
false;
This assignment has to stay I would think?

[nshaik, 2014-06-11 20:46:42]

On 2014/06/11 18:17:36, blundell wrote:
> Not LGTM
> 
> I thought the approach we decided on was to have this functionality go through
> the embedder?
> 
>
https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
> File chrome/browser/history/scored_history_match.cc (left):
> 
>
https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
> chrome/browser/history/scored_history_match.cc:205: // If this match is not
> inlineable and there's a cap on the maximum
> Why did this code go away?
> 
>
https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
> chrome/browser/history/scored_history_match.cc:588: also_do_hup_like_scoring_
=
> false;
> This assignment has to stay I would think?

I was waiting for another change to get committed to avoid conflicts.
So I thought I can do the code restructuring as a different CL. I will re do the
change.

[nshaik, 2014-06-11 20:46:50]

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
File chrome/browser/history/scored_history_match.cc (left):

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:205: // If this match is not
inlineable and there's a cap on the maximum
That code is applicable only if max_assigned_score_for_non_inlineable_matches_
!= -1
which is -1 always as also_do_hup_like_scoring_ = false.

On 2014/06/11 18:17:36, blundell wrote:
> Why did this code go away?

https://codereview.chromium.org/284183011/diff/60001/chrome/browser/history/s...
chrome/browser/history/scored_history_match.cc:588: also_do_hup_like_scoring_ =
false;
It is always assigned to false. So I deleted it.
I thought this is some legacy code left for clean up

I will On 2014/06/11 18:17:36, blundell wrote:
> This assignment has to stay I would think?

[blundell, 2014-06-12 08:45:15]

OK, I understand now. Thanks for the explanation.

Could you remove the also_do_hup_like_scoring_ and
max_assigned_score_for_non_inlineable_matches_ variables entirely and make this
CL just about that dead code removal (i.e., leave out the duplication of the
constants, since we know that's not the approach we want to take anyway)?

Thanks!

[nshaik, 2014-06-12 23:08:16]

Done. Please review.

Thanks,
Naiem

[blundell, 2014-06-13 07:39:21]

I believe that the variables are declared in the header file. Could you
eliminate them there too? Thanks!

[nshaik, 2014-06-13 08:40:42]

On 2014/06/13 07:39:21, blundell wrote:
> I believe that the variables are declared in the header file. Could you
> eliminate them there too? Thanks!

Fixed it..please review..

Thanks.

[blundell, 2014-06-13 09:02:52]

LGTM

+sky

Scott, can you confirm that the code being removed is dead (as it looks to be)?
Thanks!

[sky, 2014-06-16 17:16:42]

I believe mpearson added this. mpearson, can we nuke?

[Mark P, 2014-06-16 17:32:25]

On 2014/06/16 17:16:42, sky wrote:
> I believe mpearson added this. mpearson, can we nuke?

Please don't. I'm thinking about restarting this experiment next month.

I could re-add the code then, but that feels silly.

--mark

[blundell, 2014-06-17 14:13:48]

Thanks for the clarification, Mark! Naiem, that puts the kibosh on this CL, but
thanks for the work on it!