Partially componentize //chrome/browser/search/search.{h,cc}

Partially componentize //chrome/browser/search/search.{h,cc}

Move functions from //chrome/browser/search/search.{h,cc} to the search
component //components/search/search.{h,cc} so that they can be shared
with iOS.

Move all the code in those two files (and supporting unittests) from
the "chrome" to the "search" namespace.

Move unittests that do not depends on non-componentized functions into
the search component.

Implements GetSearchTerms() on iOS that uses a web::WebState* instead
of content::WebContents and simplify the code to remove unsupported
features.

Directly use //components/search on iOS instead of the SearchProvider
when possible.

BUG=514239
TBR=sky,droger,benwells

Committed: https://crrev.com/51bbec7be7014d4d75d39c1a5c27b1ba9ddc3dcd
Cr-Commit-Position: refs/heads/master@{#341525}

[sdefresne, 2015-07-29 11:27:20]

Patchset #3 (id:40001) has been deleted

[sdefresne, 2015-07-29 11:27:26]

Patchset #2 (id:20001) has been deleted

[sdefresne, 2015-07-29 11:27:31]

Patchset #1 (id:1) has been deleted

[sdefresne, 2015-07-29 11:34:30]

sdefresne@chromium.org changed reviewers:
+ blundell@chromium.org, samarth@chromium.org

[sdefresne, 2015-07-29 11:34:31]

blundell: please take a look at //ios

samarth: please take a look at //chrome/browser/search & //components/search

Please note that I only componentized what was used on iOS. I also moved the
code into the search namespace instead of chrome since it does not really make
sense to have code in chrome namespace in a component.

[blundell, 2015-07-29 12:53:40]

LGTM

[sdefresne, 2015-07-30 08:07:51]

samarth: ping

[sdefresne, 2015-07-31 07:46:30]

sdefresne@chromium.org changed reviewers:
+ jered@chromium.org
- samarth@chromium.org

[sdefresne, 2015-07-31 07:46:32]

jered: can you take a look as //chrome/browser/search OWNERS?

[Jered, 2015-07-31 14:00:33]

On 2015/07/31 07:46:32, sdefresne wrote:
> jered: can you take a look as //chrome/browser/search OWNERS?

This change is huge. :-) I skimmed it and got the rough idea that some code is
moving from //c/b/s,
but the diffs are very hard to follow.

Are there any feature or behavior changes here---deleting or adding flags or
code---that you're
particularly worried about, or is it purely mechanical?

[sdefresne, 2015-07-31 14:47:42]

On 2015/07/31 at 14:00:33, jered wrote:
> On 2015/07/31 07:46:32, sdefresne wrote:
> > jered: can you take a look as //chrome/browser/search OWNERS?
> 
> This change is huge. :-) I skimmed it and got the rough idea that some code is
moving from //c/b/s,
> but the diffs are very hard to follow.
> 
> Are there any feature or behavior changes here---deleting or adding flags or
code---that you're
> particularly worried about, or is it purely mechanical?

There are no behaviour changes.

The change summary is:
- renamed "chrome" namespace to "search" in chrome/browser/search/search.h and
components/search/search.h
  - changed all client code using "tools/git/mffr.py chrome::xxxx search::xxxx"
for all exported name
  - fixed code that was using the names exported and was also in the "chrome"
namespace
- moved the following methods and constants from chrome/browser/search/search.h
to components/search/search.h
  - kDisableStartMargin
  - InstantExtendedEnabledParam
  - ForceInstantResultsParam
  - IsQueryExtractionEnabled
  - ShouldPrefetchSearchResults
  - ShouldReuseInstantSearchBasePage
  - ShouldAllowPrefetchNonDefaultMatch
  - IsSuitableURLForInstant
  - EnableQueryExtractionForTesting
- added missing #include of components/search/search.h for code using those
constants/methods

All the interesting bits (i.e. code move, no functional change) are in
chrome/browser/search & components/search, the rest is mechanical.

[Jered, 2015-07-31 15:38:45]

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
File components/search/search.cc (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.cc:251: #if !defined(OS_IOS) && !defined(OS_ANDROID)
This #if guard wasn't here before. did you intend to add it?

https://codereview.chromium.org/1260033003/diff/90001/components/search/search.h
File components/search/search.h (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.h:102: bool IsSuitableURLForInstant(const GURL& url,
const TemplateURL* template_url);
This function prototype wasn't exposed previously. Is there a caller outside
search.cc, now?

[sdefresne, 2015-07-31 17:09:31]

Thank you for your review. I've tried to answer your questions. Please take
another look.

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
File components/search/search.cc (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.cc:251: #if !defined(OS_IOS) && !defined(OS_ANDROID)
On 2015/07/31 at 15:38:45, Jered wrote:
> This #if guard wasn't here before. did you intend to add it?

Yes, the command line flag kEnableQueryExtraction is only used in
IsQueryExtractionEnabled() and the code using it is not compiled on Android nor
iOS. I can remove the #if guard if you prefer (but this mean the flag need to be
defined on all platforms).

https://codereview.chromium.org/1260033003/diff/90001/components/search/search.h
File components/search/search.h (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.h:102: bool IsSuitableURLForInstant(const GURL& url,
const TemplateURL* template_url);
On 2015/07/31 at 15:38:45, Jered wrote:
> This function prototype wasn't exposed previously. Is there a caller outside
search.cc, now?

There were three call sites in chrome/browser/search/search.cc, but two of them
in function that cannot be moved into the component "bool IsInstantURL(const
GURL& url, Profile* profile)" and "bool IsQueryExtractionAllowedForURL(Profile*
profile, const GURL& url)" as they depends on Profile (which cannot be used in a
component as it is defined in src/chrome/browser).

If you do not want this to be public, I can put it in a search_internal.h file
and add a comment saying that the content of the file should only be used by
components/search and chrome/browser/search (maybe with a
-components/search/search_internal.h DEPS in //chrome/browser with the
corresponding +components/search/search_internal.h in //chrome/browser/search).

I'm exporting it to be usable for iOS implementation of "bool
IsQueryExtractionAllowedForURL(ios::ChromeBrowserState* browser_state, const
GURL& url)" and I'd prefer not to have to duplicate it.

WDYT?

[Jered, 2015-07-31 19:49:50]

On 2015/07/31 17:09:31, sdefresne wrote:
> Thank you for your review. I've tried to answer your questions. Please take
> another look.
> 
>
https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
> File components/search/search.cc (right):
> 
>
https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
> components/search/search.cc:251: #if !defined(OS_IOS) && !defined(OS_ANDROID)
> On 2015/07/31 at 15:38:45, Jered wrote:
> > This #if guard wasn't here before. did you intend to add it?
> 
> Yes, the command line flag kEnableQueryExtraction is only used in
> IsQueryExtractionEnabled() and the code using it is not compiled on Android
nor
> iOS. I can remove the #if guard if you prefer (but this mean the flag need to
be
> defined on all platforms).
> 
>
https://codereview.chromium.org/1260033003/diff/90001/components/search/search.h
> File components/search/search.h (right):
> 
>
https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
> components/search/search.h:102: bool IsSuitableURLForInstant(const GURL& url,
> const TemplateURL* template_url);
> On 2015/07/31 at 15:38:45, Jered wrote:
> > This function prototype wasn't exposed previously. Is there a caller outside
> search.cc, now?
> 
> There were three call sites in chrome/browser/search/search.cc, but two of
them
> in function that cannot be moved into the component "bool IsInstantURL(const
> GURL& url, Profile* profile)" and "bool
IsQueryExtractionAllowedForURL(Profile*
> profile, const GURL& url)" as they depends on Profile (which cannot be used in
a
> component as it is defined in src/chrome/browser).
> 
> If you do not want this to be public, I can put it in a search_internal.h file
> and add a comment saying that the content of the file should only be used by
> components/search and chrome/browser/search (maybe with a
> -components/search/search_internal.h DEPS in //chrome/browser with the
> corresponding +components/search/search_internal.h in
//chrome/browser/search).
> 
> I'm exporting it to be usable for iOS implementation of "bool
> IsQueryExtractionAllowedForURL(ios::ChromeBrowserState* browser_state, const
> GURL& url)" and I'd prefer not to have to duplicate it.
> 
> WDYT?

lgtm

[Jered, 2015-07-31 19:50:11]

(lgtm)

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
File components/search/search.cc (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.cc:251: #if !defined(OS_IOS) && !defined(OS_ANDROID)
On 2015/07/31 17:09:30, sdefresne wrote:
> On 2015/07/31 at 15:38:45, Jered wrote:
> > This #if guard wasn't here before. did you intend to add it?
> 
> Yes, the command line flag kEnableQueryExtraction is only used in
> IsQueryExtractionEnabled() and the code using it is not compiled on Android
nor
> iOS. I can remove the #if guard if you prefer (but this mean the flag need to
be
> defined on all platforms).

This way is ok.

https://codereview.chromium.org/1260033003/diff/90001/components/search/search.h
File components/search/search.h (right):

https://codereview.chromium.org/1260033003/diff/90001/components/search/searc...
components/search/search.h:102: bool IsSuitableURLForInstant(const GURL& url,
const TemplateURL* template_url);
On 2015/07/31 17:09:30, sdefresne wrote:
> On 2015/07/31 at 15:38:45, Jered wrote:
> > This function prototype wasn't exposed previously. Is there a caller outside
> search.cc, now?
> 
> There were three call sites in chrome/browser/search/search.cc, but two of
them
> in function that cannot be moved into the component "bool IsInstantURL(const
> GURL& url, Profile* profile)" and "bool
IsQueryExtractionAllowedForURL(Profile*
> profile, const GURL& url)" as they depends on Profile (which cannot be used in
a
> component as it is defined in src/chrome/browser).
> 
> If you do not want this to be public, I can put it in a search_internal.h file
> and add a comment saying that the content of the file should only be used by
> components/search and chrome/browser/search (maybe with a
> -components/search/search_internal.h DEPS in //chrome/browser with the
> corresponding +components/search/search_internal.h in
//chrome/browser/search).
> 
> I'm exporting it to be usable for iOS implementation of "bool
> IsQueryExtractionAllowedForURL(ios::ChromeBrowserState* browser_state, const
> GURL& url)" and I'd prefer not to have to duplicate it.
> 
> WDYT?

It's fine here. Thanks for explaining the rationale.

[sdefresne, 2015-08-03 09:44:38]

sdefresne@chromium.org changed reviewers:
+ benwells@chromium.org, droger@chromium.org, sky@chromium.org

[sdefresne, 2015-08-03 09:44:39]

TBR-ing OWNERS of client code impacted by refactoring
- benwells for extensions/browser/
- droger for ios/
- sky for the rest

[sdefresne, 2015-08-03 13:24:16]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2015-08-03 13:24:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, jered@chromium.org
Link to the patchset: https://codereview.chromium.org/1260033003/#ps130001
(title: "Fix compilation on iOS")

[commit-bot: I haz the power, 2015-08-03 13:24:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1260033003/130001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1260033003/130001

[commit-bot: I haz the power, 2015-08-03 14:18:22]

Committed patchset #5 (id:130001)

[commit-bot: I haz the power, 2015-08-03 14:18:58]

Patchset 5 (id:??) landed as
https://crrev.com/51bbec7be7014d4d75d39c1a5c27b1ba9ddc3dcd
Cr-Commit-Position: refs/heads/master@{#341525}