Stop depending on InstantService from BaseSearchProvider

Stop depending on InstantService from BaseSearchProvider

UIThreadSearchTermsData is responsible to get the value instead.
Remove |start_margin| argument from GetInstantURL() as all users are passing kDisableStartMargin for it.
Add a new test TemplateURLTest.OmniboxStartmargin

BUG=388515
TEST=components_unittests

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

[Peter Kasting, 2014-07-15 19:19:05]

LGTM

[hashimoto, 2014-07-16 02:43:39]

jered@, could you review this change as an owner of chrome/browser/search?

[Jered, 2014-07-16 17:21:14]

I'm worried about this CL. omnibox_start_margin_ is used to align the left
column of search results with the left of the omnibox text in any configuration
where the omnibox acts as the searchbox---I think this is true on clank.

Kausalya knows more about the current state of this.

[hashimoto, 2014-07-17 04:41:54]

On 2014/07/16 17:21:14, Jered wrote:
> I'm worried about this CL. omnibox_start_margin_ is used to align the left
> column of search results with the left of the omnibox text in any
configuration
> where the omnibox acts as the searchbox---I think this is true on clank.
> 
> Kausalya knows more about the current state of this.

This change is not removing the omnibox start margin related code.
Just moving the responsibility to get the value from autocomplete to
search_engines.

The argument is dropped from GetInstantURL() as it seems no user is using it.
(as far as I can see, clank is not an exception.)

[kmadhusu, 2014-07-17 16:59:26]

On 2014/07/17 04:41:54, hashimoto wrote:
> On 2014/07/16 17:21:14, Jered wrote:
> > I'm worried about this CL. omnibox_start_margin_ is used to align the left
> > column of search results with the left of the omnibox text in any
> configuration
> > where the omnibox acts as the searchbox---I think this is true on clank.
> > 
> > Kausalya knows more about the current state of this.
> 
> This change is not removing the omnibox start margin related code.
> Just moving the responsibility to get the value from autocomplete to
> search_engines.
> 
> The argument is dropped from GetInstantURL() as it seems no user is using it.
> (as far as I can see, clank is not an exception.)

As Jered said, omnibox_start_margin is currently used.

Use case:
1. Type "pandora" in the omnibox and press enter.
2. Observe the "es_sm" cgi param value in the search url. You should see the
default value 93.
3. Go to chrome://settings and select "Show Home button" in the Appearance
section.
4. Open a new tab and type "paypal" in the omnibox and press enter.
5. Observe the "es_sm" cgi param value in the search url. You should see a
non-default value (such as 122). 

While I was testing your changes, I noticed that the value is not set properly
in incognito window '/search' requests. es_sm is sometimes set to 0. Can you
make sure this is not a regression caused by this new change?

Thanks.

[hashimoto, 2014-07-18 04:42:59]

On 2014/07/17 16:59:26, kmadhusu wrote:
> On 2014/07/17 04:41:54, hashimoto wrote:
> > On 2014/07/16 17:21:14, Jered wrote:
> > > I'm worried about this CL. omnibox_start_margin_ is used to align the left
> > > column of search results with the left of the omnibox text in any
> > configuration
> > > where the omnibox acts as the searchbox---I think this is true on clank.
> > > 
> > > Kausalya knows more about the current state of this.
> > 
> > This change is not removing the omnibox start margin related code.
> > Just moving the responsibility to get the value from autocomplete to
> > search_engines.
> > 
> > The argument is dropped from GetInstantURL() as it seems no user is using
it.
> > (as far as I can see, clank is not an exception.)
> 
> As Jered said, omnibox_start_margin is currently used.
This change is not removing the omnibox start margin related code.
What is removed in this change is the unused argument of GetInstantURL().
TemplateURL is still able to access the omnibox start margin value via
UIThreadSearchTermsData. 
> 
> Use case:
> 1. Type "pandora" in the omnibox and press enter.
> 2. Observe the "es_sm" cgi param value in the search url. You should see the
> default value 93.
> 3. Go to chrome://settings and select "Show Home button" in the Appearance
> section.
> 4. Open a new tab and type "paypal" in the omnibox and press enter.
> 5. Observe the "es_sm" cgi param value in the search url. You should see a
> non-default value (such as 122). 
> 
> While I was testing your changes, I noticed that the value is not set properly
> in incognito window '/search' requests. es_sm is sometimes set to 0. Can you
> make sure this is not a regression caused by this new change?
es_sm can be 0 as template_url.cc is adding the es_sm parameter when
"search_terms_args.omnibox_start_margin >= 0".
When I tried with an incognito window, es_sm is always 93 (or 122 when the home
button is shown).
In which process did you see es_sm=0?
> 
> Thanks.

[kmadhusu, 2014-07-21 16:40:29]

On 2014/07/18 04:42:59, hashimoto wrote:
> On 2014/07/17 16:59:26, kmadhusu wrote:
> > On 2014/07/17 04:41:54, hashimoto wrote:
> > > On 2014/07/16 17:21:14, Jered wrote:
> > > > I'm worried about this CL. omnibox_start_margin_ is used to align the
left
> > > > column of search results with the left of the omnibox text in any
> > > configuration
> > > > where the omnibox acts as the searchbox---I think this is true on clank.
> > > > 
> > > > Kausalya knows more about the current state of this.
> > > 
> > > This change is not removing the omnibox start margin related code.
> > > Just moving the responsibility to get the value from autocomplete to
> > > search_engines.
> > > 
> > > The argument is dropped from GetInstantURL() as it seems no user is using
> it.
> > > (as far as I can see, clank is not an exception.)
> > 
> > As Jered said, omnibox_start_margin is currently used.
> This change is not removing the omnibox start margin related code.
> What is removed in this change is the unused argument of GetInstantURL().
> TemplateURL is still able to access the omnibox start margin value via
> UIThreadSearchTermsData. 
> > 
> > Use case:
> > 1. Type "pandora" in the omnibox and press enter.
> > 2. Observe the "es_sm" cgi param value in the search url. You should see the
> > default value 93.
> > 3. Go to chrome://settings and select "Show Home button" in the Appearance
> > section.
> > 4. Open a new tab and type "paypal" in the omnibox and press enter.
> > 5. Observe the "es_sm" cgi param value in the search url. You should see a
> > non-default value (such as 122). 
> > 
> > While I was testing your changes, I noticed that the value is not set
properly
> > in incognito window '/search' requests. es_sm is sometimes set to 0. Can you
> > make sure this is not a regression caused by this new change?
> es_sm can be 0 as template_url.cc is adding the es_sm parameter when
> "search_terms_args.omnibox_start_margin >= 0".
> When I tried with an incognito window, es_sm is always 93 (or 122 when the
home
> button is shown).
> In which process did you see es_sm=0?

I ran the above said steps in the Incognito window. I cannot repro this issue
consistently. 

> > 
> > Thanks.

[hashimoto, 2014-07-22 01:34:20]

On 2014/07/21 16:40:29, kmadhusu wrote:
> On 2014/07/18 04:42:59, hashimoto wrote:
> > On 2014/07/17 16:59:26, kmadhusu wrote:
> > > On 2014/07/17 04:41:54, hashimoto wrote:
> > > > On 2014/07/16 17:21:14, Jered wrote:
> > > > > I'm worried about this CL. omnibox_start_margin_ is used to align the
> left
> > > > > column of search results with the left of the omnibox text in any
> > > > configuration
> > > > > where the omnibox acts as the searchbox---I think this is true on
clank.
> > > > > 
> > > > > Kausalya knows more about the current state of this.
> > > > 
> > > > This change is not removing the omnibox start margin related code.
> > > > Just moving the responsibility to get the value from autocomplete to
> > > > search_engines.
> > > > 
> > > > The argument is dropped from GetInstantURL() as it seems no user is
using
> > it.
> > > > (as far as I can see, clank is not an exception.)
> > > 
> > > As Jered said, omnibox_start_margin is currently used.
> > This change is not removing the omnibox start margin related code.
> > What is removed in this change is the unused argument of GetInstantURL().
> > TemplateURL is still able to access the omnibox start margin value via
> > UIThreadSearchTermsData. 
> > > 
> > > Use case:
> > > 1. Type "pandora" in the omnibox and press enter.
> > > 2. Observe the "es_sm" cgi param value in the search url. You should see
the
> > > default value 93.
> > > 3. Go to chrome://settings and select "Show Home button" in the Appearance
> > > section.
> > > 4. Open a new tab and type "paypal" in the omnibox and press enter.
> > > 5. Observe the "es_sm" cgi param value in the search url. You should see a
> > > non-default value (such as 122). 
> > > 
> > > While I was testing your changes, I noticed that the value is not set
> properly
> > > in incognito window '/search' requests. es_sm is sometimes set to 0. Can
you
> > > make sure this is not a regression caused by this new change?
> > es_sm can be 0 as template_url.cc is adding the es_sm parameter when
> > "search_terms_args.omnibox_start_margin >= 0".
> > When I tried with an incognito window, es_sm is always 93 (or 122 when the
> home
> > button is shown).
> > In which process did you see es_sm=0?
> 
> I ran the above said steps in the Incognito window. I cannot repro this issue
> consistently. 
You mean you launched out/Debug/chrome, press Ctrl+Shift+N to open a incognito
window, and type some search query+Enter?
On which platform did you see the problem and what was the GYP_DEFINES value?
I cannot reproduce this on Linux, with a binary built with
GYP_DEFINES="component=shared_library".
How frequent do you see this?
Are you sure you don't observe the same problem without this patch?
> 
> > > 
> > > Thanks.

[kmadhusu, 2014-07-22 17:55:05]

On 2014/07/22 01:34:20, hashimoto wrote:
> On 2014/07/21 16:40:29, kmadhusu wrote:
> > On 2014/07/18 04:42:59, hashimoto wrote:
> > > On 2014/07/17 16:59:26, kmadhusu wrote:
> > > > On 2014/07/17 04:41:54, hashimoto wrote:
> > > > > On 2014/07/16 17:21:14, Jered wrote:
> > > > > > I'm worried about this CL. omnibox_start_margin_ is used to align
the
> > left
> > > > > > column of search results with the left of the omnibox text in any
> > > > > configuration
> > > > > > where the omnibox acts as the searchbox---I think this is true on
> clank.
> > > > > > 
> > > > > > Kausalya knows more about the current state of this.
> > > > > 
> > > > > This change is not removing the omnibox start margin related code.
> > > > > Just moving the responsibility to get the value from autocomplete to
> > > > > search_engines.
> > > > > 
> > > > > The argument is dropped from GetInstantURL() as it seems no user is
> using
> > > it.
> > > > > (as far as I can see, clank is not an exception.)
> > > > 
> > > > As Jered said, omnibox_start_margin is currently used.
> > > This change is not removing the omnibox start margin related code.
> > > What is removed in this change is the unused argument of GetInstantURL().
> > > TemplateURL is still able to access the omnibox start margin value via
> > > UIThreadSearchTermsData. 
> > > > 
> > > > Use case:
> > > > 1. Type "pandora" in the omnibox and press enter.
> > > > 2. Observe the "es_sm" cgi param value in the search url. You should see
> the
> > > > default value 93.
> > > > 3. Go to chrome://settings and select "Show Home button" in the
Appearance
> > > > section.
> > > > 4. Open a new tab and type "paypal" in the omnibox and press enter.
> > > > 5. Observe the "es_sm" cgi param value in the search url. You should see
a
> > > > non-default value (such as 122). 
> > > > 
> > > > While I was testing your changes, I noticed that the value is not set
> > properly
> > > > in incognito window '/search' requests. es_sm is sometimes set to 0. Can
> you
> > > > make sure this is not a regression caused by this new change?
> > > es_sm can be 0 as template_url.cc is adding the es_sm parameter when
> > > "search_terms_args.omnibox_start_margin >= 0".
> > > When I tried with an incognito window, es_sm is always 93 (or 122 when the
> > home
> > > button is shown).
> > > In which process did you see es_sm=0?
> > 
> > I ran the above said steps in the Incognito window. I cannot repro this
issue
> > consistently. 
> You mean you launched out/Debug/chrome, press Ctrl+Shift+N to open a incognito
> window, and type some search query+Enter?
> On which platform did you see the problem and what was the GYP_DEFINES value?
> I cannot reproduce this on Linux, with a binary built with
> GYP_DEFINES="component=shared_library".
> How frequent do you see this?
> Are you sure you don't observe the same problem without this patch?
> > 
> > > > 
> > > > Thanks.

Okay. I am unable to repro the issue. I will keep an eye. If I see the issue
again, I will file a bug.
OS: Linux
GYP_DEFINES value: "target_arch=x64 component=shared_library disable_nacl=1
remove_webcore_debug_symbols=1 enable_svg=0"

LGTM. Thanks.

[hashimoto, 2014-07-23 03:07:03]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2014-07-23 03:08:41]

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

[commit-bot: I haz the power, 2014-07-23 07:04:49]

Change committed as 284878