alternate ntp: fix content origin when overlay shrinks to shorter than top bars

alternate ntp: fix content origin when overlay shrinks to shorter than top bars

BUG=229897
TEST=verify per bug rpt

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

[dhollowa, 2013-04-11 21:56:34]

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:174: // If overlay's height
has shrunk and |active_top_margin_| was used to
If |overlay_|'s height...

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:176: // determine if its
origin still needs to be preserved (which only happens if
...preserved.  This only happens... .  In this case force a re-layout of
BrowserView.

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:216:
browser_view_->Layout();
Doesn't browser_view_ contain |this| as a view?  So, shouldn't it be:

  if (layout_browser_view)
    browser_view_->Layout();
  else
    Layout();

To avoid a double-layout of |this|?

[kuan, 2013-04-11 23:39:19]

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

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:174: // If overlay's height
has shrunk and |active_top_margin_| was used to
On 2013/04/11 21:56:34, dhollowa wrote:
> If |overlay_|'s height...

Done.

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:176: // determine if its
origin still needs to be preserved (which only happens if
On 2013/04/11 21:56:34, dhollowa wrote:
> ...preserved.  This only happens... .  In this case force a re-layout of
> BrowserView.

Done. i slightly modify "This only happens" to "The origin is only preserved if
..".

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:216:
browser_view_->Layout();
On 2013/04/11 21:56:34, dhollowa wrote:
> Doesn't browser_view_ contain |this| as a view?  So, shouldn't it be:
> 
>   if (layout_browser_view)
>     browser_view_->Layout();
>   else
>     Layout();
> 
> To avoid a double-layout of |this|?

the overlay needs to be laid out before BrowserViewLayout::Layout is called,
'cos the latter retrieves the overlay height before it lays out the children
(see beginning of function in browser_view_layout.cc).

[kuan, 2013-04-11 23:45:23]

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13974003/diff/1/chrome/browser/ui/views/frame...
chrome/browser/ui/views/frame/contents_container.cc:216:
browser_view_->Layout();
On 2013/04/11 23:39:20, kuan wrote:
> On 2013/04/11 21:56:34, dhollowa wrote:
> > Doesn't browser_view_ contain |this| as a view?  So, shouldn't it be:
> > 
> >   if (layout_browser_view)
> >     browser_view_->Layout();
> >   else
> >     Layout();
> > 
> > To avoid a double-layout of |this|?
> 
> the overlay needs to be laid out before BrowserViewLayout::Layout is called,
> 'cos the latter retrieves the overlay height before it lays out the children
> (see beginning of function in browser_view_layout.cc).

hm.. let me try to change BrowserViewLayout::Layout to see if i can get the
overlay height later.

[kuan, 2013-04-12 00:56:29]

i've fixed the double layout of ContentsContainer in patch set 3.  ptal.  thx.

[dhollowa, 2013-04-12 17:30:03]

LGTM.

[sky, 2013-04-15 23:39:57]

LGTM - I'm adding James who has been studying this code recently.

https://codereview.chromium.org/13974003/diff/11001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.h (right):

https://codereview.chromium.org/13974003/diff/11001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.h:35: explicit
ContentsContainer(views::WebView* active,
no explicit.

[kuan, 2013-04-15 23:45:38]

i've addressed scott's comment in patch set 3, which includes rebase.

https://codereview.chromium.org/13974003/diff/11001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.h (right):

https://codereview.chromium.org/13974003/diff/11001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.h:35: explicit
ContentsContainer(views::WebView* active,
On 2013/04/15 23:39:57, sky wrote:
> no explicit.

Done.

[James Cook, 2013-04-16 00:00:27]

lgtm

[commit-bot: I haz the power, 2013-04-18 00:11:06]

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

[commit-bot: I haz the power, 2013-04-18 04:41:54]

Change committed as 194761