Remove protected virtual methods from BaseSearchProvider

Remove protected virtual methods from BaseSearchProvider

This change is a follow-up of https://codereview.chromium.org/436833002/.

-GetTemplateURL(), GetInput() and ShouldAppendExtraParams()
Instead of calling virtual methods, pass these values as arguments to
AddMatchToMap() and ParseSuggestResults().

-StopSuggest() and ClearAllResults()
These methods are used to customize the behavior of Stop().
Instead, define Stop() in SearchProvider and ZeroSuggestProvider.

-ModifyProviderInfo()
This method is only needed by ZeroSuggestProvider.
Instead, ZeroSuggestProvider overrides AddProviderInfo() to do the same thing.

BUG=None

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

[Peter Kasting, 2014-08-12 18:31:18]

I don't really understand why this is useful.

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

https://codereview.chromium.org/456843003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:56: const
base::UserMetricsAction& uma_deletion_failure,
Unfortunately you can't pass in UMAs like this.  RecordAction() has to take the
name of the action directly as a string literal, because there are tools that
scan our codebase looking for these calls and recording the strings passed to
them.

This is also why our current uses of these aren't using ?:.

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

https://codereview.chromium.org/456843003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:303: return is_keyword ?
providers_.GetKeywordProviderURL()
Nit: clang-format error, please break after ? and not before : (break after : if
you need to break again)

[hashimoto, 2014-08-13 05:21:22]

Sorry for forgetting to describe the purpose of this change.
This change is a follow-up of https://codereview.chromium.org/436833002/ which
aims at improving the readability of the code.

Protected virtual methods makes it difficult to understand SearchProvider's
behavior because, every time the reader encounters a method call, they have to
look up the header to know if a method is defined in SearchProvider or
BaseSearchProvider.
By removing protected virtual methods from BaseSearchProvider, at least in
base_search_provider.cc, we can be sure that the definition of the method being
called can be found in the same file without looking up the header.
(this is one of the reasons why our style guide dislikes implementation
inheritance
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inheri...)

We still have to look up the header when reading search_provider.cc because
there are still non-virtual protected methods defined in
base_search_provider.cc, but it's better than the current state.


Regarding each method being removed:
-GetTemplateURL(), GetInput() and ShouldAppendExtraParams()
Instead of calling virtual methods, pass these values as arguments to
AddMatchToMap() and ParseSuggestResults().

-StopSuggest() and ClearAllResults()
These methods are used to customize the behavior of Stop().
Instead, define Stop() in SearchProvider and ZeroSuggestProvider.

-ModifyProviderInfo()
This method is only needed by ZeroSuggestProvider.
Instead, ZeroSuggestProvider overrides AddProviderInfo() to do the same thing.

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

https://codereview.chromium.org/456843003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/base_search_provider.h:56: const
base::UserMetricsAction& uma_deletion_failure,
On 2014/08/12 18:31:18, Peter Kasting wrote:
> Unfortunately you can't pass in UMAs like this.  RecordAction() has to take
the
> name of the action directly as a string literal, because there are tools that
> scan our codebase looking for these calls and recording the strings passed to
> them.
> 
> This is also why our current uses of these aren't using ?:.

Ugh, you're right.
I should have carefully read the comments in
base/metrics/user_metrics_action.h...

Revived RecordDeletionResult().

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

https://codereview.chromium.org/456843003/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/search_provider.cc:303: return is_keyword ?
providers_.GetKeywordProviderURL()
On 2014/08/12 18:31:18, Peter Kasting wrote:
> Nit: clang-format error, please break after ? and not before : (break after :
if
> you need to break again)

Done.

[Peter Kasting, 2014-08-13 06:19:36]

On 2014/08/13 05:21:22, hashimoto wrote:
> Sorry for forgetting to describe the purpose of this change.
> This change is a follow-up of https://codereview.chromium.org/436833002/ which
> aims at improving the readability of the code.
> 
> Protected virtual methods makes it difficult to understand SearchProvider's
> behavior because, every time the reader encounters a method call, they have to
> look up the header to know if a method is defined in SearchProvider or
> BaseSearchProvider.

I don't find that to be a readability hindrance.  MSVC has right click->Go To
Definition that works for this.  This is what IDEs (and code search) are made
for.

I'm going to evaluate this change on a function-by-function basis.

> Regarding each method being removed:
> -GetTemplateURL(), GetInput() and ShouldAppendExtraParams()
> Instead of calling virtual methods, pass these values as arguments to
> AddMatchToMap() and ParseSuggestResults().

Doing this significantly expands the multiple callsites throughout
search_provider.cc for what doesn't feel like much gain.  I think we should
revert this group of changes.

> -StopSuggest() and ClearAllResults()
> These methods are used to customize the behavior of Stop().
> Instead, define Stop() in SearchProvider and ZeroSuggestProvider.

This is fine, but inline the definitions of StopSuggest() and ClearAllResults()
into SearchProvider::Stop() the way you did in ZeroSuggestProvider::Stop(),
since that's the only callsite.

> -ModifyProviderInfo()
> This method is only needed by ZeroSuggestProvider.
> Instead, ZeroSuggestProvider overrides AddProviderInfo() to do the same thing.

This is fine.

[hashimoto, 2014-08-13 08:59:43]

On 2014/08/13 06:19:36, pkasting (away thru Aug 20) wrote:
> On 2014/08/13 05:21:22, hashimoto wrote:
> > Sorry for forgetting to describe the purpose of this change.
> > This change is a follow-up of https://codereview.chromium.org/436833002/
which
> > aims at improving the readability of the code.
> > 
> > Protected virtual methods makes it difficult to understand SearchProvider's
> > behavior because, every time the reader encounters a method call, they have
to
> > look up the header to know if a method is defined in SearchProvider or
> > BaseSearchProvider.
> 
> I don't find that to be a readability hindrance.  MSVC has right click->Go To
> Definition that works for this.  This is what IDEs (and code search) are made
> for.
> 
> I'm going to evaluate this change on a function-by-function basis.
> 
> > Regarding each method being removed:
> > -GetTemplateURL(), GetInput() and ShouldAppendExtraParams()
> > Instead of calling virtual methods, pass these values as arguments to
> > AddMatchToMap() and ParseSuggestResults().
> 
> Doing this significantly expands the multiple callsites throughout
> search_provider.cc for what doesn't feel like much gain.  I think we should
> revert this group of changes.
OK, reverted this part.
> 
> > -StopSuggest() and ClearAllResults()
> > These methods are used to customize the behavior of Stop().
> > Instead, define Stop() in SearchProvider and ZeroSuggestProvider.
> 
> This is fine, but inline the definitions of StopSuggest() and
ClearAllResults()
> into SearchProvider::Stop() the way you did in ZeroSuggestProvider::Stop(),
> since that's the only callsite.
> 
> > -ModifyProviderInfo()
> > This method is only needed by ZeroSuggestProvider.
> > Instead, ZeroSuggestProvider overrides AddProviderInfo() to do the same
thing.
> 
> This is fine.

[Peter Kasting, 2014-08-13 17:33:45]

LGTM

https://codereview.chromium.org/456843003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/456843003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider.cc:318: void
SearchProvider::StopSuggest() {
Nit: You still haven't inlined these two functions into Stop().

[hashimoto, 2014-08-14 03:52:10]

https://codereview.chromium.org/456843003/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/456843003/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/search_provider.cc:318: void
SearchProvider::StopSuggest() {
On 2014/08/13 17:33:45, pkasting (away thru Aug 20) wrote:
> Nit: You still haven't inlined these two functions into Stop().

Sorry to not mentioning this.
StopSuggest() and ClearAllResults() are also called in StartOrStopSuggestQuery()
so we cannot inline them in Stop() straightforwardly.

[hashimoto, 2014-08-18 05:37:06]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-08-18 05:38:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hashimoto@chromium.org/456843003/120001

[commit-bot: I haz the power, 2014-08-18 07:45:20]

Committed patchset #4 (120001) as 290222