Implement the Instant extended API startMargin, endMargin, and rtl properties and the onmarginchang…

Implement the Instant extended API startMargin, endMargin, and rtl properties and the onmarginchange event for Views only.


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

[dhollowa, 2012-11-14 17:20:46]

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:95: void SetPopupBounds(const
gfx::Rect& bounds);
Are both the popup bounds and omnibox bounds still in use?  Or could you remove
the omnibox bounds?

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:259: // Current omnibox popup
bounds.
Would it be possible to eliminate this "cache" of bounds?  The same bounds are
held on the renderer side, as well as in the OmniboxView class, so having a
third copy here seems like a potential source of error.

https://codereview.chromium.org/11359198/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1055:
OmniboxBoundsChangedTo(view_->GetBounds());
The OmniboxView class holds a pointer to an OmniboxEditModel, so why not just
call this directly from OmniboxViewViews::OnBoundsChanged()?  I'd rename it to
OmniboxEditModel::OnOmniboxBoundsChanged() though.

[samarth, 2012-11-14 19:02:04]

As we discussed offline yesterday, for the margin, we may really care about
where the query/URL begins in the omnibox, not the omnibox itself.  Does your
patch handle this?  (Alternatively, if the distance between the omnibox edge and
the text is stable per-platform, we won't need this.)

Samarth

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:95: void SetPopupBounds(const
gfx::Rect& bounds);
On 2012/11/14 17:20:46, dhollowa wrote:
> Are both the popup bounds and omnibox bounds still in use?  Or could you
remove
> the omnibox bounds?

I think we want omnibox bounds and not popup bounds since the popup doesn't
exist anymore.

[melevin, 2012-11-14 22:16:26]

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:95: void SetPopupBounds(const
gfx::Rect& bounds);
On 2012/11/14 19:02:04, samarth wrote:
> On 2012/11/14 17:20:46, dhollowa wrote:
> > Are both the popup bounds and omnibox bounds still in use?  Or could you
> remove
> > the omnibox bounds?
> 
> I think we want omnibox bounds and not popup bounds since the popup doesn't
> exist anymore.

We might want to do this in a separate change. The popup bounds are currently
used to provide the Searchbox API x, y, width, and height properties. Although
they are isolated as unused in the new API, they are still a part of it.

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:259: // Current omnibox popup
bounds.
On 2012/11/14 17:20:46, dhollowa wrote:
> Would it be possible to eliminate this "cache" of bounds?  The same bounds are
> held on the renderer side, as well as in the OmniboxView class, so having a
> third copy here seems like a potential source of error.

I don't think so.

We need at least one additional set of bounds here to be able to tell when the
bounds changed, so we need at least "last_popup_bounds_". But it also turns out
that "popup_bounds_" is used to pass arguments to the SendBoundsToPage function.
The reason we can't pass arguments directly to SendBoundsToPage is due to the
way base::OneShotTimer works. It has to take a argument-less member function as
an argument.

https://codereview.chromium.org/11359198/diff/1/chrome/browser/ui/omnibox/omn...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/ui/omnibox/omn...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1055:
OmniboxBoundsChangedTo(view_->GetBounds());
On 2012/11/14 17:20:46, dhollowa wrote:
> The OmniboxView class holds a pointer to an OmniboxEditModel, so why not just
> call this directly from OmniboxViewViews::OnBoundsChanged()?  I'd rename it to
> OmniboxEditModel::OnOmniboxBoundsChanged() though.

Thanks, this is much cleaner.

[dhollowa, 2012-11-14 22:53:55]

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:95: void SetPopupBounds(const
gfx::Rect& bounds);
On 2012/11/14 22:16:27, melevin wrote:
> On 2012/11/14 19:02:04, samarth wrote:
> > On 2012/11/14 17:20:46, dhollowa wrote:
> > > Are both the popup bounds and omnibox bounds still in use?  Or could you
> > remove
> > > the omnibox bounds?
> > 
> > I think we want omnibox bounds and not popup bounds since the popup doesn't
> > exist anymore.
> 
> We might want to do this in a separate change. The popup bounds are currently
> used to provide the Searchbox API x, y, width, and height properties. Although
> they are isolated as unused in the new API, they are still a part of it.

We shouldn't really be setting/sending full "bounds" if we only need left/right
"margin" information.

https://codereview.chromium.org/11359198/diff/4001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11359198/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:280: void
OnOmniboxBoundsChanged(const gfx::Rect& bounds);
Oh, now there's a mismatch with |PopupBoundsChangedTo| naming.  Let's just
rename that one as well.  Also, the comments above both of these are wrong - "is
going to change" -> "did change".  i.e. The tense in the comments should match
the tense of the method names.

[melevin, 2012-11-15 23:40:01]

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/1/chrome/browser/instant/instan...
chrome/browser/instant/instant_controller.h:95: void SetPopupBounds(const
gfx::Rect& bounds);
On 2012/11/14 22:53:56, dhollowa wrote:
> On 2012/11/14 22:16:27, melevin wrote:
> > On 2012/11/14 19:02:04, samarth wrote:
> > > On 2012/11/14 17:20:46, dhollowa wrote:
> > > > Are both the popup bounds and omnibox bounds still in use?  Or could you
> > > remove
> > > > the omnibox bounds?
> > > 
> > > I think we want omnibox bounds and not popup bounds since the popup
doesn't
> > > exist anymore.
> > 
> > We might want to do this in a separate change. The popup bounds are
currently
> > used to provide the Searchbox API x, y, width, and height properties.
Although
> > they are isolated as unused in the new API, they are still a part of it.
> 
> We shouldn't really be setting/sending full "bounds" if we only need
left/right
> "margin" information.

I've split the bounds into separate start and end ints.

https://codereview.chromium.org/11359198/diff/4001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11359198/diff/4001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.h:280: void
OnOmniboxBoundsChanged(const gfx::Rect& bounds);
On 2012/11/14 22:53:56, dhollowa wrote:
> Oh, now there's a mismatch with |PopupBoundsChangedTo| naming.  Let's just
> rename that one as well.  Also, the comments above both of these are wrong -
"is
> going to change" -> "did change".  i.e. The tense in the comments should match
> the tense of the method names.

Done.

[dhollowa, 2012-11-15 23:55:14]

https://codereview.chromium.org/11359198/diff/9001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:99: void SetMarginSize(int start,
int end);
My understanding is that "start" means left-margin-size and "end" means
right-margin-size when in LTR mode, and vice-versa when in RTL mode.

[melevin, 2012-11-16 00:12:42]

https://codereview.chromium.org/11359198/diff/9001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/9001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:99: void SetMarginSize(int start,
int end);
On 2012/11/15 23:55:14, dhollowa wrote:
> My understanding is that "start" means left-margin-size and "end" means
> right-margin-size when in LTR mode, and vice-versa when in RTL mode.

Ah, I thought the coordinates swapped automatically. I should have tested more
thoroughly, they don't. Changed the code to swap manually in OmniboxEditModel.

[dhollowa, 2012-11-16 17:29:02]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
A couple questions.  (1) Isn't this supposed to be the other way round?  (2) To
compute |end|, which is a width, I'd assume you'd need the containing bounds to
determine it correctly.

This naming is the source of the confusion I think.  We should change the
implementation and the spec to be something like |SetMarginSize(first_margin,
last_margin)| or something.

[samarth, 2012-11-16 22:08:26]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:45: // An artificial delay (in
milliseconds) we introduce before telling the Instant
I suspect this was necessary because the height of the popup is fairly volatile.
I hope we don't need this for the omnibox bounds, but something to keep in mind.

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:344: // Clear the margins so they
are set when the preview is shown again.
Why is this necessary?

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:266: // Offset from the start edge
of the page to the start of the page text area.
Page text area is a little confusing in this context. Do you mean "omnibox text
area"?

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
On 2012/11/16 17:29:03, dhollowa wrote:
> A couple questions.  (1) Isn't this supposed to be the other way round?  (2)
To
> compute |end|, which is a width, I'd assume you'd need the containing bounds
to
> determine it correctly.
> 
> This naming is the source of the confusion I think.  We should change the
> implementation and the spec to be something like |SetMarginSize(first_margin,
> last_margin)| or something.

I'm also confused why this isn't the other way around.

Re (2), David: are the bounds reported by GetBoundsInScreen() not absolute?

http://codereview.chromium.org/11359198/diff/13001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/common/render_messa...
chrome/common/render_messages.h:289: int /* start */, int /* end */)
nit: one per line

http://codereview.chromium.org/11359198/diff/13001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:406: return
v8::Boolean::New(base::i18n::IsRTL());
Can IsRTL change while Chrome is running?

[dhollowa, 2012-11-16 22:14:27]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
On 2012/11/16 22:08:26, samarth wrote:
> On 2012/11/16 17:29:03, dhollowa wrote:
> > A couple questions.  (1) Isn't this supposed to be the other way round?  (2)
> To
> > compute |end|, which is a width, I'd assume you'd need the containing bounds
> to
> > determine it correctly.
> > 
> > This naming is the source of the confusion I think.  We should change the
> > implementation and the spec to be something like
|SetMarginSize(first_margin,
> > last_margin)| or something.
> 
> I'm also confused why this isn't the other way around.
> 
> Re (2), David: are the bounds reported by GetBoundsInScreen() not absolute?

Not sure, but consider: b = { x = 10, y = 20, w = 50, h = 60 },
then x is the "left margin" assuming 0 is the left edge of the container.  But
what is the right edge of the container.  I'd assume it is greater than x+w, and
that (x+w) - right_edge is the "right margin" we're looking for.  Or am I being
dense?

[melevin, 2012-11-16 22:17:14]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
On 2012/11/16 17:29:03, dhollowa wrote:
> A couple questions.  (1) Isn't this supposed to be the other way round?

Oops, yes it is.

>  (2) To
> compute |end|, which is a width, I'd assume you'd need the containing bounds
to
> determine it correctly.
> 
> This naming is the source of the confusion I think.  We should change the
> implementation and the spec to be something like |SetMarginSize(first_margin,
> last_margin)| or something.

We can't change the naming because it is defined in the Searchbox API as
"startMargin" and "endMargin". However, I looked into using the container's
dimensions to calculate the width from the end edge and changed the code to use
that instead. So now we are actually passing around the start and end margin
size and the naming should make sense.

[melevin, 2012-11-16 22:34:43]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:45: // An artificial delay (in
milliseconds) we introduce before telling the Instant
On 2012/11/16 22:08:26, samarth wrote:
> I suspect this was necessary because the height of the popup is fairly
volatile.
> I hope we don't need this for the omnibox bounds, but something to keep in
mind.

We'll have to test once the JavaScript is wired.

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:344: // Clear the margins so they
are set when the preview is shown again.
On 2012/11/16 22:08:26, samarth wrote:
> Why is this necessary?

When the SearchBox object is created it does not have the bounds set. We need to
clear these here so that the margin-change event goes through and sets them.
Perhaps there is a better way but I could not find any other example of the
SearchBox pulling information from Instant on creation.

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.h (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.h:266: // Offset from the start edge
of the page to the start of the page text area.
On 2012/11/16 22:08:26, samarth wrote:
> Page text area is a little confusing in this context. Do you mean "omnibox
text
> area"?

Yeah I suppose. I wasn't sure what we were calling it since this is not only the
dropdown margin but also the results margin.

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
On 2012/11/16 22:08:26, samarth wrote:
> On 2012/11/16 17:29:03, dhollowa wrote:
> > A couple questions.  (1) Isn't this supposed to be the other way round?  (2)
> To
> > compute |end|, which is a width, I'd assume you'd need the containing bounds
> to
> > determine it correctly.
> > 
> > This naming is the source of the confusion I think.  We should change the
> > implementation and the spec to be something like
|SetMarginSize(first_margin,
> > last_margin)| or something.
> 
> I'm also confused why this isn't the other way around.
> 
> Re (2), David: are the bounds reported by GetBoundsInScreen() not absolute?

They are relative to the browser window, so x=0 y=0 refers to the top-left
corner of the browser window regardless of RTL status.

http://codereview.chromium.org/11359198/diff/13001/chrome/common/render_messa...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/common/render_messa...
chrome/common/render_messages.h:289: int /* start */, int /* end */)
On 2012/11/16 22:08:26, samarth wrote:
> nit: one per line

Done.

http://codereview.chromium.org/11359198/diff/13001/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:406: return
v8::Boolean::New(base::i18n::IsRTL());
On 2012/11/16 22:08:26, samarth wrote:
> Can IsRTL change while Chrome is running?

It's not impossible but I think would be extremely rare. You would need to
change the system interface language which I believe few systems (maybe Linux
can?) can do on-the-fly.

[samarth, 2012-11-19 21:42:44]

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:344: // Clear the margins so they
are set when the preview is shown again.
On 2012/11/16 22:34:43, melevin wrote:
> On 2012/11/16 22:08:26, samarth wrote:
> > Why is this necessary?
> 
> When the SearchBox object is created it does not have the bounds set. We need
to
> clear these here so that the margin-change event goes through and sets them.
> Perhaps there is a better way but I could not find any other example of the
> SearchBox pulling information from Instant on creation.

Take a look at ResetLoader. Whenever we load a new instant page, we send it any
state it expects to have already.

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

http://codereview.chromium.org/11359198/diff/13001/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1021: if (base::i18n::IsRTL()) {
On 2012/11/16 22:34:43, melevin wrote:
> On 2012/11/16 22:08:26, samarth wrote:
> > On 2012/11/16 17:29:03, dhollowa wrote:
> > > A couple questions.  (1) Isn't this supposed to be the other way round? 
(2)
> > To
> > > compute |end|, which is a width, I'd assume you'd need the containing
bounds
> > to
> > > determine it correctly.
> > > 
> > > This naming is the source of the confusion I think.  We should change the
> > > implementation and the spec to be something like
> |SetMarginSize(first_margin,
> > > last_margin)| or something.
> > 
> > I'm also confused why this isn't the other way around.
> > 
> > Re (2), David: are the bounds reported by GetBoundsInScreen() not absolute?
> 
> They are relative to the browser window, so x=0 y=0 refers to the top-left
> corner of the browser window regardless of RTL status.

Ok, I get it now. I was misreading the API: I thought we were sending the start
and end coordinates relative to the start edge.

We can actually change the API naming and semantics since it's not published yet
(only exists at go/searchboxapi3). Let's quickly chat with David (Black) about
what makes the most sense here.

[melevin, 2012-11-20 22:32:20]

https://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:344: // Clear the margins so they
are set when the preview is shown again.
On 2012/11/19 21:42:44, samarth wrote:
> On 2012/11/16 22:34:43, melevin wrote:
> > On 2012/11/16 22:08:26, samarth wrote:
> > > Why is this necessary?
> > 
> > When the SearchBox object is created it does not have the bounds set. We
need
> to
> > clear these here so that the margin-change event goes through and sets them.
> > Perhaps there is a better way but I could not find any other example of the
> > SearchBox pulling information from Instant on creation.
> 
> Take a look at ResetLoader. Whenever we load a new instant page, we send it
any
> state it expects to have already.

I only see the instant URL being communicated to the TabContents in ResetLoader.
It's not clear to me how the page margins would be communicated this way, I
don't think it's right to wire them through the TabContents object.

[samarth, 2012-11-20 22:47:37]

https://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/13001/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:344: // Clear the margins so they
are set when the preview is shown again.
On 2012/11/20 22:32:20, melevin wrote:
> On 2012/11/19 21:42:44, samarth wrote:
> > On 2012/11/16 22:34:43, melevin wrote:
> > > On 2012/11/16 22:08:26, samarth wrote:
> > > > Why is this necessary?
> > > 
> > > When the SearchBox object is created it does not have the bounds set. We
> need
> > to
> > > clear these here so that the margin-change event goes through and sets
them.
> > > Perhaps there is a better way but I could not find any other example of
the
> > > SearchBox pulling information from Instant on creation.
> > 
> > Take a look at ResetLoader. Whenever we load a new instant page, we send it
> any
> > state it expects to have already.
> 
> I only see the instant URL being communicated to the TabContents in
ResetLoader.
> It's not clear to me how the page margins would be communicated this way, I
> don't think it's right to wire them through the TabContents object.

I meant something like
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
where we make sure a fresh instant overlay has the correct context (we used to
also update the omnibox focus state here before that got ripped out completely).

[melevin, 2012-11-21 22:06:30]

On 2012/11/20 22:47:37, samarth wrote:
> > I only see the instant URL being communicated to the TabContents in
> ResetLoader.
> > It's not clear to me how the page margins would be communicated this way, I
> > don't think it's right to wire them through the TabContents object.
> 
> I meant something like
>
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
> where we make sure a fresh instant overlay has the correct context (we used to
> also update the omnibox focus state here before that got ripped out
completely).

Did you mean:
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...

?

I could rig it to set the margins as part of the "search-mode changed" event.

[samarth, 2012-11-21 23:43:50]

On 2012/11/21 22:06:30, melevin wrote:
> On 2012/11/20 22:47:37, samarth wrote:
> > > I only see the instant URL being communicated to the TabContents in
> > ResetLoader.
> > > It's not clear to me how the page margins would be communicated this way,
I
> > > don't think it's right to wire them through the TabContents object.
> > 
> > I meant something like
> >
>
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
> > where we make sure a fresh instant overlay has the correct context (we used
to
> > also update the omnibox focus state here before that got ripped out
> completely).
> 
> Did you mean:
>
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
> 
> ?

Yes, that.  (Sorry apparently code search does not point to a particular version
of a file.)

> 
> I could rig it to set the margins as part of the "search-mode changed" event.

I don't think you want to tie it to the search context change.  But you can just
add a call there for the margin:
if (extended_enabled_) {
  loader_->OnActiveTabModeChanged(..)
  loader_->SetMarginSize(...);
}
Does that solve the issue?

[melevin, 2012-11-28 00:34:53]

On 2012/11/21 23:43:50, samarth wrote:
> On 2012/11/21 22:06:30, melevin wrote:
> > On 2012/11/20 22:47:37, samarth wrote:
> > > > I only see the instant URL being communicated to the TabContents in
> > > ResetLoader.
> > > > It's not clear to me how the page margins would be communicated this
way,
> I
> > > > don't think it's right to wire them through the TabContents object.
> > > 
> > > I meant something like
> > >
> >
>
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
> > > where we make sure a fresh instant overlay has the correct context (we
used
> to
> > > also update the omnibox focus state here before that got ripped out
> > completely).
> > 
> > Did you mean:
> >
>
https://cs.corp.google.com/#chrome/src/chrome/browser/instant/instant_control...
> > 
> > ?
> 
> Yes, that.  (Sorry apparently code search does not point to a particular
version
> of a file.)
> 
> > 
> > I could rig it to set the margins as part of the "search-mode changed"
event.
> 
> I don't think you want to tie it to the search context change.  But you can
just
> add a call there for the margin:
> if (extended_enabled_) {
>   loader_->OnActiveTabModeChanged(..)
>   loader_->SetMarginSize(...);
> }
> Does that solve the issue?

That works, updated the CL.

[samarth, 2012-11-28 03:51:19]

A few nits below but otherwise lgtm.

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:717: // Ensure the searchbox API
has the correct theme-related info and context.
Update comment please

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
nit: make this const

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:7: #include <ctype.h>
Why do you need this?

[dhollowa, 2012-11-28 16:44:08]

LGTM w/ nits.

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1014: int tmp = start;
std::swap(start, end)

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:58: double zoom = GetZoom();
IMO |GetZoom| should return float.  Double is excessive (I know Webkit returns
this but Webkit also casts to float internally).
And then you can use rect_f.h / |RectF::Scale| directly to do the
scaling/zooming.

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:45: int GetStartMargin();
nit: const

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:46: int GetEndMargin();
nit: const

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:49: gfx::Rect GetPopupBounds();
nit: const

[melevin, 2012-11-28 20:28:18]

Adding sky@ for chrome/common changes.

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/instant/ins...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/instant/ins...
chrome/browser/instant/instant_controller.cc:717: // Ensure the searchbox API
has the correct theme-related info and context.
On 2012/11/28 03:51:19, samarth wrote:
> Update comment please

Done.

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1014: int tmp = start;
On 2012/11/28 16:44:08, dhollowa wrote:
> std::swap(start, end)

I've always been conflicted about this function. It certainly looks cleaner but
is it worth it to #include <algorithm> just to swap two numbers?

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:58: double zoom = GetZoom();
On 2012/11/28 16:44:08, dhollowa wrote:
> IMO |GetZoom| should return float.  Double is excessive (I know Webkit returns
> this but Webkit also casts to float internally).
> And then you can use rect_f.h / |RectF::Scale| directly to do the
> scaling/zooming.

That's true but we would have to convert it to a gfx::Rect again, I'm not sure
that it will look any cleaner.

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:45: int GetStartMargin();
On 2012/11/28 16:44:08, dhollowa wrote:
> nit: const

Done.

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:46: int GetEndMargin();
On 2012/11/28 16:44:08, dhollowa wrote:
> nit: const

Done.

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:49: gfx::Rect GetPopupBounds();
On 2012/11/28 16:44:08, dhollowa wrote:
> nit: const

Done.

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
On 2012/11/28 03:51:19, samarth wrote:
> nit: make this const

I tried this originally, the compiler was not amused:

../../chrome/renderer/searchbox/searchbox.cc: In member function ‘double
SearchBox::GetZoom() const’:
../../chrome/renderer/searchbox/searchbox.cc:235:43: error: passing ‘const
SearchBox’ as ‘this’ argument of ‘content::RenderView*
content::RenderViewObserver::render_view()’ discards qualifiers [-fpermissive]

However, it looks like we can also make that function const without any
problems. Is this a part of the codebase we can change?

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox_extension.cc:7: #include <ctype.h>
On 2012/11/28 03:51:19, samarth wrote:
> Why do you need this?

Oops, I think this was a bad merge.

[dhollowa, 2012-11-28 20:44:33]

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/browser/ui/omnibox/...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1014: int tmp = start;
On 2012/11/28 20:28:19, melevin wrote:
> On 2012/11/28 16:44:08, dhollowa wrote:
> > std::swap(start, end)
> 
> I've always been conflicted about this function. It certainly looks cleaner
but
> is it worth it to #include <algorithm> just to swap two numbers?

Yes.  It expresses intent clearly and adds to readability.

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.cc:58: double zoom = GetZoom();
On 2012/11/28 20:28:19, melevin wrote:
> On 2012/11/28 16:44:08, dhollowa wrote:
> > IMO |GetZoom| should return float.  Double is excessive (I know Webkit
returns
> > this but Webkit also casts to float internally).
> > And then you can use rect_f.h / |RectF::Scale| directly to do the
> > scaling/zooming.
> 
> That's true but we would have to convert it to a gfx::Rect again, I'm not sure
> that it will look any cleaner.

Ya, ok.

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
On 2012/11/28 20:28:19, melevin wrote:
> On 2012/11/28 03:51:19, samarth wrote:
> > nit: make this const
> 
> I tried this originally, the compiler was not amused:
> 
> ../../chrome/renderer/searchbox/searchbox.cc: In member function ‘double
> SearchBox::GetZoom() const’:
> ../../chrome/renderer/searchbox/searchbox.cc:235:43: error: passing ‘const
> SearchBox’ as ‘this’ argument of ‘content::RenderView*
> content::RenderViewObserver::render_view()’ discards qualifiers [-fpermissive]
> 
> However, it looks like we can also make that function const without any
> problems. Is this a part of the codebase we can change?

That's annoying, but yes, worth a try.  You'll want to include jam@ as reviewer
for that change.

[melevin, 2012-11-28 21:39:23]

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

https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
On 2012/11/28 20:44:33, dhollowa wrote:
> On 2012/11/28 20:28:19, melevin wrote:
> > On 2012/11/28 03:51:19, samarth wrote:
> > > nit: make this const
> > 
> > I tried this originally, the compiler was not amused:
> > 
> > ../../chrome/renderer/searchbox/searchbox.cc: In member function ‘double
> > SearchBox::GetZoom() const’:
> > ../../chrome/renderer/searchbox/searchbox.cc:235:43: error: passing ‘const
> > SearchBox’ as ‘this’ argument of ‘content::RenderView*
> > content::RenderViewObserver::render_view()’ discards qualifiers
[-fpermissive]
> > 
> > However, it looks like we can also make that function const without any
> > problems. Is this a part of the codebase we can change?
> 
> That's annoying, but yes, worth a try.  You'll want to include jam@ as
reviewer
> for that change.

The code review tool says "jam" isn't a valid user. Did you mean joi@?

[dhollowa, 2012-11-28 21:52:55]

On 2012/11/28 21:39:23, melevin wrote:
>
https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
> File chrome/renderer/searchbox/searchbox.h (right):
> 
>
https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
> chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
> On 2012/11/28 20:44:33, dhollowa wrote:
> > On 2012/11/28 20:28:19, melevin wrote:
> > > On 2012/11/28 03:51:19, samarth wrote:
> > > > nit: make this const
> > > 
> > > I tried this originally, the compiler was not amused:
> > > 
> > > ../../chrome/renderer/searchbox/searchbox.cc: In member function ‘double
> > > SearchBox::GetZoom() const’:
> > > ../../chrome/renderer/searchbox/searchbox.cc:235:43: error: passing ‘const
> > > SearchBox’ as ‘this’ argument of ‘content::RenderView*
> > > content::RenderViewObserver::render_view()’ discards qualifiers
> [-fpermissive]
> > > 
> > > However, it looks like we can also make that function const without any
> > > problems. Is this a part of the codebase we can change?
> > 
> > That's annoying, but yes, worth a try.  You'll want to include jam@ as
> reviewer
> > for that change.
> 
> The code review tool says "jam" isn't a valid user. Did you mean joi@?

Trust OWNERS, ya.

[melevin, 2012-11-28 22:01:14]

On 2012/11/28 21:52:55, dhollowa wrote:
> On 2012/11/28 21:39:23, melevin wrote:
> >
>
https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
> > File chrome/renderer/searchbox/searchbox.h (right):
> > 
> >
>
https://codereview.chromium.org/11359198/diff/9003/chrome/renderer/searchbox/...
> > chrome/renderer/searchbox/searchbox.h:79: double GetZoom();
> > On 2012/11/28 20:44:33, dhollowa wrote:
> > > On 2012/11/28 20:28:19, melevin wrote:
> > > > On 2012/11/28 03:51:19, samarth wrote:
> > > > > nit: make this const
> > > > 
> > > > I tried this originally, the compiler was not amused:
> > > > 
> > > > ../../chrome/renderer/searchbox/searchbox.cc: In member function ‘double
> > > > SearchBox::GetZoom() const’:
> > > > ../../chrome/renderer/searchbox/searchbox.cc:235:43: error: passing
‘const
> > > > SearchBox’ as ‘this’ argument of ‘content::RenderView*
> > > > content::RenderViewObserver::render_view()’ discards qualifiers
> > [-fpermissive]
> > > > 
> > > > However, it looks like we can also make that function const without any
> > > > problems. Is this a part of the codebase we can change?
> > > 
> > > That's annoying, but yes, worth a try.  You'll want to include jam@ as
> > reviewer
> > > for that change.
> > 
> > The code review tool says "jam" isn't a valid user. Did you mean joi@?
> 
> Trust OWNERS, ya.

Adding joi@ for content/public changes.

[sky, 2012-11-28 22:29:01]

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1008: void
OmniboxEditModel::OnOmniboxBoundsChanged(const gfx::Rect& omnibox_bounds,
Passing bounds like this strikes me as specific to the implementation and not
something the model should care about. This method should a single bounds and if
you need insets/margins it should take that too.

[Jói, 2012-11-29 16:33:25]

LGTM for content/public.

[sreeram, 2012-11-29 19:22:48]

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:340: !GetPreviewContents() ||
loader_->IsPointerDownFromActivate())
Add "|| !extended_enabled_".

Remove "loader_->IsPointerDownFromActivate()". It doesn't apply here.

It was added to SendBoundsToPage because, when the user clicks on the preview,
omnibox loses focus and closes the popup. This might cause an onresize() event
that causes the page to shift its results, and thus whatever the user was trying
to click. So, we check to make sure that the click isn't in progress before
sending the resize. Same reason why we delay the commit until the mouse has been
released.

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

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:183: void SendBoundsToPage();
Rename to SendPopupBoundsToPage() so it's clearer.

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:447: // parent view's
bounds so that the end margin can be calculated.
What does "gray text" have to do with this? I thought the page needed the
omnibox bounds so that it could size its suggestions list accordingly.

https://codereview.chromium.org/11359198/diff/13002/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:242: }
Move this above Reset() to match the .h order.

[melevin, 2012-11-30 23:37:09]

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:340: !GetPreviewContents() ||
loader_->IsPointerDownFromActivate())
On 2012/11/29 19:22:49, sreeram wrote:
> Add "|| !extended_enabled_".
> 
> Remove "loader_->IsPointerDownFromActivate()". It doesn't apply here.
> 
> It was added to SendBoundsToPage because, when the user clicks on the preview,
> omnibox loses focus and closes the popup. This might cause an onresize() event
> that causes the page to shift its results, and thus whatever the user was
trying
> to click. So, we check to make sure that the click isn't in progress before
> sending the resize. Same reason why we delay the commit until the mouse has
been
> released.

Thanks for the explanation, I just copy-pasted it without thinking too hard
about it.

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

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:183: void SendBoundsToPage();
On 2012/11/29 19:22:49, sreeram wrote:
> Rename to SendPopupBoundsToPage() so it's clearer.

Done.

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:1008: void
OmniboxEditModel::OnOmniboxBoundsChanged(const gfx::Rect& omnibox_bounds,
On 2012/11/28 22:29:02, sky wrote:
> Passing bounds like this strikes me as specific to the implementation and not
> something the model should care about. This method should a single bounds and
if
> you need insets/margins it should take that too.

Done.

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:447: // parent view's
bounds so that the end margin can be calculated.
On 2012/11/29 19:22:49, sreeram wrote:
> What does "gray text" have to do with this? I thought the page needed the
> omnibox bounds so that it could size its suggestions list accordingly.

Yes, that's what we need it for. But we can't use the dimensions of the
OmniboxView for that. When gray text is showing that view does not fill the
entire location bar.

https://codereview.chromium.org/11359198/diff/13002/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/13002/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:242: }
On 2012/11/29 19:22:49, sreeram wrote:
> Move this above Reset() to match the .h order.

Done.

[sreeram, 2012-12-01 00:04:25]

lgtm

[commit-bot: I haz the power, 2012-12-03 20:14:30]

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

[commit-bot: I haz the power, 2012-12-03 20:14:47]

Presubmit check for 11359198-15018 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/render_messages.h
    chrome/browser/ui/views
    chrome/browser/ui/omnibox

Presubmit checks took 8.4s to calculate.

[melevin, 2012-12-03 21:47:08]

On 2012/12/03 20:14:47, I haz the power (commit-bot) wrote:
> Presubmit check for 11359198-15018 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     chrome/common/render_messages.h

Adding palmer@ for security review.

>     chrome/browser/ui/views
>     chrome/browser/ui/omnibox

Sorry for the premature submit, didn't realize sky@ hadn't lgtm'd.

> Presubmit checks took 8.4s to calculate.

[palmer, 2012-12-03 23:00:36]

LGTM from an IPC security perspective.

[melevin, 2012-12-04 22:53:14]

Rebased after Sreeram's changes in r171018. Sreeram, please have another quick
look to make sure I didn't mess it up.

Also, sky@, I'm still waiting on your review. Thanks!

[sky, 2012-12-04 23:06:36]

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:276: // Invoked when the popup
has changed its bounds to |bounds|.
You need to document the coordinate system of both of these functions.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:204: virtual gfx::Rect
GetContainerBounds() const = 0;
It's not clear what you mean by 'container' here, and your implementation isn't
really doing what the description says.

It seems you only use this in OmniboxEditModel::OnOmniboxBoundsChanged. Instead
of adding a new method make OnOmniboxBoundsChanged take the region you need
(this echoes my earlier comment).

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:449:
model()->OnOmniboxBoundsChanged(location_bar_view_->GetBoundsInScreen());
The locationbarview might contain a bunch of other views, eg the star, ev
stuff... Are you sure you want to include them? Also, if you care about the
bounds of the lbv, why is this here and not in the lbv?

[sreeram, 2012-12-05 00:14:46]

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:86: // Tells the preview page what size
start and end margins to use.
Nit: preview page -> page

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:370: return;
The margin should be sent to committed tabs too. Instead of trying to wrangle
which should be used, I'd just send it to both:

if (!extended_enabled_ || (start_margin_ == ...))
  return;

start_margin_ = start;
end_margin_ = end;

if (loader_)
  loader_->SetMarginSize(...);
if (instant_tab_)
  instant_tab_->SetMarginSize(...);

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:828: instant_tab_->Init();
Similar to ResetLoader(), you should do this here:
  instant_tab_->SetMarginSize(...);

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:940: if (last_omnibox_bounds_ ==
omnibox_bounds_ || !loader_ ||
Shouldn't this be "last_popup_bounds_ == popup_bounds_"?

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

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:249: gfx::Rect last_omnibox_bounds_;
Shouldn't these (omnibox_bounds_ and last_omnibox_bounds_) be removed now?

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:263: // Timer used to update the
bounds of the omnibox.
"... bounds of the omnibox popup."

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:161: DVLOG(1) << render_view() << "
OnResize";
Nit: OnResize -> OnPopupResize

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

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:49: // Returns the bounds of the Searchbox
popup in screen coordinates.
Nit: searchbox -> omnibox

[melevin, 2012-12-06 23:12:59]

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_client.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_client.h:86: // Tells the preview page what size
start and end margins to use.
On 2012/12/05 00:14:46, sreeram wrote:
> Nit: preview page -> page

Done.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:370: return;
On 2012/12/05 00:14:46, sreeram wrote:
> The margin should be sent to committed tabs too. Instead of trying to wrangle
> which should be used, I'd just send it to both:
> 
> if (!extended_enabled_ || (start_margin_ == ...))
>   return;
> 
> start_margin_ = start;
> end_margin_ = end;
> 
> if (loader_)
>   loader_->SetMarginSize(...);
> if (instant_tab_)
>   instant_tab_->SetMarginSize(...);

Done.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:828: instant_tab_->Init();
On 2012/12/05 00:14:46, sreeram wrote:
> Similar to ResetLoader(), you should do this here:
>   instant_tab_->SetMarginSize(...);

Added this but I'm not sure what it's doing. Once the page is committed, the
start/end margin remain inaccessible. Do we want these values to be accessible
after commit?

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:940: if (last_omnibox_bounds_ ==
omnibox_bounds_ || !loader_ ||
On 2012/12/05 00:14:46, sreeram wrote:
> Shouldn't this be "last_popup_bounds_ == popup_bounds_"?

Done.

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

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:249: gfx::Rect last_omnibox_bounds_;
On 2012/12/05 00:14:46, sreeram wrote:
> Shouldn't these (omnibox_bounds_ and last_omnibox_bounds_) be removed now?

Yup, done.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:263: // Timer used to update the
bounds of the omnibox.
On 2012/12/05 00:14:46, sreeram wrote:
> "... bounds of the omnibox popup."

Done.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_edit_model.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_edit_model.h:276: // Invoked when the popup
has changed its bounds to |bounds|.
On 2012/12/04 23:06:36, sky wrote:
> You need to document the coordinate system of both of these functions.

Done.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/omnibox...
chrome/browser/ui/omnibox/omnibox_view.h:204: virtual gfx::Rect
GetContainerBounds() const = 0;
On 2012/12/04 23:06:36, sky wrote:
> It's not clear what you mean by 'container' here, and your implementation
isn't
> really doing what the description says.
> 
> It seems you only use this in OmniboxEditModel::OnOmniboxBoundsChanged.
Instead
> of adding a new method make OnOmniboxBoundsChanged take the region you need
> (this echoes my earlier comment).

I've removed both functions. The LocationBarView will now compute and pass the
margins directly to Instant.

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/views/o...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/ui/views/o...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:449:
model()->OnOmniboxBoundsChanged(location_bar_view_->GetBoundsInScreen());
On 2012/12/04 23:06:36, sky wrote:
> The locationbarview might contain a bunch of other views, eg the star, ev
> stuff... Are you sure you want to include them?

Yes, we want the outer bounds of the location box.

> Also, if you care about the
> bounds of the lbv, why is this here and not in the lbv?

Ok, I've moved the code to respond to a change in the bounds of the
LocationBarView.

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:161: DVLOG(1) << render_view() << "
OnResize";
On 2012/12/05 00:14:46, sreeram wrote:
> Nit: OnResize -> OnPopupResize

Done.

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

https://codereview.chromium.org/11359198/diff/12055/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.h:49: // Returns the bounds of the Searchbox
popup in screen coordinates.
On 2012/12/05 00:14:46, sreeram wrote:
> Nit: searchbox -> omnibox

Done.

[samarth, 2012-12-07 01:44:48]

Still LGTM but one nit below.

Thanks,
Samarth

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:828: instant_tab_->Init();
On 2012/12/06 23:13:00, melevin wrote:
> On 2012/12/05 00:14:46, sreeram wrote:
> > Similar to ResetLoader(), you should do this here:
> >   instant_tab_->SetMarginSize(...);
> 
> Added this but I'm not sure what it's doing. Once the page is committed, the
> start/end margin remain inaccessible. Do we want these values to be accessible
> after commit?

Yes, why not?  Search pages can be committed and they should have access to the
current values for the margins.  In the future, the NTP will be committed too.

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

https://codereview.chromium.org/11359198/diff/28002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:68: // Sets the widths of the start
and end margins of the Instant page.
nit: this isn't quite right.  Something like "Sets the start and end margins of
the omnibox text area."

[melevin, 2012-12-08 00:04:09]

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
File chrome/browser/instant/instant_controller.cc (right):

https://codereview.chromium.org/11359198/diff/12055/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.cc:828: instant_tab_->Init();
On 2012/12/07 01:44:49, samarth wrote:
> On 2012/12/06 23:13:00, melevin wrote:
> > On 2012/12/05 00:14:46, sreeram wrote:
> > > Similar to ResetLoader(), you should do this here:
> > >   instant_tab_->SetMarginSize(...);
> > 
> > Added this but I'm not sure what it's doing. Once the page is committed, the
> > start/end margin remain inaccessible. Do we want these values to be
accessible
> > after commit?
> 
> Yes, why not?  Search pages can be committed and they should have access to
the
> current values for the margins.  In the future, the NTP will be committed too.

Oops, it wasn't working due to a copy-paste error. Fixed and working now.

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

https://codereview.chromium.org/11359198/diff/28002/chrome/browser/instant/in...
chrome/browser/instant/instant_controller.h:68: // Sets the widths of the start
and end margins of the Instant page.
On 2012/12/07 01:44:49, samarth wrote:
> nit: this isn't quite right.  Something like "Sets the start and end margins
of
> the omnibox text area."

Done.

[melevin, 2012-12-12 00:06:49]

Scott, is there a chance you can take another look at this CL this week? We are
trying to meet an end-of-week deadline and there is another CL that we need to
follow-up with after this one to fix an important issue.

[sky, 2012-12-12 03:46:55]

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:88: void SetMarginSize(int start,
int end);
This isn't margins but actual position, right? Wouldn't a better name be
SetOmniboxViewBounds(int leading, int trailing)?

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

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1411: void
LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
Make position match that of header.

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1412: if (browser_ &&
browser_->instant_controller()) {
Add a check for parent() here too.

[melevin, 2012-12-12 19:32:43]

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:88: void SetMarginSize(int start,
int end);
On 2012/12/12 03:46:55, sky wrote:
> This isn't margins but actual position, right? Wouldn't a better name be
> SetOmniboxViewBounds(int leading, int trailing)?

All of these functions refer to margins because they are implementing the
"startMargin" and "endMargin" properties of the SearchBox API:
https://docs.google.com/a/google.com/document/d/11aOAG80JauTJBsblI2h7RF6xa5uc...

I also think its a bit misleading to call a "SetOmniboxViewBounds" with anything
other than a rect that has the actual OmniboxView bounds (which is not what we
want either).

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

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1411: void
LocationBarView::OnBoundsChanged(const gfx::Rect& previous_bounds) {
On 2012/12/12 03:46:55, sky wrote:
> Make position match that of header.

Done.

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:1412: if (browser_ &&
browser_->instant_controller()) {
On 2012/12/12 03:46:55, sky wrote:
> Add a check for parent() here too.

Done.

[sky, 2012-12-12 21:12:46]

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.h (right):

https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.h:88: void SetMarginSize(int start,
int end);
On 2012/12/12 19:32:43, melevin wrote:
> On 2012/12/12 03:46:55, sky wrote:
> > This isn't margins but actual position, right? Wouldn't a better name be
> > SetOmniboxViewBounds(int leading, int trailing)?
> 
> All of these functions refer to margins because they are implementing the
> "startMargin" and "endMargin" properties of the SearchBox API:
>
https://docs.google.com/a/google.com/document/d/11aOAG80JauTJBsblI2h7RF6xa5uc...
> 
> I also think its a bit misleading to call a "SetOmniboxViewBounds" with
anything
> other than a rect that has the actual OmniboxView bounds (which is not what we
> want either).

I agree that bounds implies a rectangle. That is easily fixed by passing in an
actual rectangle. I still think margin is a bad name here and in the search box
API.

[sky, 2012-12-12 21:40:43]

On 2012/12/12 21:12:46, sky wrote:
>
https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
> File chrome/browser/ui/browser_instant_controller.h (right):
> 
>
https://codereview.chromium.org/11359198/diff/36001/chrome/browser/ui/browser...
> chrome/browser/ui/browser_instant_controller.h:88: void SetMarginSize(int
start,
> int end);
> On 2012/12/12 19:32:43, melevin wrote:
> > On 2012/12/12 03:46:55, sky wrote:
> > > This isn't margins but actual position, right? Wouldn't a better name be
> > > SetOmniboxViewBounds(int leading, int trailing)?
> > 
> > All of these functions refer to margins because they are implementing the
> > "startMargin" and "endMargin" properties of the SearchBox API:
> >
>
https://docs.google.com/a/google.com/document/d/11aOAG80JauTJBsblI2h7RF6xa5uc...
> > 
> > I also think its a bit misleading to call a "SetOmniboxViewBounds" with
> anything
> > other than a rect that has the actual OmniboxView bounds (which is not what
we
> > want either).
> 
> I agree that bounds implies a rectangle. That is easily fixed by passing in an
> actual rectangle. I still think margin is a bad name here and in the search
box
> API.

In chrome code a margin or inset is, to my knowledge, never relative to the
screen. What you're passing in here isn't a size either, it's the left/right (or
leading/trailing) edges. The InstantController method should be better named to
match the chrome conventions, eg SetOmniboxViewBounds(const gfx::Rect&) (ideally
you would change InstantController::SetOmniboxBounds to SetOmniboxPopupBounds).
You can strip out the parts you need and pass them down to the appropriate
search box API.

[melevin, 2012-12-12 21:57:18]

On 2012/12/12 21:40:43, sky wrote:
> In chrome code a margin or inset is, to my knowledge, never relative to the
> screen. What you're passing in here isn't a size either, it's the left/right
(or
> leading/trailing) edges.

I'm not sure I follow your reasoning. The margins passed are the screen
dimensions of the left and right page margins on the Instant search results page
and in the Searchbox dropdown. These are certainly sizes and would probably be
implemented using CSS "margin-left" and "margin-right". I'm not sure I
understand what the issue with this naming is.

> The InstantController method should be better named to
> match the chrome conventions, eg SetOmniboxViewBounds(const gfx::Rect&)

Well here is the problem-- it's not actually passing to Instant the dimensions
of the OmniboxView so this naming would not be ideal either. It's passing in the
distance from the left edge of the LocationBoxView and it's parent's left edge
and likewise the right edge. These values can't really be expressed as a
"bounds" unless we call two functions to set the bounds of the LocationBoxView
and then again for it's parent which I think makes the API more cumbersome than
it needs to be.

> (ideally
> you would change InstantController::SetOmniboxBounds to
SetOmniboxPopupBounds).
> You can strip out the parts you need and pass them down to the appropriate
> search box API.

[sky, 2012-12-12 22:18:06]

On Wed, Dec 12, 2012 at 1:57 PM,  <melevin@chromium.org> wrote:
> On 2012/12/12 21:40:43, sky wrote:
>>
>> In chrome code a margin or inset is, to my knowledge, never relative to
>> the
>> screen. What you're passing in here isn't a size either, it's the
>> left/right
>
> (or
>>
>> leading/trailing) edges.
>
>
> I'm not sure I follow your reasoning. The margins passed are the screen
> dimensions of the left and right page margins on the Instant search results
> page
> and in the Searchbox dropdown. These are certainly sizes and would probably
> be
> implemented using CSS "margin-left" and "margin-right". I'm not sure I
> understand what the issue with this naming is.
>
>
>> The InstantController method should be better named to
>> match the chrome conventions, eg SetOmniboxViewBounds(const gfx::Rect&)
>
>
> Well here is the problem-- it's not actually passing to Instant the
> dimensions
> of the OmniboxView so this naming would not be ideal either. It's passing in
> the
> distance from the left edge of the LocationBoxView and it's parent's left
> edge
> and likewise the right edge. These values can't really be expressed as a
> "bounds" unless we call two functions to set the bounds of the
> LocationBoxView
> and then again for it's parent which I think makes the API more cumbersome
> than
> it needs to be.

Ah, ok. In that case I have the following question.

  if (browser_ && browser_->instant_controller() && parent()) {
    // Pass the side margins of the location bar to the Instant Controller.
    const gfx::Rect bounds = GetBoundsInScreen();
    const gfx::Rect parent_bounds = parent()->GetBoundsInScreen();
    int start = bounds.x();
    int end =
        parent_bounds.x() + parent_bounds.width() - bounds.width() - start;
    if (base::i18n::IsRTL())
      std::swap(start, end);
    browser_->instant_controller()->SetMarginSize(start, end);
  }
}

My confusion is start (bounds.x()) is in terms of screen coordinates.
That is, the position of the LocationBarView in screen coordinates,
but end is a distance from the right edge of the parent. Is that
really what you want? Shouldn't the coordinates be in the same
coordinate system?

If this is really what you want, then:

end = parent_bounds.right() - bounds.right();

is a lot easier to understand.

  -Scott

[melevin, 2012-12-13 00:07:13]

On 2012/12/12 22:18:06, sky wrote:
> Ah, ok. In that case I have the following question.
> 
>   if (browser_ && browser_->instant_controller() && parent()) {
>     // Pass the side margins of the location bar to the Instant Controller.
>     const gfx::Rect bounds = GetBoundsInScreen();
>     const gfx::Rect parent_bounds = parent()->GetBoundsInScreen();
>     int start = bounds.x();
>     int end =
>         parent_bounds.x() + parent_bounds.width() - bounds.width() - start;
>     if (base::i18n::IsRTL())
>       std::swap(start, end);
>     browser_->instant_controller()->SetMarginSize(start, end);
>   }
> }
> 
> My confusion is start (bounds.x()) is in terms of screen coordinates.
> That is, the position of the LocationBarView in screen coordinates,
> but end is a distance from the right edge of the parent. Is that
> really what you want? Shouldn't the coordinates be in the same
> coordinate system?

Yes, they should be. It's a subtle oversight since the parent container has an
"x" value of 0. I've changed it to subtract out the parent value "x". Hopefully
the intent is clearer now.

> If this is really what you want, then:
> 
> end = parent_bounds.right() - bounds.right();

Ah, I didn't know about the "right" function. That is much clearer.
 
> is a lot easier to understand.
> 
>   -Scott

[sky, 2012-12-13 00:58:11]

LGTM

[commit-bot: I haz the power, 2012-12-13 01:09:30]

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

[commit-bot: I haz the power, 2012-12-13 01:09:38]

Failed to apply patch for chrome/browser/instant/instant_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/instant/instant_controller.cc
  Hunk #1 FAILED at 157.
  Hunk #3 FAILED at 840.
  Hunk #4 succeeded at 908 (offset 27 lines).
  Hunk #5 succeeded at 1017 (offset 27 lines).
  Hunk #6 succeeded at 1041 (offset 27 lines).
  2 out of 6 hunks FAILED -- saving rejects to file
chrome/browser/instant/instant_controller.cc.rej

Patch:       chrome/browser/instant/instant_controller.cc
Index: chrome/browser/instant/instant_controller.cc
diff --git a/chrome/browser/instant/instant_controller.cc
b/chrome/browser/instant/instant_controller.cc
index
d3e654553b22918bb55bde6dfd1710d6fe874a4d..05923dafc53a2ea82fa2e381f2afbda38cc552a3
100644
--- a/chrome/browser/instant/instant_controller.cc
+++ b/chrome/browser/instant/instant_controller.cc
@@ -157,6 +157,8 @@
InstantController::InstantController(chrome::BrowserInstantController* browser,
       last_transition_type_(content::PAGE_TRANSITION_LINK),
       last_match_was_search_(false),
       is_omnibox_focused_(false),
+      start_margin_(0),
+      end_margin_(0),
       allow_preview_to_show_search_suggestions_(false) {
 }
 
@@ -350,24 +352,36 @@ bool InstantController::Update(const AutocompleteMatch&
match,
 
 // TODO(tonyg): This method only fires when the omnibox bounds change. It also
 // needs to fire when the preview bounds change (e.g.: open/close info bar).
-void InstantController::SetOmniboxBounds(const gfx::Rect& bounds) {
+void InstantController::SetPopupBounds(const gfx::Rect& bounds) {
   if (!extended_enabled_ && !instant_enabled_)
     return;
 
-  if (omnibox_bounds_ == bounds)
+  if (popup_bounds_ == bounds)
     return;
 
-  omnibox_bounds_ = bounds;
-  if (omnibox_bounds_.height() > last_omnibox_bounds_.height()) {
+  popup_bounds_ = bounds;
+  if (popup_bounds_.height() > last_popup_bounds_.height()) {
     update_bounds_timer_.Stop();
-    SendBoundsToPage();
+    SendPopupBoundsToPage();
   } else if (!update_bounds_timer_.IsRunning()) {
     update_bounds_timer_.Start(FROM_HERE,
         base::TimeDelta::FromMilliseconds(kUpdateBoundsDelayMS), this,
-        &InstantController::SendBoundsToPage);
+        &InstantController::SendPopupBoundsToPage);
   }
 }
 
+void InstantController::SetMarginSize(int start, int end) {
+  if (!extended_enabled_ || (start_margin_ == start && end_margin_ == end))
+    return;
+
+  start_margin_ = start;
+  end_margin_ = end;
+  if (loader_)
+    loader_->SetMarginSize(start_margin_, end_margin_);
+  if (instant_tab_)
+    instant_tab_->SetMarginSize(start_margin_, end_margin_);
+}
+
 void InstantController::HandleAutocompleteResults(
     const std::vector<AutocompleteProvider*>& providers) {
   if (!extended_enabled_)
@@ -828,6 +842,7 @@ bool InstantController::ResetLoader(const TemplateURL*
template_url,
     browser_->UpdateThemeInfoForPreview();
     loader_->SetDisplayInstantResults(instant_enabled_);
     loader_->SearchModeChanged(search_mode_);
+    loader_->SetMarginSize(start_margin_, end_margin_);
   }
 
   // Restart the stale loader timer.
@@ -869,6 +884,7 @@ void InstantController::ResetInstantTab() {
       instant_tab_.reset(new InstantTab(this, active_tab));
       instant_tab_->Init();
       instant_tab_->SetDisplayInstantResults(instant_enabled_);
+      instant_tab_->SetMarginSize(start_margin_, end_margin_);
     }
 
     // Hide the |loader_| since we are now using |instant_tab_| instead.
@@ -977,14 +993,14 @@ void InstantController::ShowLoader(InstantShownReason
reason,
     model_.SetPreviewState(search_mode_, 100, INSTANT_SIZE_PERCENT);
 }
 
-void InstantController::SendBoundsToPage() {
-  if (last_omnibox_bounds_ == omnibox_bounds_ || !loader_ ||
+void InstantController::SendPopupBoundsToPage() {
+  if (last_popup_bounds_ == popup_bounds_ || !loader_ ||
       loader_->is_pointer_down_from_activate())
     return;
 
-  last_omnibox_bounds_ = omnibox_bounds_;
+  last_popup_bounds_ = popup_bounds_;
   gfx::Rect preview_bounds = browser_->GetInstantBounds();
-  gfx::Rect intersection = gfx::IntersectRects(omnibox_bounds_,
preview_bounds);
+  gfx::Rect intersection = gfx::IntersectRects(popup_bounds_, preview_bounds);
 
   // Translate into window coordinates.
   if (!intersection.IsEmpty()) {
@@ -1001,7 +1017,7 @@ void InstantController::SendBoundsToPage() {
   DCHECK_LE(0, intersection.width());
   DCHECK_LE(0, intersection.height());
 
-  loader_->SetOmniboxBounds(intersection);
+  loader_->SetPopupBounds(intersection);
 }
 
 bool InstantController::GetInstantURL(const TemplateURL* template_url,

[commit-bot: I haz the power, 2012-12-13 02:10:08]

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

[commit-bot: I haz the power, 2012-12-13 04:30:41]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-13 20:01:04]

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

[commit-bot: I haz the power, 2012-12-13 21:32:49]

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

[commit-bot: I haz the power, 2012-12-14 01:11:07]

Change committed as 173028