Misc. cleanup:

Misc. cleanup:

* Reduce indenting levels
* Align args per style guide
* Improve (or remove) comments
* Pass around TemplateURL*s where convenient and safe instead of converting
  to/from keywords.  This generally simplifies things slightly and will be
  useful for subsequent changes that need the TemplateURL*s in more places.
* Don't handle DCHECK failure
* Nuke long-outdated disabled KeywordProvider unittest
* Attempt (and probably fail) to put SearchProvider::CreateSearchSuggestion()
  args in some sort of order
* Remove using directives where they don't save lines
* Follow style guide class declaration order
* Use OVERRIDE
* Make function declaration and definition order match
* Shorten/simplify code
* Put all three IsGoogleXXX() functions that take URLs in one group
* Eliminate unnecessary #includes
* Add GetDefaultSearchURLForSearchTerms() function to c/b/search_engines/util.h
  since many disparate callers want to do that (and in a subsequent change I'll
  want to slightly modify how that works)
* Fix presubmit warning about mismatched braces
* Remove unnecessary ResetDefaultTemplateURL() calls from ToolbarModel tests

BUG=249197
TEST=none
R=cpu@chromium.org, jered@chromium.org

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

[Peter Kasting, 2013-06-27 22:57:53]

Jered: This is all the cleanup split off from
https://codereview.chromium.org/17022004/ , so you'll have reviewed some of this
already (which is why I'm sending it to you in the first place), but please
re-review since I'm sure some bits are new.

Carlos: OWNER for c/b/metro_viewer.

[cpu_(ooo_6.6-7.5), 2013-06-28 20:53:53]

lgtm for my file

[Jered, 2013-06-28 21:57:28]

lgtm

I have a bunch of nits. This is indeed pretty "misc".

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:476: 
Consider const string16& keyword = template_url()->keyword() here since it's
used a bunch.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:574:
model->GetTemplateURLForKeyword(keyword), input, keyword.length(),
Move this outside the loop.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:197: bool is_home_page_base = (path ==
"/webhp") || (path == "/");
nit: You could extract a helper function and call it here and at 186 now.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search/sear...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search/sear...
chrome/browser/search/search.cc:426: // Extended mode requires HTTPS.  Force it.
Move this comment down below the if, since that's the code that does the
forcing.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search_engi...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search_engi...
chrome/browser/search_engines/util.h:28: GURL
GetDefaultSearchURLForSearchTerms(Profile* profile, const string16& terms);
I'm surprised this doesn't already exist. This seems like the main thing you
would want to do with a database of search engine URLs.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:393: profile,
param.LossyDisplayName().substr(2)));
Keep string16 search_term = param.LossyDisplayName().substr(2) to make it clear
that "param" is a search term.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_win.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_win.cc:19: string16 params;
"params" sounds confusingly plural.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_win.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_win.cc:471: browser->profile(),
reinterpret_cast<const wchar_t*>(l_param));
Keep the named temp for search terms.

[Peter Kasting, 2013-06-28 22:16:01]

One question, repeated in two places below.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
File chrome/browser/autocomplete/keyword_provider.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:476: 
On 2013/06/28 21:57:28, Jered wrote:
> Consider const string16& keyword = template_url()->keyword() here since it's
> used a bunch.

Done.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/autocomplet...
chrome/browser/autocomplete/keyword_provider.cc:574:
model->GetTemplateURLForKeyword(keyword), input, keyword.length(),
On 2013/06/28 21:57:28, Jered wrote:
> Move this outside the loop.

Done.  Nice catch.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/google/goog...
File chrome/browser/google/google_util.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/google/goog...
chrome/browser/google/google_util.cc:197: bool is_home_page_base = (path ==
"/webhp") || (path == "/");
On 2013/06/28 21:57:28, Jered wrote:
> nit: You could extract a helper function and call it here and at 186 now.

Done.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search/sear...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search/sear...
chrome/browser/search/search.cc:426: // Extended mode requires HTTPS.  Force it.
On 2013/06/28 21:57:28, Jered wrote:
> Move this comment down below the if, since that's the code that does the
> forcing.

I agree, but I'm going to leave it where it is only because my next patch
modifies this comment to refer more deeply to the conditional.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search_engi...
File chrome/browser/search_engines/util.h (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/search_engi...
chrome/browser/search_engines/util.h:28: GURL
GetDefaultSearchURLForSearchTerms(Profile* profile, const string16& terms);
On 2013/06/28 21:57:28, Jered wrote:
> I'm surprised this doesn't already exist. This seems like the main thing you
> would want to do with a database of search engine URLs.

Well, as you can tell from this patch, I can use this in three or four existing
places.  But beyond that there aren't that many places in the frontend that need
to search for things -- the omnibox is the chief originator of searches, and it
doesn't use this kind of a simple helper.

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:393: profile,
param.LossyDisplayName().substr(2)));
On 2013/06/28 21:57:28, Jered wrote:
> Keep string16 search_term = param.LossyDisplayName().substr(2) to make it
clear
> that "param" is a search term.

You don't think that's clear from the function name and the comment above?

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator_win.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator_win.cc:19: string16 params;
On 2013/06/28 21:57:28, Jered wrote:
> "params" sounds confusingly plural.

It can be plural, AFAICT, in that we can have multiple launch params.  (I don't
really know, I'm not changing that part of the code, but that's the function
name.)

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_win.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_win.cc:471: browser->profile(),
reinterpret_cast<const wchar_t*>(l_param));
On 2013/06/28 21:57:28, Jered wrote:
> Keep the named temp for search terms.

Same question, you think that really buys much over just having the function
name?

[Jered, 2013-06-28 22:22:28]

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_frame_win.cc (right):

https://codereview.chromium.org/18119005/diff/2007/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_frame_win.cc:471: browser->profile(),
reinterpret_cast<const wchar_t*>(l_param));
On 2013/06/28 22:16:02, Peter Kasting wrote:
> On 2013/06/28 21:57:28, Jered wrote:
> > Keep the named temp for search terms.
> 
> Same question, you think that really buys much over just having the function
> name?

I mostly like it for code search, so there's a line that says "search_terms =
l_param" somewhere. But this is a super nit so feel free to ignore it.

[Peter Kasting, 2013-06-28 22:52:22]

Committed patchset #7 manually as r209228 (presubmit successful).