Part 6 of search provider refactoring.

Part 6 of search provider refactoring.

Makes changes to how the state is recorded in zero_suggest_provider
to facilitate moving out ParseSuggestResults into super class.

Replaces navigation_results_, query_matches_map_, and
verbatim_relevance_ objects with a Results results_ instance. The latter
keeps track of navigation_results_ and query_results_ and constructs
the query matches map on when it's needed.

verbatim_relevance_ in the old code is only used to see the current url
match (based on the server relevance returned in the previous request
or a static value). The new code keeps track of the server verbatim
relevance in results_ and uses an accessor to get either server results
or a constant value if no server results are available, matching the
old semantics.

Merged FillResults with ParseSuggestResults function and moved match
map creation code into ConvertResultsToAutocompleteMatches where it's
used.

BUG=338955
TBR=mpearson

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

[msw, 2014-02-19 01:11:06]

lgtm with a nit

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

https://codereview.chromium.org/171323002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:114: int
GetVerbatimRelevance();
nit: const

[H Fung, 2014-02-19 06:33:30]

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.cc (right):

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:192: // value on the next
query to set the relevance of the current URL match.
I don't think I understand this comment.

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:289:
results_.verbatim_relevance = -1;
It seems like verbatim_relevance is only set in this function so that it's
uninitialized if the request failed.  There might not be any noticeable behavior
in that case since there's only the URL suggestion, but it might be better to
make sure it's initialized anyway?

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.h (left):

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:86: // TODO(hfung): Refactor
them into a new base class common to both
I think the comment and TODO are still valid, except the 4 should now be 2.

[Maria, 2014-02-19 18:25:53]

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.cc (right):

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:192: // value on the next
query to set the relevance of the current URL match.
On 2014/02/19 06:33:31, H Fung wrote:
> I don't think I understand this comment.

Clear() call resets results_.relevance value, but we want to use that value in
StartZeroSuggest() call to create the current url match, hence we omit resetting
it here.

I'll re-word the comment.

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:289:
results_.verbatim_relevance = -1;
On 2014/02/19 06:33:31, H Fung wrote:
> It seems like verbatim_relevance is only set in this function so that it's
> uninitialized if the request failed.  There might not be any noticeable
behavior
> in that case since there's only the URL suggestion, but it might be better to
> make sure it's initialized anyway?

It's also initialized in Results constructor to -1.

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.h (left):

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:86: // TODO(hfung): Refactor
them into a new base class common to both
On 2014/02/19 06:33:31, H Fung wrote:
> I think the comment and TODO are still valid, except the 4 should now be 2.

I figure that rather than keep updating the countdown, I'd remove the TODO -- I
don't think we are going to need it at the end of the refactoring.

[H Fung, 2014-02-19 21:03:16]

lgtm

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
File chrome/browser/autocomplete/zero_suggest_provider.cc (right):

https://codereview.chromium.org/171323002/diff/70001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:192: // value on the next
query to set the relevance of the current URL match.
On 2014/02/19 18:25:53, Maria wrote:
> On 2014/02/19 06:33:31, H Fung wrote:
> > I don't think I understand this comment.
> 
> Clear() call resets results_.relevance value, but we want to use that value in
> StartZeroSuggest() call to create the current url match, hence we omit
resetting
> it here.
> 
> I'll re-word the comment.

I think there might be a bug since the verbatim score should apply to the
current URL, but I'll look into it later.

[Maria, 2014-02-19 21:51:44]

The CQ bit was checked by mariakhomenko@chromium.org

[Maria, 2014-02-19 21:59:18]

I filed https://code.google.com/p/chromium/issues/detail?id=345002 to track the
verbatim_relevance issue.

[commit-bot: I haz the power, 2014-02-19 22:15:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/171323002/1...

[commit-bot: I haz the power, 2014-02-20 00:49:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/171323002/1...

[commit-bot: I haz the power, 2014-02-20 03:39:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/171323002/1...

[commit-bot: I haz the power, 2014-02-20 08:46:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/171323002/1...

[commit-bot: I haz the power, 2014-02-20 12:19:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/171323002/1...

[commit-bot: I haz the power, 2014-02-20 13:09:09]

Change committed as 252206