cros: Instant extended support for immersive fullscreen

cros: Instant extended support for immersive fullscreen

In immersive fullscreen the "top container" holding the tabstrip and toolbar can slide down over the web content. Instant extended shows suggestions in an overlay web view, this must be adjusted to match the omnibox position when the top container is revealed. Instant extended also shows a suggestion+result page in the active web view, which must also be adjusted to align with the omnibox.

BUG=169414
TEST=Added to interactive_ui_tests BrowserViewTest and also manual.
On Chrome OS, enable the flags for instant extended and immersive fullscreen. Open a normal tab and one to the NTP. Test omnibox suggestions in both tabs, ensure they align with the omnibox. Hit F4 to go into fullscreen, do the same tests. Select "Show Bookmark Bar" and repeat both sets of tests.
TBR=sreeram@chromium.org for instant_extended.html changes

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

[James Cook, 2013-04-05 02:05:05]

kuan, PTAL. I pair-programmed this with gspencer, so I'm pretty sure the
modifications I made to the instant extended bookmark bar offset code are
functionally equivalent to the old version.

(BTW, I see a lot of janky loading and web content shifting when I type things
like "hello" into a Debug build of Chrome, but they're present both with and
without my patch and don't seem to get worse. For example, the Google logo
shifts position up and down by a pixel or two, but that seems to be in the
content itself.)

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:513:
contents_split_->SetBoundsRect(contents_split_bounds);
The diff is confusing - LayoutTabContents() contained a large amount of dead
code. The only thing it actually did was set the bounds of the contents split.

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:581:
BrowserViewLayout::InstantUIState BrowserViewLayout::GetInstantUIState() {
This is a near-copy of the Mac function currentInstantUIState().

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.h:70: enum InstantUIState {
This mirrors an enum used in the Mac implementation to deal with presentation
mode and active/overlay offsets.

[James Cook, 2013-04-05 02:05:33]

On 2013/04/05 02:05:05, James Cook (Chromium) wrote:
> kuan, PTAL. I pair-programmed this with gspencer, so I'm pretty sure the
> modifications I made to the instant extended bookmark bar offset code are
> functionally equivalent to the old version.
> 
> (BTW, I see a lot of janky loading and web content shifting when I type things
> like "hello" into a Debug build of Chrome, but they're present both with and
> without my patch and don't seem to get worse. For example, the Google logo
> shifts position up and down by a pixel or two, but that seems to be in the
> content itself.)
> 
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> File chrome/browser/ui/views/frame/browser_view_layout.cc (right):
> 
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> chrome/browser/ui/views/frame/browser_view_layout.cc:513:
> contents_split_->SetBoundsRect(contents_split_bounds);
> The diff is confusing - LayoutTabContents() contained a large amount of dead
> code. The only thing it actually did was set the bounds of the contents split.
> 
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> chrome/browser/ui/views/frame/browser_view_layout.cc:581:
> BrowserViewLayout::InstantUIState BrowserViewLayout::GetInstantUIState() {
> This is a near-copy of the Mac function currentInstantUIState().
> 
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> File chrome/browser/ui/views/frame/browser_view_layout.h (right):
> 
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> chrome/browser/ui/views/frame/browser_view_layout.h:70: enum InstantUIState {
> This mirrors an enum used in the Mac implementation to deal with presentation
> mode and active/overlay offsets.

Greg, just FYI. You've already seen all this code.

[kuan, 2013-04-05 16:49:22]

if u don't mind, i'll get to this once i resolve my stable blocker crash,
hopefully early next wk.

On 2013/04/05 02:05:33, James Cook (Chromium) wrote:
> On 2013/04/05 02:05:05, James Cook (Chromium) wrote:
> > kuan, PTAL. I pair-programmed this with gspencer, so I'm pretty sure the
> > modifications I made to the instant extended bookmark bar offset code are
> > functionally equivalent to the old version.
> > 
> > (BTW, I see a lot of janky loading and web content shifting when I type
things
> > like "hello" into a Debug build of Chrome, but they're present both with and
> > without my patch and don't seem to get worse. For example, the Google logo
> > shifts position up and down by a pixel or two, but that seems to be in the
> > content itself.)
> > 
> >
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> > File chrome/browser/ui/views/frame/browser_view_layout.cc (right):
> > 
> >
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> > chrome/browser/ui/views/frame/browser_view_layout.cc:513:
> > contents_split_->SetBoundsRect(contents_split_bounds);
> > The diff is confusing - LayoutTabContents() contained a large amount of dead
> > code. The only thing it actually did was set the bounds of the contents
split.
> > 
> >
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> > chrome/browser/ui/views/frame/browser_view_layout.cc:581:
> > BrowserViewLayout::InstantUIState BrowserViewLayout::GetInstantUIState() {
> > This is a near-copy of the Mac function currentInstantUIState().
> > 
> >
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> > File chrome/browser/ui/views/frame/browser_view_layout.h (right):
> > 
> >
>
https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
> > chrome/browser/ui/views/frame/browser_view_layout.h:70: enum InstantUIState
{
> > This mirrors an enum used in the Mac implementation to deal with
presentation
> > mode and active/overlay offsets.
> 
> Greg, just FYI. You've already seen all this code.

[kuan, 2013-04-08 15:56:30]

i applied your local patch and verified a few 1993-only scenarios; i've also
debugged what didn't work; pls see my comments below.

i've not verified beyond the 3 scenarios i mentioned, but figured i should send
this out so that you can proceed.  i'll continue to verify more scenarios, and
with immersive mode. and send my observations your way.

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:513:
contents_split_->SetBoundsRect(contents_split_bounds);
if SetBoundsRect changes bounds of contents_container_, it will re-layout its
children at the top margins u've set in the previous Layout().
this works if your previous margins were 0; @543, u'll then get the new origins
of the active and overlay webviews as if they were laid out afresh.
if however, u've set some margins in the previous Layout(), the webviews will be
laid out at the previous margins.  then @555, ur active_top_margin will be
incorrect, because it doesn't consider the margin used to layout the active
webview.
i think that's why the previous impl of calling SetActiveTopMargin twice worked
because the webviews were always re-laid out at the new margin, and we need to
know where the webviews will be without the spacer.

fyi, here are the scenarios that work:
a) if there's only bmb: at amazon.com, enable bmb, type a query that reproduces
a url as the first suggestion, u'll notice contents of amazon.com don't shift
b) if there's only infobar: at google.de, disable bmb, u shld have translate
infobar, type same query, u'll notice same results as (a).

here's the scenario that fails:
if there's bmb AND infobar: at google.de, enable bmb, u shld have translate
infobar, type same query, u'll notice contents of google.de shift up.

let me know if u need screenshots/video for better illustration.

[kuan, 2013-04-08 16:05:33]

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/7001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:513:
contents_split_->SetBoundsRect(contents_split_bounds);
On 2013/04/08 15:56:30, kuan wrote:
> if SetBoundsRect changes bounds of contents_container_, it will re-layout its
> children at the top margins u've set in the previous Layout().
> this works if your previous margins were 0; @543, u'll then get the new
origins
> of the active and overlay webviews as if they were laid out afresh.
> if however, u've set some margins in the previous Layout(), the webviews will
be
> laid out at the previous margins.  then @555, ur active_top_margin will be
> incorrect, because it doesn't consider the margin used to layout the active
> webview.
> i think that's why the previous impl of calling SetActiveTopMargin twice
worked
> because the webviews were always re-laid out at the new margin, and we need to
> know where the webviews will be without the spacer.
> 
> fyi, here are the scenarios that work:
> a) if there's only bmb: at http://amazon.com, enable bmb, type a query that
reproduces
> a url as the first suggestion, u'll notice contents of http://amazon.com don't
shift
> b) if there's only infobar: at google.de, disable bmb, u shld have translate
> infobar, type same query, u'll notice same results as (a).
> 
> here's the scenario that fails:
> if there's bmb AND infobar: at google.de, enable bmb, u shld have translate
> infobar, type same query, u'll notice contents of google.de shift up.
> 
> let me know if u need screenshots/video for better illustration.  

in the scenario that fails, Layout is called separately when bmb and infobar are
hidden; u set a margin when the 1st bar gets hidden, then u reuse that margin to
position active webview when the 2nd bar gets hidden.  this gives u an origin
that is already shifted down, but u don't consider that when u set a new margin.

[kuan, 2013-04-08 23:28:11]

https://codereview.chromium.org/13684002/diff/17001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/17001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:582: if
(!browser()->search_model()->mode().is_search_suggestions())
heads up: this has changed in r192680 to "is_search()", which is the correct
one.

[kuan, 2013-04-09 13:33:20]

i've run through the scenarios where immersive and 1993 co-exist; they seem
fine, except for the same scenario that broke with 1993-only.  i'll re-verify
the scenarios when ur new patch is ready.

[James Cook, 2013-04-12 21:06:56]

kuan, please take another look

I'm working on tests now, but I'd like to get feedback on the code.  I think it
works in all cases, and doesn't change behavior for non-immersive-mode.

[kuan, 2013-04-15 18:49:35]

i've run your patch through 1993-only and 1993+immersive scenarios.
the scenario with bookmark and info bars that previously broke works now.

however, i've encountered a scenario: when searching from a serp page with the
top container revealed, the 1st suggestion is obscured by the top container. 
i've been modifying the parameters to narrow down the repro scenario. 
unfortunately, because it doesn't always repro, plus a bug in today's chrome/gws
that the serp page is sometimes empty, i haven't gotten far enough to nail down
and debug the cause.  i did manage to capture one scenario (which i repro'ed
twice consecutively) in a video, which i'll attach shortly.

btw, i don't see the potential fix we discussed last wk for the top-left google
logo showing up at the wrong position before u set the margins.  u don't need it
anymore?  which part of the cl fixes that?

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:537: // content to be
offset.
hm. the sentence sounds weird, did u miss a "; this" b4 "requires"?

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.h:115: int
GetImmersiveInstantContentTopMargin();
maybe name it like the above GetTopMarginFor...?

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:215: // TODO(jamescook):
This can be done more efficiently with ReorderChildView().
i think not.  when porting 1993 to windows, mad@ had problem stacking overlay_
at top with Reorder, that's why he did this.

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:270: overlay_->SetBounds(0,
overlay_top_margin_, width(), overlay_height_pixels);
so u have problem making the height like content_height?
hm.. does making the height smaller result in the bottom gray rect in one of
those screenshots u emailed me?

[kuan, 2013-04-15 18:53:55]

video w/ broken scenario is at:
https://docs.google.com/a/google.com/file/d/0BxVj4w-Jk9vJUkZEa25WcUVRSEk/edit...

[kuan, 2013-04-16 14:27:16]

i may have figured out what's wrong.  pls try it out.  in the meantime, i'll do
more testing.

https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:342:
contents_container_->SetActiveTopMargin(active_top_margin);
see comment below for line 587 first, then this.

because u need to know |contents_container_|'s new bounds after
LayoutcontentsSplitView(), i've tried the following:
1) move lines 341 & 342 to before 347
2) after 347, call static_cast<views::View*>(contents_container_)->Layout().
(Layout() is private in ContentsContainer, so i had to downcast to views::View*
to call it).

this seems to work for the 2 broken scenarios i mentioned so far.  u can try it
out and test more (maybe even w/out calling ContentsContainer::Layout()), while
i'll do the same.

https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:587: &bottom_edge);
here, when u try to get the y-pos within |contents_container_|,
LayoutContentsSplitView hasn't been called yet, and hence u're using the
previous |contents_container_|'s bounds.  it won't be correct.

[kuan, 2013-04-16 14:49:08]

On 2013/04/16 14:27:16, kuan wrote:
> i may have figured out what's wrong.  pls try it out.  in the meantime, i'll
do
> more testing.
> 
>
https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
> File chrome/browser/ui/views/frame/browser_view_layout.cc (right):
> 
>
https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
> chrome/browser/ui/views/frame/browser_view_layout.cc:342:
> contents_container_->SetActiveTopMargin(active_top_margin);
> see comment below for line 587 first, then this.
> 
> because u need to know |contents_container_|'s new bounds after
> LayoutcontentsSplitView(), i've tried the following:
> 1) move lines 341 & 342 to before 347
> 2) after 347, call static_cast<views::View*>(contents_container_)->Layout().
> (Layout() is private in ContentsContainer, so i had to downcast to
views::View*
> to call it).
> 
> this seems to work for the 2 broken scenarios i mentioned so far.  u can try
it
> out and test more (maybe even w/out calling ContentsContainer::Layout()),
while
> i'll do the same.

hm.. u probably need to call ContentsContainer::Layout() so that the new bounds
of active web contents can be retreived later.

>
https://codereview.chromium.org/13684002/diff/45001/chrome/browser/ui/views/f...
> chrome/browser/ui/views/frame/browser_view_layout.cc:587: &bottom_edge);
> here, when u try to get the y-pos within |contents_container_|,
> LayoutContentsSplitView hasn't been called yet, and hence u're using the
> previous |contents_container_|'s bounds.  it won't be correct.

[James Cook, 2013-04-16 18:51:00]

kuan, PTAL. This has the implementation you suggested this morning, and cleanup
from the previous rounds of comments.

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.cc (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:537: // content to be
offset.
On 2013/04/15 18:49:36, kuan wrote:
> hm. the sentence sounds weird, did u miss a "; this" b4 "requires"?

Oops. Not sure how that sentence came out so strange. Rewritten.

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.h:115: int
GetImmersiveInstantContentTopMargin();
On 2013/04/15 18:49:36, kuan wrote:
> maybe name it like the above GetTopMarginFor...?

Done.

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:215: // TODO(jamescook):
This can be done more efficiently with ReorderChildView().
On 2013/04/15 18:49:36, kuan wrote:
> i think not.  when porting 1993 to windows, mad@ had problem stacking overlay_
> at top with Reorder, that's why he did this.

OK. Removed TODO.

https://codereview.chromium.org/13684002/diff/39001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:270: overlay_->SetBounds(0,
overlay_top_margin_, width(), overlay_height_pixels);
On 2013/04/15 18:49:36, kuan wrote:
> so u have problem making the height like content_height?
> hm.. does making the height smaller result in the bottom gray rect in one of
> those screenshots u emailed me?

I thought that clamping the overlay size was causing the gray rectangles, but
apparently not -- I can no longer reproduce them, and I saw them over the
weekend in a canary without my change.  Added back a height limit.

[James Cook, 2013-04-16 23:30:11]

Updated - I added one test for layout with immersive fullscreen + instant
extended. I think I know how to add another tomorrow.

[kuan, 2013-04-17 13:30:26]

i've tested ur patch #10 with multiple 1993-only and 1993+immersive scenarios;
they seem fine.  i hope i covered all scenarios, but if i miss anything, we can
deal w/ it after this cl.

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.h:94: // Layout the
WebContents container, between the coordinates |top| and
maybe update comment to mention contents split?  "WebContents container" sounds
like the contents_container_.

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:273: std::min(height() -
overlay_top_margin_, target_overlay_height);
i think u need to "std::max(0, overlay_height)" in case "height() -
overlay_top_margin" is negative.

[James Cook, 2013-04-17 17:22:58]

Comments addressed, Windows compile fixed, please take another look.

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/browser_view_layout.h (right):

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.h:94: // Layout the
WebContents container, between the coordinates |top| and
On 2013/04/17 13:30:26, kuan wrote:
> maybe update comment to mention contents split?  "WebContents container"
sounds
> like the contents_container_.

Done.

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
File chrome/browser/ui/views/frame/contents_container.cc (right):

https://codereview.chromium.org/13684002/diff/56001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/contents_container.cc:273: std::min(height() -
overlay_top_margin_, target_overlay_height);
On 2013/04/17 13:30:26, kuan wrote:
> i think u need to "std::max(0, overlay_height)" in case "height() -
> overlay_top_margin" is negative.

Good catch. Done.

[kuan, 2013-04-17 17:26:24]

lgtm.

[James Cook, 2013-04-17 21:47:44]

Committed patchset #14 manually as r194694 (presubmit successful).