alternate ntp: clip middle omnibox when resizing browser window

alternate ntp: clip middle omnibox when resizing browser window

- when resizing browser window, middle omnibox wasn't clipped; it would float in the root window if the browser window is too small to contain it
- fix to make LocationBarContainer clip its child LocationBarView
- LocationBarContainer's bounds is resized to within its parent's, while LocationBarView's width fills it but height stays the same, allowing the former to clip the latter without resizing the latter

BUG=145744
TEST=verify per bug rpt.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156675

[kuan, 2012-09-12 18:10:07]

ptal at patch set 3.

scott, fyi, browser window can't clip its children, else we lose its 3d shadows,
so i make LocationBarContainer does the job instead.

[sky, 2012-09-12 21:07:31]

I don't understand this. Why do we need the LBC bounds to differ from that of
the LBV?

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_container.cc (right):

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_container.cc:61: void
LocationBarContainer::SetBoundsForContainerAndView(
Why doesn't SetBounds do what you want here?

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_container_aura.cc
(right):

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_container_aura.cc:16:
layer()->SetMasksToBounds(true);
Add a comment why you need this.

[kuan, 2012-09-12 21:33:04]

LBC is resized to within browser window.  if LBV also has the same size, its
placeholder text gets squished in a vertically-small box when the browser
window's bottom edge gets to above the bottom edge of the omnibox.  i've always
assumed that elements on the page should be clipped, not resized.

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_container.cc (right):

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_container.cc:61: void
LocationBarContainer::SetBoundsForContainerAndView(
On 2012/09/12 21:07:31, sky wrote:
> Why doesn't SetBounds do what you want here?

u're right, i don't need to SetBounds for location_bar_view_.  however, i still
need to provide this method to store the size of location_bar_view so that i can
ue it in Layout.

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_container_aura.cc
(right):

http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_container_aura.cc:16:
layer()->SetMasksToBounds(true);
On 2012/09/12 21:07:31, sky wrote:
> Add a comment why you need this.

Done.

[sky, 2012-09-12 23:16:13]

Can we always give LBV its preferred height then?

On Wed, Sep 12, 2012 at 2:33 PM,  <kuan@chromium.org> wrote:
> LBC is resized to within browser window.  if LBV also has the same size, its
> placeholder text gets squished in a vertically-small box when the browser
> window's bottom edge gets to above the bottom edge of the omnibox.  i've
> always
> assumed that elements on the page should be clipped, not resized.
>
>
>
>
http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/location_bar_container.cc
> (right):
>
>
http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/location_bar_container.cc:61: void
> LocationBarContainer::SetBoundsForContainerAndView(
> On 2012/09/12 21:07:31, sky wrote:
>>
>> Why doesn't SetBounds do what you want here?
>
>
> u're right, i don't need to SetBounds for location_bar_view_.  however,
> i still need to provide this method to store the size of
> location_bar_view so that i can ue it in Layout.
>
>
>
http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
> File chrome/browser/ui/views/location_bar/location_bar_container_aura.cc
> (right):
>
>
http://codereview.chromium.org/10907038/diff/2002/chrome/browser/ui/views/loc...
> chrome/browser/ui/views/location_bar/location_bar_container_aura.cc:16:
> layer()->SetMasksToBounds(true);
> On 2012/09/12 21:07:31, sky wrote:
>>
>> Add a comment why you need this.
>
>
> Done.
>
> http://codereview.chromium.org/10907038/

[kuan, 2012-09-13 18:05:16]

scott, u're right!  LBV can always retain its preferred height.  the result is a
much cleaner implementation in patch set 6.

ptal.  thx!

[dhollowa, 2012-09-13 18:24:08]

Thanks for the simplifications...  LGTM w/ nit.

http://codereview.chromium.org/10907038/diff/9003/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/10907038/diff/9003/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:436: if (intersect_rect.IsEmpty()) {
if (interset_rect.IsEmpty())
  intersect_rect = gfx::Rect();

[kuan, 2012-09-13 19:10:29]

i've addressed david's comments in patch set 6.  ptal.  thx.

http://codereview.chromium.org/10907038/diff/9003/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/10907038/diff/9003/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar_view.cc:436: if (intersect_rect.IsEmpty()) {
On 2012/09/13 18:24:08, dhollowa wrote:
> if (interset_rect.IsEmpty())
>   intersect_rect = gfx::Rect();

Done.  i shortened it even more in SetBoundsRect.

[sky, 2012-09-13 19:58:23]

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_container.cc (right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_container.cc:11: #include
"ui/views/layout/fill_layout.h"
remove include

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_container_aura.cc
(right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_container_aura.cc:16: //
Allows clipping of children.
This comment duplicates what the code is doing. Instead say why we need this.
Maybe something like: the LocationBarView may be taller than us (otherwise the
text gets squished in weird ways). To allow the LocationBarView to be taller yet
clipped to our bounds we explicitly turn on clipping.

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/to...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/to...
chrome/browser/ui/views/toolbar_view.cc:437: intersect_rect.IsEmpty() ?
gfx::Rect() : intersect_rect);
Why do you need the empty test? If empty, then isn't intersect_rect the same as
gfx::Rect?

[kuan, 2012-09-13 20:13:28]

i've addressed all comments in patch set 8.  ptal.  thx.

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_container.cc (right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_container.cc:11: #include
"ui/views/layout/fill_layout.h"
On 2012/09/13 19:58:24, sky wrote:
> remove include

Done.

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_container_aura.cc
(right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_container_aura.cc:16: //
Allows clipping of children.
On 2012/09/13 19:58:24, sky wrote:
> This comment duplicates what the code is doing. Instead say why we need this.
> Maybe something like: the LocationBarView may be taller than us (otherwise the
> text gets squished in weird ways). To allow the LocationBarView to be taller
yet
> clipped to our bounds we explicitly turn on clipping.

Done.

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/to...
File chrome/browser/ui/views/toolbar_view.cc (right):

http://codereview.chromium.org/10907038/diff/11003/chrome/browser/ui/views/to...
chrome/browser/ui/views/toolbar_view.cc:437: intersect_rect.IsEmpty() ?
gfx::Rect() : intersect_rect);
On 2012/09/13 19:58:24, sky wrote:
> Why do you need the empty test? If empty, then isn't intersect_rect the same
as
> gfx::Rect?

Done.  i thot Rect::Intersect may return negative values for x and y if its
width or height is 0, but i was wrong.

[sky, 2012-09-13 21:13:59]

LGTM

[commit-bot: I haz the power, 2012-09-13 21:16:19]

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

[commit-bot: I haz the power, 2012-09-13 23:37:29]

Change committed as 156675