Have SearchProvider look up the omnibox start margin directly.

Have SearchProvider look up the omnibox start margin directly.
BUG=261834
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216812

[jeremycho, 2013-07-31 19:48:24]

Hi Samarth - can you take a look before I send this out for OWNERs review? 
Thanks!

[jeremycho, 2013-08-02 20:10:29]

friendly ping

[samarth, 2013-08-02 22:46:57]

FYI I'd prefer to take all of this out (c.f. the other thread about inline JS).
I'll take a look once we sort that out.

Thanks,
Samarth

[samarth, 2013-08-06 18:09:32]

Ok, we should fix crbug.com/247517 but in the meantime, this does improve
things.  Can you rebase your change?

Thanks,
Samarth

https://codereview.chromium.org/21042013/diff/5001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/21042013/diff/5001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.h:63: gfx::Rect GetOmniboxBounds();
Make this a simple accessor: omnibox_bounds()

[jeremycho, 2013-08-06 21:03:04]

Rebased.

https://codereview.chromium.org/21042013/diff/5001/chrome/browser/ui/search/i...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/21042013/diff/5001/chrome/browser/ui/search/i...
chrome/browser/ui/search/instant_controller.h:63: gfx::Rect GetOmniboxBounds();
On 2013/08/06 18:09:32, samarth wrote:
> Make this a simple accessor: omnibox_bounds()

Done.

[samarth, 2013-08-07 16:19:06]

lgtm for search

+pkasting for OWNERS. Peter, see my comment above and the updates in
crbug.com/247517. I'm all on board for getting rid of the margin query param.
Until the server supports it, we should have it work correctly though.

Thanks,
Samarth

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: #if !defined(OS_ANDROID)
nit: don't indent this line or the #endif.

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: #if !defined(OS_ANDROID)
Do you need this because browser finder doesn't exist on Android? FWIW, we don't
do any of this margin shenanigans on IOS either so you may want to exclude that
as well. Please also add a comment here.

[jeremycho, 2013-08-07 22:16:04]

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: #if !defined(OS_ANDROID)
On 2013/08/07 16:19:07, samarth wrote:
> nit: don't indent this line or the #endif.

Done.

https://codereview.chromium.org/21042013/diff/17001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: #if !defined(OS_ANDROID)
On 2013/08/07 16:19:07, samarth wrote:
> Do you need this because browser finder doesn't exist on Android? FWIW, we
don't
> do any of this margin shenanigans on IOS either so you may want to exclude
that
> as well. Please also add a comment here.

Yeah, done.

[jeremycho, 2013-08-09 16:43:48]

ping pkasting

[Peter Kasting, 2013-08-09 21:03:46]

LGTM

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: // browser_finder does not
exist for Android. On IOS, start margin is not used.
Nit: To avoid unusual comment indenting, how about:

  // On non-mobile, ask the instant controller for the appropriate start margin.
  // On Android, FindBrowserWithProfile() doesn't exist, and on iOS, the start
  // margin is unused, so leave the value as default there.
  int omnibox_start_margin = chrome::kDisableStartMargin;
#if ...

You can simplify this comment if we don't use the start margin on Android
anyway, by just saying that this is unused on mobile.

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:101: gfx::Rect omnibox_bounds() {
Nit: Function const

[jeremycho, 2013-08-09 23:55:09]

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/autocomple...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/autocomple...
chrome/browser/autocomplete/search_provider.cc:1385: // browser_finder does not
exist for Android. On IOS, start margin is not used.
On 2013/08/09 21:03:46, Peter Kasting wrote:
> Nit: To avoid unusual comment indenting, how about:
> 
>   // On non-mobile, ask the instant controller for the appropriate start
margin.
>   // On Android, FindBrowserWithProfile() doesn't exist, and on iOS, the start
>   // margin is unused, so leave the value as default there.
>   int omnibox_start_margin = chrome::kDisableStartMargin;
> #if ...
> 
> You can simplify this comment if we don't use the start margin on Android
> anyway, by just saying that this is unused on mobile.

Done.

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/21042013/diff/22001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.h:101: gfx::Rect omnibox_bounds() {
On 2013/08/09 21:03:46, Peter Kasting wrote:
> Nit: Function const

Done.

[commit-bot: I haz the power, 2013-08-09 23:57:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/21042013/35001

[commit-bot: I haz the power, 2013-08-10 00:28:08]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2013-08-10 01:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremycho@chromium.org/21042013/35001

[commit-bot: I haz the power, 2013-08-10 07:06:43]

Change committed as 216812

[pkl (ping after 24h if needed), 2013-08-14 13:25:15]

jeremycho: It would be great if you can fix the IOS -> OS_IOS in an upstream CL.
Otherwise, I'll fix it later.

Peter.

https://chromiumcodereview.appspot.com/21042013/diff/35001/chrome/browser/aut...
File chrome/browser/autocomplete/search_provider.cc (right):

https://chromiumcodereview.appspot.com/21042013/diff/35001/chrome/browser/aut...
chrome/browser/autocomplete/search_provider.cc:1387: #if !defined(OS_ANDROID) &&
!defined(IOS)
iOS should be !defined(OS_IOS)
Unfortunately, this mistake often happens.