Part 1 of search provider refactoring.

Part 1 of search provider refactoring.

Creates a base class shared by zero_suggest_provider and search_provider
which will contain common implementation pieces for both classes.

Moves Result, NavigationsResults, SuggestResult, Results objects into
the base class without any changes to the implementation. These objects
are used by both classes.

Moves common typedefs up: MatchKey, MatchMap, NavigationResults,
SuggestResults.

BUG=338955
TBR=mpearson@chromium.org

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

[Maria, 2014-01-29 03:50:55]

I am taking Mike's suggestion on: https://codereview.chromium.org/131433003/ and
splitting up the big refactoring into easier-to-review portions. I am hoping
that we could have faster turn-around on these changes. Eventually, I imagine
the result will be fairly close to what I had in the original review by done in
a step-by-step process, which hopefully will make verifying correctness a little
simpler.

[H Fung, 2014-01-30 02:35:33]

lgtm

I assume that most of the code was copy/pasted, but it otherwise seemed
straightforward to me.  Maybe some the static functions
SearchProvider::CreateSearchSuggestion, SearchProvider::DeserializeJsonData, and
SearchProvider::CanSendURL can be moved in this change too if the other
reviewers agree.

https://codereview.chromium.org/131003011/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:186: const TemplateURL*
SearchProvider::Providers::GetKeywordProviderURL()
I'm not sure why you changed this since it had fit on one line.

[Maria, 2014-01-30 02:47:02]

I already prepared a separate change to move the functions you mention over, so
I would prefer to keep this CL as-is.

Mark, your owners approval would be much appreciated.

https://codereview.chromium.org/131003011/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:186: const TemplateURL*
SearchProvider::Providers::GetKeywordProviderURL()
On 2014/01/30 02:35:33, H Fung wrote:
> I'm not sure why you changed this since it had fit on one line.

Done.

[msw, 2014-02-03 22:53:14]

LGTM with nits; some more worthwhile than others.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:9: #include
"chrome/browser/autocomplete/autocomplete_input.h"
nit: remove redundant includes from base_search_provider.h like this.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:165: base::string16
match_contents = net::FormatUrl(url_,
nit: I prefer the old wrapped arguments version of this call.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:181:
AutocompleteMatch::ClassifyLocationInString(match_start,
nit: I prefer the old wrapped arguments version of this call.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:222: ++i) {
nit: I prefer this being wrapped on the line above; ditto below.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:238:
BaseSearchProvider::~BaseSearchProvider() {}
nit: reorder this according to any change in the header.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.h (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:21: #include
"chrome/browser/autocomplete/autocomplete_provider_listener.h"
nit: use a forward declaration instead.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:22: #include
"chrome/browser/profiles/profile.h"
nit: use a forward declaration instead.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:28: // autocomplete providers
utilizing the functionality.
nit: s/the/its/

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:132: const OVERRIDE;
nit: avoid wrapping like this; keep it as it was:
virtual int CalculateRelevance(
    const AutocompleteInput& input,
    bool keyword_provider_requested) const OVERRIDE;

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:242: virtual
~BaseSearchProvider();
nit: order this immediately after the ctor.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:247: #endif  //
CHROME_BROWSER_AUTOCOMPLETE_BASE_SEARCH_PROVIDER_H_
nit: add a blank line above.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:23: #include
"chrome/browser/autocomplete/autocomplete_match.h"
nit: remove redundant includes from search_provider.h, etc. like this.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:26: #include
"chrome/browser/autocomplete/base_search_provider.h"
nit: remove this; it's included in search_provider.h

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:24: #include
"chrome/browser/autocomplete/autocomplete_input.h"
nit: you can optionally remove includes here that are now part of
base_search_provider.h: autocomplete_input.h, autocomplete_match.h,
autocomplete_provider.h, url_fetcher_delegate.h

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

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:21: #include
"chrome/browser/autocomplete/base_search_provider.h"
nit: remove redundant includes like this.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:188: profile,
nit: this can be wrapped on the line above.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:202: const base::Value&
root_val,
nit: wrap this on the line above; indent others to match.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:458: it !=
navigation_results_.end(); ++it)
nit: indent one more space.

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

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:25: #include
"chrome/browser/autocomplete/autocomplete_provider_listener.h"
nit: remove redundant includes like this.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:30: class AutocompleteInput;
nit: remove redundant forward declarations like this.

[Mark P, 2014-02-03 23:02:12]

I'm removing myself from the reviewers list because msw@ is a more appropriate
reviewer than me, and there's no need for additional (redundant) comments from
me.

--mark

[mariakhomenko, 2014-02-03 23:37:25]

Mark,

AFAIK there are three people in the OWNERS file for autocomplete: pkasting,
mpearson, and sky. My understanding is that I'll need an LGTM from one of
you to submit. Once I address the comments, would you be able to do OWNERS
approvals?


On Mon, Feb 3, 2014 at 3:02 PM, <mpearson@chromium.org> wrote:

>
> I'm removing myself from the reviewers list because msw@ is a more
> appropriate
> reviewer than me, and there's no need for additional (redundant) comments
> from
> me.
>
> --mark
>
> https://codereview.chromium.org/131003011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Maria, 2014-02-04 03:16:38]

Just a couple of general notes:
1) The re-wrapping of arguments was done via "git cl format". I've changed it
back where you commented -- won't use auto-formatter after this.

2) I am a bit surprised you suggest I remove duplicate includes in subclasses
since it seems against "include what you use" rule, but I don't have a strong
preference, so I de-duped.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:9: #include
"chrome/browser/autocomplete/autocomplete_input.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: remove redundant includes from base_search_provider.h like this.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:165: base::string16
match_contents = net::FormatUrl(url_,
On 2014/02/03 22:53:14, msw wrote:
> nit: I prefer the old wrapped arguments version of this call.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:181:
AutocompleteMatch::ClassifyLocationInString(match_start,
On 2014/02/03 22:53:14, msw wrote:
> nit: I prefer the old wrapped arguments version of this call.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:222: ++i) {
On 2014/02/03 22:53:14, msw wrote:
> nit: I prefer this being wrapped on the line above; ditto below.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.cc:238:
BaseSearchProvider::~BaseSearchProvider() {}
On 2014/02/03 22:53:14, msw wrote:
> nit: reorder this according to any change in the header.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/base_search_provider.h (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:21: #include
"chrome/browser/autocomplete/autocomplete_provider_listener.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: use a forward declaration instead.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:22: #include
"chrome/browser/profiles/profile.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: use a forward declaration instead.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:28: // autocomplete providers
utilizing the functionality.
On 2014/02/03 22:53:14, msw wrote:
> nit: s/the/its/

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:132: const OVERRIDE;
On 2014/02/03 22:53:14, msw wrote:
> nit: avoid wrapping like this; keep it as it was:
> virtual int CalculateRelevance(
>     const AutocompleteInput& input,
>     bool keyword_provider_requested) const OVERRIDE;

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:242: virtual
~BaseSearchProvider();
On 2014/02/03 22:53:14, msw wrote:
> nit: order this immediately after the ctor.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:247: #endif  //
CHROME_BROWSER_AUTOCOMPLETE_BASE_SEARCH_PROVIDER_H_
On 2014/02/03 22:53:14, msw wrote:
> nit: add a blank line above.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:23: #include
"chrome/browser/autocomplete/autocomplete_match.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: remove redundant includes from search_provider.h, etc. like this.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:26: #include
"chrome/browser/autocomplete/base_search_provider.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: remove this; it's included in search_provider.h

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
File chrome/browser/autocomplete/search_provider.h (right):

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.h:24: #include
"chrome/browser/autocomplete/autocomplete_input.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: you can optionally remove includes here that are now part of
> base_search_provider.h: autocomplete_input.h, autocomplete_match.h,
> autocomplete_provider.h, url_fetcher_delegate.h

Done.

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

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:21: #include
"chrome/browser/autocomplete/base_search_provider.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: remove redundant includes like this.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:188: profile,
On 2014/02/03 22:53:14, msw wrote:
> nit: this can be wrapped on the line above.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:202: const base::Value&
root_val,
On 2014/02/03 22:53:14, msw wrote:
> nit: wrap this on the line above; indent others to match.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.cc:458: it !=
navigation_results_.end(); ++it)
On 2014/02/03 22:53:14, msw wrote:
> nit: indent one more space.

Done.

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

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:25: #include
"chrome/browser/autocomplete/autocomplete_provider_listener.h"
On 2014/02/03 22:53:14, msw wrote:
> nit: remove redundant includes like this.

Done.

https://codereview.chromium.org/131003011/diff/30001/chrome/browser/autocompl...
chrome/browser/autocomplete/zero_suggest_provider.h:30: class AutocompleteInput;
On 2014/02/03 22:53:14, msw wrote:
> nit: remove redundant forward declarations like this.

Done.

[msw, 2014-02-04 03:29:54]

On 2014/02/04 03:16:38, Maria wrote:
> Just a couple of general notes:
> 1) The re-wrapping of arguments was done via "git cl format". I've changed it
> back where you commented -- won't use auto-formatter after this.

Most were innocuous, except wrapping "const OVERRIDE" to its own line.
They're mostly minor preferences; thanks for addressing them.

> 2) I am a bit surprised you suggest I remove duplicate includes in subclasses
> since it seems against "include what you use" rule, but I don't have a strong
> preference, so I de-duped.

I like IWYU, but also like transitive inclusion of necessary dependencies.
Again, these were mostly minor preferences; thanks for addressing them.

Still LGTM.

[Maria, 2014-02-04 07:45:27]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2014-02-04 07:46:32]

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

[commit-bot: I haz the power, 2014-02-04 08:53:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-04 08:53:10]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Maria, 2014-02-04 18:01:04]

The CQ bit was checked by mariakhomenko@chromium.org

[commit-bot: I haz the power, 2014-02-04 18:03:26]

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

[commit-bot: I haz the power, 2014-02-04 20:08:40]

Retried try job too often on ios_dbg_simulator for step(s) url_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2014-02-04 20:11:45]

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

[commit-bot: I haz the power, 2014-02-05 00:48:16]

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

[commit-bot: I haz the power, 2014-02-05 05:18:35]

Change committed as 248856