Move GoogleImageSearchSource() from template_url.cc to UIThreadSearchTermsData

Move GoogleImageSearchSource() from template_url.cc to UIThreadSearchTermsData

To stop depending on chrome::VersionInfo from TemplateURL.

BUG=386365
TEST=unit_tests

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

[hashimoto, 2014-06-19 19:49:54]

On 2014/06/19 19:45:37, hashimoto wrote:

It should be OK to put this code in UIThreadSearchTermsData as it's only called
on the UI thraed.
Image search uses a URL generated with GoogleImageSearchSource() in
CoreTabHelper::OnRequestThumbnailForContextNodeACK() which runs on the UI
thread.
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)

[Peter Kasting, 2014-06-19 21:05:11]

Why does this have to be in UIThreadSearchTermsData, instead of being in
SearchTermsData?  Can chrome::VersionInfo only be created on the UI thread?  (I
didn't see any obvious thread restrictions in that class.)

[hashimoto, 2014-06-19 22:29:52]

On 2014/06/19 21:05:11, Peter Kasting wrote:
> Why does this have to be in UIThreadSearchTermsData, instead of being in
> SearchTermsData?  Can chrome::VersionInfo only be created on the UI thread? 
(I
> didn't see any obvious thread restrictions in that class.)

This code cannot be placed in SearchTermsData because components/search_engines
cannot depend on chrome/common/chrome_version_info.h.

I thought it was OK to place this code in UIThreadSearchTermsData because
TemplateURL::image_url_ref()'s only user, CoreTabHelper, is living on the UI
thread.
But, as you noted, there is nothing specific to the UI thread in this code.

So I added a new class ChromeSearchTermsData to host Chrome specific code which
is not bound to the UI thread.

Also, fixed SearchTermsDataSnapshot in history_url_provider.cc to store the
newly added property for consistency, even though it's just used to call
IsSearchURL() so the GoogelImageSearchSource() has nothing to do with it.

Other places where SearchTermsData is instantiated is not changed.
(Namely, keyword_table.cc, search_provider_install_data.cc,
template_url_parser.cc, default_search_policy_handler.cc.
https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=%...)
They are not interested in image search URL at all.

[Peter Kasting, 2014-06-19 22:54:23]

I think adding a new base class for UIThreadSearchTermsData is excessive given
that no one actually wants a ChromeSearchTermsData and not a
UIThreadSearchTermsData.

Instead, I would place this implementation in UIThreadSearchTermsData like you
had it, but NOT guard it with a thread-check, and add a comment explicitly
saying that it's actually OK to calculate on any thread, but not in
SearchTermsData in order to avoid a dependence on VersionInfo there, and that if
someone ever needs this from a non-UI thread, this code could be hoisted to a
new class that lived in between SearchTermsData and UIThreadSearchTermsData.

[hashimoto, 2014-06-19 23:15:29]

On 2014/06/19 22:54:23, Peter Kasting wrote:
> I think adding a new base class for UIThreadSearchTermsData is excessive given
> that no one actually wants a ChromeSearchTermsData and not a
> UIThreadSearchTermsData.
> 
> Instead, I would place this implementation in UIThreadSearchTermsData like you
> had it, but NOT guard it with a thread-check, and add a comment explicitly
> saying that it's actually OK to calculate on any thread, but not in
> SearchTermsData in order to avoid a dependence on VersionInfo there, and that
if
> someone ever needs this from a non-UI thread, this code could be hoisted to a
> new class that lived in between SearchTermsData and UIThreadSearchTermsData.

Added comments, and removed ChromeSearchTermsData and the DCHECK.

[Peter Kasting, 2014-06-20 00:33:03]

LGTM

https://codereview.chromium.org/345803002/diff/60001/chrome/browser/search_en...
File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right):

https://codereview.chromium.org/345803002/diff/60001/chrome/browser/search_en...
chrome/browser/search_engines/ui_thread_search_terms_data.cc:150: // It's
acutally OK to call this method on any threads, but it's currently
Nit: threads -> thread

[hashimoto, 2014-06-20 05:09:28]

https://codereview.chromium.org/345803002/diff/60001/chrome/browser/search_en...
File chrome/browser/search_engines/ui_thread_search_terms_data.cc (right):

https://codereview.chromium.org/345803002/diff/60001/chrome/browser/search_en...
chrome/browser/search_engines/ui_thread_search_terms_data.cc:150: // It's
acutally OK to call this method on any threads, but it's currently
On 2014/06/20 00:33:03, Peter Kasting wrote:
> Nit: threads -> thread

Done.

[hashimoto, 2014-06-20 05:09:34]

The CQ bit was checked by hashimoto@chromium.org

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

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

[hashimoto, 2014-06-20 06:03:05]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-06-20 06:04:56]

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

[commit-bot: I haz the power, 2014-06-20 13:40:32]

Change committed as 278702