Consolidate search terms extraction and Instant process determination.

Consolidate search terms extraction and Instant process determination.

Reverts changes made to template_url.* in http://crrev.com/178755.

BUG=170390
R=dhollowa@chromium.org,pkasting@chromium.org,sky@chromium.org
TEST=none; no visible changes.


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

[sreeram, 2013-02-13 06:59:02]

dhollowa: c/b/ui/search/... and chrome_content_browser_client.cc
pkasting: c/b/search_engines/... and c/b/ui/toolbar/...
sky: c/b/instant/... and browser_instant_controller.cc

[sreeram, 2013-02-13 10:00:27]

joaodasilva: policy_browsertest.cc
sail: omnibox_view_mac_unittest.mm

[Joao da Silva, 2013-02-13 10:26:45]

policy/ lgtm

[sky, 2013-02-13 15:37:04]

LGTM

[dhollowa, 2013-02-13 17:42:28]

LGTM.

https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:192: return search_terms;
For these early returns, and anywhere you expect search_terms to be empty, it
would be clearer to use "return string16();".

[sreeram, 2013-02-13 17:45:05]

https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
File chrome/browser/ui/search/search.cc (right):

https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
chrome/browser/ui/search/search.cc:192: return search_terms;
On 2013/02/13 17:42:28, dhollowa wrote:
> For these early returns, and anywhere you expect search_terms to be empty, it
> would be clearer to use "return string16();".

The reason why I kept it this way is to allow NRVO (i.e., all return paths
return the same local object). If you think it's not worthwhile trying to get
that optimization, I can switch over to string16(). I agree string16() is
clearer. The style guide does say not to worry too much about passing strings by
value vs reference, but when there's an opportunity, why not grab it, right?

[dhollowa, 2013-02-13 17:52:46]

On 2013/02/13 17:45:05, sreeram wrote:
>
https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
> File chrome/browser/ui/search/search.cc (right):
> 
>
https://codereview.chromium.org/12250033/diff/3001/chrome/browser/ui/search/s...
> chrome/browser/ui/search/search.cc:192: return search_terms;
> On 2013/02/13 17:42:28, dhollowa wrote:
> > For these early returns, and anywhere you expect search_terms to be empty,
it
> > would be clearer to use "return string16();".
> 
> The reason why I kept it this way is to allow NRVO (i.e., all return paths
> return the same local object). If you think it's not worthwhile trying to get
> that optimization, I can switch over to string16(). I agree string16() is
> clearer. The style guide does say not to worry too much about passing strings
by
> value vs reference, but when there's an opportunity, why not grab it, right?

Understood.  My preference is for clarity in this case, as I found myself
visually searching up and down the function to determine the expected return
value.  Kinda tough on the reader.  But I'll leave it up to you.

[sail, 2013-02-13 17:54:32]

cocoa/* LGTM

[sreeram, 2013-02-13 17:57:45]

On 2013/02/13 17:52:46, dhollowa wrote:
> I found myself visually searching up and down the function

That's good enough reason not to do it. Done, changed to string16() for early
returns. Thanks for the feedback!

[sreeram, 2013-02-13 21:54:31]

@dhollowa/@sky: Please take another look at the changes introduced in patchset
#4.

The InstantService needs to exist in testing profiles as well for
ToolbarModelTest to extract search terms correctly.

The Instant tests have some fundamental issues, mainly because we end up
creating an overlay at browser startup. I'm fixing this in my upcoming refactor.
For now, I've disabled some tests, and hacked another to go green.

[Peter Kasting, 2013-02-13 22:16:27]

LGTM; the APIs in search.h look much more comprehensible to me than
IsInstantURL() was, as well!

https://codereview.chromium.org/12250033/diff/5010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/12250033/diff/5010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:51: const content::WebContents*
contents = delegate_->GetActiveWebContents();
Nit: I'd probably just inline this into the subsequent line (2 places), but up
to you

[sreeram, 2013-02-13 22:30:26]

https://codereview.chromium.org/12250033/diff/5010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/12250033/diff/5010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:51: const content::WebContents*
contents = delegate_->GetActiveWebContents();
On 2013/02/13 22:16:27, Peter Kasting wrote:
> Nit: I'd probably just inline this into the subsequent line (2 places), but up
> to you

Done, but only here. I left the other call site alone because the indentation
was making it less readable (due to the ".empty()" at the end).

[commit-bot: I haz the power, 2013-02-13 22:42:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12250033/5013

[sky, 2013-02-13 23:06:37]

Won't creating an overlay at startup effect startup timing?

  -Scott

On Wed, Feb 13, 2013 at 1:54 PM,  <sreeram@chromium.org> wrote:
> @dhollowa/@sky: Please take another look at the changes introduced in
> patchset
> #4.
>
> The InstantService needs to exist in testing profiles as well for
> ToolbarModelTest to extract search terms correctly.
>
> The Instant tests have some fundamental issues, mainly because we end up
> creating an overlay at browser startup. I'm fixing this in my upcoming
> refactor.
> For now, I've disabled some tests, and hacked another to go green.
>
> https://codereview.chromium.org/12250033/

[sreeram, 2013-02-13 23:06:39]

On Wed, Feb 13, 2013 at 2:34 PM, Scott Violet <sky@chromium.org> wrote:
> Won't creating an overlay at startup effect startup timing?

Indeed. That's why I want to fix that (CL coming soon, hopefully by tomorrow).

[commit-bot: I haz the power, 2013-02-13 23:20:54]

Retried try job too often on linux_clang for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[dhollowa, 2013-02-13 23:28:38]

lgtm

[commit-bot: I haz the power, 2013-02-13 23:58:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sreeram@chromium.org/12250033/5013

[commit-bot: I haz the power, 2013-02-14 03:39:35]

Change committed as 182381