Change the SearchBox API from using the start/end margins of the location bar to using the start ma…

Change the SearchBox API from using the start/end margins of the location bar to using the start margin and width. This change is necessary to avoid accounting for the presence of the scrollbar in the search page JavaScript.


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

[melevin, 2013-02-05 23:12:07]

Samarth & David, can you guys take a look at this CL? It's blocking the es_sm/em
CGI args change.

[samarth, 2013-02-05 23:39:33]

Sorry, missed your initial email.  This looks good from the chrome side.

Thanks,
Samarth

https://codereview.chromium.org/12047107/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/12047107/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1293: int margin =
base::i18n::IsRTL() ?
nit: const int (and below)

https://codereview.chromium.org/12047107/diff/3001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/12047107/diff/3001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:211:
this.__defineGetter__('locationBarMargin', GetLocationBarMargin);
I like that the names are more explicit throughout the chrome code, but the
names in the API should still be startMargin and width. From the point of view
of a page implementing the api, it should just be
  chrome.searchBox.startMargin
not
  chrome.searchBox.locationBarMargin

[samarth, 2013-02-05 23:42:00]

Oh also, can you look into adding a test in instant_extended_browsertest for
this? I would just make sure that toggling the home button causes the saved
value in InstantController to change.

(Fine to do it in a follow-up CL since it was missing tests anyway.)

Thanks,
Samarth

[melevin, 2013-02-06 23:43:56]

On 2013/02/05 23:42:00, samarth wrote:
> Oh also, can you look into adding a test in instant_extended_browsertest for
> this? I would just make sure that toggling the home button causes the saved
> value in InstantController to change.

Any tips on how to toggle the home button from within a test?

> (Fine to do it in a follow-up CL since it was missing tests anyway.)
> 
> Thanks,
> Samarth

[melevin, 2013-02-06 23:44:04]

https://codereview.chromium.org/12047107/diff/3001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/12047107/diff/3001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1293: int margin =
base::i18n::IsRTL() ?
On 2013/02/05 23:39:33, samarth wrote:
> nit: const int (and below)

Done.

https://codereview.chromium.org/12047107/diff/3001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/searchbox_api.js (right):

https://codereview.chromium.org/12047107/diff/3001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/searchbox_api.js:211:
this.__defineGetter__('locationBarMargin', GetLocationBarMargin);
On 2013/02/05 23:39:33, samarth wrote:
> I like that the names are more explicit throughout the chrome code, but the
> names in the API should still be startMargin and width. From the point of view
> of a page implementing the api, it should just be
>   chrome.searchBox.startMargin
> not
>   chrome.searchBox.locationBarMargin

Done.

[samarth, 2013-02-06 23:48:24]

On 2013/02/06 23:43:56, melevin wrote:
> On 2013/02/05 23:42:00, samarth wrote:
> > Oh also, can you look into adding a test in instant_extended_browsertest for
> > this? I would just make sure that toggling the home button causes the saved
> > value in InstantController to change.
> 
> Any tips on how to toggle the home button from within a test?

There's a pref called kShowHomeButton. I would try toggling it.
(See InstantTestBase::SetupInstantUsingTemplateURL for an example of changing a
pref from a test)

> 
> > (Fine to do it in a follow-up CL since it was missing tests anyway.)
> > 
> > Thanks,
> > Samarth

[melevin, 2013-02-07 00:14:31]

On 2013/02/06 23:48:24, samarth wrote:
> On 2013/02/06 23:43:56, melevin wrote:
> > On 2013/02/05 23:42:00, samarth wrote:
> > > Oh also, can you look into adding a test in instant_extended_browsertest
for
> > > this? I would just make sure that toggling the home button causes the
saved
> > > value in InstantController to change.
> > 
> > Any tips on how to toggle the home button from within a test?
> 
> There's a pref called kShowHomeButton. I would try toggling it.
> (See InstantTestBase::SetupInstantUsingTemplateURL for an example of changing
a
> pref from a test)

Yup, I also found some Objective C code that exercised it. That was easier than
I expected. I added the test to this CL.

> > 
> > > (Fine to do it in a follow-up CL since it was missing tests anyway.)
> > > 
> > > Thanks,
> > > Samarth

[samarth, 2013-02-07 00:28:37]

Thanks for adding a test. lgtm for the instant code. +sreeram for instant
OWNERS.

https://codereview.chromium.org/12047107/diff/11001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/12047107/diff/11001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:112: EXPECT_NE(width,
newWidth);
Is it always the case that the deltas between the old/new margins and the
old/new widths will equal each other? If so, let's also test for that.

https://codereview.chromium.org/12047107/diff/11001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/12047107/diff/11001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:57: // Returns the extended Instant
start-edge margin to use in screen pixels,
In extended Instant, returns the start-ege margin of the location bar in screen
pixels.

[melevin, 2013-02-07 21:21:22]

https://codereview.chromium.org/12047107/diff/11001/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/12047107/diff/11001/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:112: EXPECT_NE(width,
newWidth);
On 2013/02/07 00:28:37, samarth wrote:
> Is it always the case that the deltas between the old/new margins and the
> old/new widths will equal each other? If so, let's also test for that.

Ah good point, that should be true.

https://codereview.chromium.org/12047107/diff/11001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/12047107/diff/11001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:57: // Returns the extended Instant
start-edge margin to use in screen pixels,
On 2013/02/07 00:28:37, samarth wrote:
> In extended Instant, returns the start-ege margin of the location bar in
screen
> pixels.

Done.

[sreeram, 2013-02-14 18:26:56]

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:102: // Sets the stored start-edge
margin and width.
// Sets the start-edge margin and width of the omnibox.

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:358: // The start-edge margin and
width to use for aligning the preview contents
// The start-edge margin and width of the omnibox, used by the page to align its
suggestions with the omnibox.

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:361: int width_;
These variables are too generic. Name them "omnibox_start_margin_" and
"omnibox_width_".

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:354: int startMargin;
start_margin

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:368: int newWidth;
new_start_margin
new_width

https://codereview.chromium.org/12047107/diff/10003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:112: int width_;
Why do we need this? It's the same as the popup_bounds_.width.

[melevin, 2013-02-19 22:00:49]

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:102: // Sets the stored start-edge
margin and width.
On 2013/02/14 18:26:57, sreeram wrote:
> // Sets the start-edge margin and width of the omnibox.

I added "stored" because wanted to make it clear that this function won't
actually resize the omnibox.

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:358: // The start-edge margin and
width to use for aligning the preview contents
On 2013/02/14 18:26:57, sreeram wrote:
> // The start-edge margin and width of the omnibox, used by the page to align
its
> suggestions with the omnibox.

Done.

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:361: int width_;
On 2013/02/14 18:26:57, sreeram wrote:
> These variables are too generic. Name them "omnibox_start_margin_" and
> "omnibox_width_".

Done.

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:354: int startMargin;
On 2013/02/14 18:26:57, sreeram wrote:
> start_margin

Oops, went into JavaScript mode...

https://codereview.chromium.org/12047107/diff/10003/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:368: int newWidth;
On 2013/02/14 18:26:57, sreeram wrote:
> new_start_margin
> new_width

Done.

https://codereview.chromium.org/12047107/diff/10003/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.h (right):

https://codereview.chromium.org/12047107/diff/10003/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:112: int width_;
On 2013/02/14 18:26:57, sreeram wrote:
> Why do we need this? It's the same as the popup_bounds_.width.

Yeah I suppose we can reuse that.

[sreeram, 2013-02-20 15:40:10]

lgtm

https://codereview.chromium.org/12047107/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12047107/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:358: // The start-edge margin and
width of the omnibox, used by the page align its
page align -> page to align

https://codereview.chromium.org/12047107/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/12047107/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:228: // In extended Instant,
this gets the width of the location bar instead.
These are exactly the same. I.e., the width of the popup (dropdown) is equal to
the width of the omnibox textbox. In old Instant, the width isn't used anyway.
Old Instant, AFAIK, only uses the height part of the popup bounds (to move its
results down by that much). So, I'd just say something like:
// Gets the width of the region of the search box that overlaps the window,
// i.e., the width of the omnibox.

[melevin, 2013-02-20 20:11:45]

https://codereview.chromium.org/12047107/diff/16001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/12047107/diff/16001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:358: // The start-edge margin and
width of the omnibox, used by the page align its
On 2013/02/20 15:40:10, sreeram wrote:
> page align -> page to align

Done.

https://codereview.chromium.org/12047107/diff/16001/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/12047107/diff/16001/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox_extension.cc:228: // In extended Instant,
this gets the width of the location bar instead.
On 2013/02/20 15:40:10, sreeram wrote:
> These are exactly the same. I.e., the width of the popup (dropdown) is equal
to
> the width of the omnibox textbox. In old Instant, the width isn't used anyway.
> Old Instant, AFAIK, only uses the height part of the popup bounds (to move its
> results down by that much). So, I'd just say something like:
> // Gets the width of the region of the search box that overlaps the window,
> // i.e., the width of the omnibox.

Done.

[commit-bot: I haz the power, 2013-02-20 20:12:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/melevin@chromium.org/12047107/22001

[commit-bot: I haz the power, 2013-02-20 20:12:21]

Presubmit check for 12047107-22001 failed and returned exit status 1.

INFO:root:Found 12 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/ui/views/PRESUBMIT.py

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui
    chrome/browser/ui/views

Presubmit checks took 1.8s to calculate.

[melevin, 2013-02-20 21:29:49]

Adding pkasting for chrome/browser/ui changes. PTAL when you have a minute,
thanks!

[Peter Kasting, 2013-02-26 22:07:19]

LGTM with one change.

https://codereview.chromium.org/12047107/diff/22001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/12047107/diff/22001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1258:
parent_bounds.right() - bounds.right() : bounds.x() - parent_bounds.x();
Accessing the parent bounds at all, let alone using GetBoundsInScreen(), is
inefficient.

bounds() returns your bounds in parent coordinates.  If that doesn't do the
right thing for the RTL case, look at GetMirroredX() or GetMirroredBounds()
(probably the former is all you need).  Hopefully with those you can avoid
needing to explicitly calculate things.

[melevin, 2013-02-26 23:43:52]

https://codereview.chromium.org/12047107/diff/22001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/12047107/diff/22001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1258:
parent_bounds.right() - bounds.right() : bounds.x() - parent_bounds.x();
On 2013/02/26 22:07:19, Peter Kasting wrote:
> Accessing the parent bounds at all, let alone using GetBoundsInScreen(), is
> inefficient.
> 
> bounds() returns your bounds in parent coordinates.  If that doesn't do the
> right thing for the RTL case, look at GetMirroredX() or GetMirroredBounds()
> (probably the former is all you need).  Hopefully with those you can avoid
> needing to explicitly calculate things.

Ah you're right, this code simplified down to just using the bounds.

[Peter Kasting, 2013-02-26 23:46:26]

https://codereview.chromium.org/12047107/diff/24002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/12047107/diff/24002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1282: bounds_rect.x(),
bounds_rect.width());
Nit: Rather than using a margin and width pair here and inside the instant
controller everywhere, you might want to consider using a Rect (and simply
ignoring the Y-related components).  That might ultimately simplify your code a
bit.

[commit-bot: I haz the power, 2013-02-26 23:47:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/melevin@chromium.org/12047107/24002

[commit-bot: I haz the power, 2013-02-27 17:05:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/melevin@chromium.org/12047107/32003

[commit-bot: I haz the power, 2013-02-27 17:05:04]

Failed to apply patch for chrome/renderer/resources/extensions/searchbox_api.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/renderer/resources/extensions/searchbox_api.js
  Hunk #1 FAILED at 43.
  Hunk #2 FAILED at 235.
  2 out of 2 hunks FAILED -- saving rejects to file
chrome/renderer/resources/extensions/searchbox_api.js.rej

Patch:       chrome/renderer/resources/extensions/searchbox_api.js
Index: chrome/renderer/resources/extensions/searchbox_api.js
diff --git a/chrome/renderer/resources/extensions/searchbox_api.js
b/chrome/renderer/resources/extensions/searchbox_api.js
index
48616b382abcaed410a3210b85697ddfd07d4302..43caa68b419c1809c1cf85e912a166f393b97ea8
100644
--- a/chrome/renderer/resources/extensions/searchbox_api.js
+++ b/chrome/renderer/resources/extensions/searchbox_api.js
@@ -43,7 +43,6 @@ if (!chrome.embeddedSearch) {
     native function GetSelectionStart();
     native function GetSelectionEnd();
     native function GetStartMargin();
-    native function GetEndMargin();
     native function GetRightToLeft();
     native function GetAutocompleteResults();
     native function GetDisplayInstantResults();
@@ -235,7 +234,6 @@ if (!chrome.embeddedSearch) {
     this.__defineGetter__('selectionStart', GetSelectionStart);
     this.__defineGetter__('selectionEnd', GetSelectionEnd);
     this.__defineGetter__('startMargin', GetStartMargin);
-    this.__defineGetter__('endMargin', GetEndMargin);
     this.__defineGetter__('rtl', GetRightToLeft);
     this.__defineGetter__('nativeSuggestions', GetAutocompleteResultsWrapper);
     this.__defineGetter__('isKeyCaptureEnabled', IsKeyCaptureEnabled);

[commit-bot: I haz the power, 2013-02-27 18:04:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/melevin@chromium.org/12047107/32004

[commit-bot: I haz the power, 2013-02-27 19:13:22]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-02-28 00:21:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/melevin@chromium.org/12047107/37001

[commit-bot: I haz the power, 2013-02-28 02:25:10]

Change committed as 185115