Move GenerateSearchURL() and GenerateKeyword() from TemplateURLService to TemplateURL

Move GenerateSearchURL() and GenerateKeyword() from TemplateURLService to TemplateURL

TemplateURL should not depend on TemplateURLService.

BUG=386365
TEST=unit_tests
R=pkasting@chromium.org
TBR=profile_resetter, renderer_context_menu}, sky@chromium.org for chrome/browser/{importer

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

[Peter Kasting, 2014-06-19 00:17:50]

Is this because you don't ultimately intend to componentize TemplateURLService? 
I'm not sure I understand why TemplateURL can't depend on it.

I'm not really opposed to the change, but the motivation is unclear.

https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url.h:655: static base::string16
GenerateKeyword(const GURL& url);
Nit: Place this with the other static methods above.

[hashimoto, 2014-06-19 01:06:00]

On 2014/06/19 00:17:50, Peter Kasting wrote:
> Is this because you don't ultimately intend to componentize
TemplateURLService? 
> I'm not sure I understand why TemplateURL can't depend on it.
I'm going to componentize TemplateURLService after TemplateURL.
But I want TemplateURL to be independent of TemplateURLService to componentize
it first.
This enables us to componentize classes in search_engines and other directories
before finishing componentization of TemplateURLService.

Besides the componentization easiness, IMO we should avoid circular dependencies
between classes when possible
so that changes in TemplateURLService never affect TemplateURL.
This should make future bug analysis and future refactoring effort easier.

Since these two methods being moved in this change are using nothing in
TemplateURLService, I saw no problem in having them in TemplateURL.
Do these two methods have a reason to be in TemplateURLService, not in
TemplateURL?
> 
> I'm not really opposed to the change, but the motivation is unclear.
> 
>
https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
> File chrome/browser/search_engines/template_url.h (right):
> 
>
https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
> chrome/browser/search_engines/template_url.h:655: static base::string16
> GenerateKeyword(const GURL& url);
> Nit: Place this with the other static methods above.

[hashimoto, 2014-06-19 01:11:45]

https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/343823002/diff/20001/chrome/browser/search_en...
chrome/browser/search_engines/template_url.h:655: static base::string16
GenerateKeyword(const GURL& url);
On 2014/06/19 00:17:49, Peter Kasting wrote:
> Nit: Place this with the other static methods above.

Moved to below GenerateFaviconURL() which is somehow similar to this method.

[Peter Kasting, 2014-06-19 17:41:59]

On 2014/06/19 01:06:00, hashimoto wrote:
> Besides the componentization easiness, IMO we should avoid circular
dependencies
> between classes when possible
> so that changes in TemplateURLService never affect TemplateURL.
> This should make future bug analysis and future refactoring effort easier.

Yeah, I agree that's probably preferable.

> Since these two methods being moved in this change are using nothing in
> TemplateURLService, I saw no problem in having them in TemplateURL.
> Do these two methods have a reason to be in TemplateURLService, not in
> TemplateURL?

I originally put them in TemplateURLService because I was trying to make
TemplateURL as slim and devoid of API as I could.  I don't have a problem with
moving them, though.  GenerateSearchURL() in particular should probably have
been moved regardless.

LGTM

https://codereview.chromium.org/343823002/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/343823002/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/template_url.h:488: static base::string16
GenerateKeyword(const GURL& url);
Tiny nit: Given that the keyword member comes before the favicon one, I'd
probably put this function above GenerateFaviconURL().  (I didn't think about
this yesterday or I would have suggested it.)

[hashimoto, 2014-06-19 17:56:05]

https://codereview.chromium.org/343823002/diff/40001/chrome/browser/search_en...
File chrome/browser/search_engines/template_url.h (right):

https://codereview.chromium.org/343823002/diff/40001/chrome/browser/search_en...
chrome/browser/search_engines/template_url.h:488: static base::string16
GenerateKeyword(const GURL& url);
On 2014/06/19 17:41:59, Peter Kasting wrote:
> Tiny nit: Given that the keyword member comes before the favicon one, I'd
> probably put this function above GenerateFaviconURL().  (I didn't think about
> this yesterday or I would have suggested it.)

Makes sense.
Done.

[hashimoto, 2014-06-19 17:56:07]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-19 18:00:25]

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

[commit-bot: I haz the power, 2014-06-19 21:40:48]

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

[commit-bot: I haz the power, 2014-06-19 21:40:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[hashimoto, 2014-06-19 21:46:32]

TBRing sky@chromium.org for changes in
chrome/browser/{importer,profile_resetter,renderer_context_menu}

[hashimoto, 2014-06-19 21:46:36]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-19 21:47:30]

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

[commit-bot: I haz the power, 2014-06-19 23:32:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-19 23:43:13]

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

[commit-bot: I haz the power, 2014-06-19 23:43:14]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[hashimoto, 2014-06-19 23:56:12]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-19 23:57:47]

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

[commit-bot: I haz the power, 2014-06-20 08:22:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 11:24:37]

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

[commit-bot: I haz the power, 2014-06-20 11:24:39]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[hashimoto, 2014-06-20 15:07:21]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:09:31]

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

[commit-bot: I haz the power, 2014-06-20 15:17:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-20 15:23:14]

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

[commit-bot: I haz the power, 2014-06-20 15:23:15]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[hashimoto, 2014-06-20 15:38:39]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-20 15:39:12]

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

[hashimoto, 2014-06-20 16:56:25]

The CQ bit was unchecked by hashimoto@chromium.org

[hashimoto, 2014-06-20 16:56:43]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-20 17:00:51]

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

[hashimoto, 2014-06-20 17:36:29]

Committed patchset #5 manually as r278751 (presubmit successful).