alternate ntp: fix website page jankiness when suggestions show and top bars are hidden

alternate ntp: fix jankiness when suggestions show on website page with bars

when suggestions show, pinned bookmark bar and infobars would hide, causing contents to shift up.
if ESC is hit to cancel the query, the bars would then appear, causing contents to shift down.
all these cause jankiness.

this cl is the first in the line to fix such jankiness:
- tackle website pages where suggestions appear in a preview overlay
- modify infobar container to delay hiding its bars until instant preview is ready, instead of when user starts typing
- during layout, if preview is taller than all hidden bars, preserve the position that contents previously had with the visible bars by setting a top margin in contents container

later cls will tackle ntp and serp pages where suggestions appear in the contents, so showing and hiding of bars require synchronization with renderer's searchbox api.

BUG=171075
TEST=verify per bug rpt

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

[kuan, 2013-01-25 20:10:32]

ping...

[dhollowa, 2013-01-25 21:24:08]

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:42: //   synchronization with
renderer and will be implemented as the next step;
nit: this sounds like a TODO and should be marked as such.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:48: // infobars only to instantly
animate them closed.  The window is canceled if a
nit: "The *animation* is canceled..." ?

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:74: chrome::search::SearchModel*
search_model,
The const for InstantModel is nice.  Could you please add mutable to the
SearchModel observer list and const to the Add/RemoveObservers.

This will provide symmetry here.  And at the risk of going const-crazy... 
Maybe?:

const SearchModel* const search_model_;
const InstantModel* const instant_model_;

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
File chrome/browser/ui/gtk/infobars/infobar_container_gtk.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/infobar_container_gtk.h:41:
chrome::search::SearchModel* search_model,
const chrome::search::SearchModel* search_model,

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/infobar_container_gtk.h:42: InstantModel*
instant_model,
const InstantModel* instant_model,

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/contents_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.h:59: int preview_height()
const;
nit: since this is snake case (trivial) better to inline the implementation here
in the header?

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/infobar_container_view.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/infobar_container_view.h:18:
chrome::search::SearchModel* search_model,
const

[Peter Kasting, 2013-01-27 23:54:26]

When you list multiple reviewers, you always need to say explicitly what you
want each to review.

[Peter Kasting, 2013-01-27 23:57:27]

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:74: chrome::search::SearchModel*
search_model,
On 2013/01/25 21:24:08, dhollowa wrote:
> The const for InstantModel is nice.  Could you please add mutable to the
> SearchModel observer list and const to the Add/RemoveObservers.

It does not seem at all correct that you should be able to add and remove
observers on a pointer-to-const.  That is not an internal implementation detail
that should be mutable.

If the InstantModel allows this, I claim that's a bug and the API should be
fixed.

[kuan, 2013-01-28 15:03:50]

peter, cld u pls review files in chrome/browser/infobars and
chrome/browser/ui/views?  thx.

[dhollowa, 2013-01-28 16:43:06]

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:74: chrome::search::SearchModel*
search_model,
On 2013/01/27 23:57:28, Peter Kasting wrote:
> On 2013/01/25 21:24:08, dhollowa wrote:
> > The const for InstantModel is nice.  Could you please add mutable to the
> > SearchModel observer list and const to the Add/RemoveObservers.
> 
> It does not seem at all correct that you should be able to add and remove
> observers on a pointer-to-const.  That is not an internal implementation
detail
> that should be mutable.
> 
> If the InstantModel allows this, I claim that's a bug and the API should be
> fixed.

That's a design choice really.  What I like about making the observer list
mutable is that it allows situations like this where view classes can take a
const-model to observe changes, but are required to go through a controller
class to change the (real) model itself.  Only controller classes then, should
have a non-const-model ptr.

That said, I'm happy to leave this change out of this CL, especially if there's
controversy.

[Peter Kasting, 2013-01-28 21:28:28]

On 2013/01/28 16:43:06, dhollowa wrote:
> What I like about making the observer list
> mutable is that it allows situations like this where view classes can take a
> const-model to observe changes, but are required to go through a controller
> class to change the (real) model itself.  Only controller classes then, should
> have a non-const-model ptr.

My perspective on this is as someone having spent months fixing nasty
const-correctness issue in TemplateURL (etc.).  We were using const in something
like the way you describe -- to control who could perform certain kinds of
changes, as opposed to simply using it to directly indicate whether some
operation changed the observable state of the object.

This turned out to be a mistake or a number of reasons, but one of the simplest
ones was merely that it was surprising.  A reader seeing a const object expects
it to mean that the object's observable state can't be changed.  Using it to
mean something else generally leads to mass confusion over time, even if it does
have nice properties for certain APIs.

In our case, we ultimately preserved the API properties we wanted by making most
functions on TemplateURL be private, and then having a single class as a friend
that could perform those operations.  This ultimately made the code much saner.

I suspect there are ways you could do something similar to preserve the kinds of
access guarantees you want without changing the meaning of const.  Even if there
aren't, I would still ultimately consider it a bug -- despite the side benefits
-- for InstantModel to have a mutable observer list long term.

[dhollowa, 2013-01-28 21:34:31]

On 2013/01/28 21:28:28, Peter Kasting wrote:
> On 2013/01/28 16:43:06, dhollowa wrote:
> > What I like about making the observer list
> > mutable is that it allows situations like this where view classes can take a
> > const-model to observe changes, but are required to go through a controller
> > class to change the (real) model itself.  Only controller classes then,
should
> > have a non-const-model ptr.
> 
> My perspective on this is as someone having spent months fixing nasty
> const-correctness issue in TemplateURL (etc.).  We were using const in
something
> like the way you describe -- to control who could perform certain kinds of
> changes, as opposed to simply using it to directly indicate whether some
> operation changed the observable state of the object.
> 
> This turned out to be a mistake or a number of reasons, but one of the
simplest
> ones was merely that it was surprising.  A reader seeing a const object
expects
> it to mean that the object's observable state can't be changed.  Using it to
> mean something else generally leads to mass confusion over time, even if it
does
> have nice properties for certain APIs.
> 
> In our case, we ultimately preserved the API properties we wanted by making
most
> functions on TemplateURL be private, and then having a single class as a
friend
> that could perform those operations.  This ultimately made the code much
saner.
> 
> I suspect there are ways you could do something similar to preserve the kinds
of
> access guarantees you want without changing the meaning of const.  Even if
there
> aren't, I would still ultimately consider it a bug -- despite the side
benefits
> -- for InstantModel to have a mutable observer list long term.

What you're saying makes sense.  I appreciate the anecdotes / experiential
lessons with TemplateURL.  I'll take it as an action to bring the InstantModel
back into the fold.

[Peter Kasting, 2013-01-28 21:51:00]

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:40:
instant_model_->AddObserver(this);
Please pass in a non-const pointer if you're going to call this, and file a bug
to fix InstantModel to not allow Add/RemoveObserver() calls on a const object. 
See latest reply to David as to why I'm asking for this.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:176: // PreviewStateChanged().
Nit: Add commentary on why you do this (i.e. what the effects of doing it/not
doing it are).

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:39: // It hides infobars temporarily
if the user changes into suggestions mode when:
Readers may not know what "suggestions mode" means.  Refer them to other places
that fill out the terms you use here.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:41: // - not on a website page:
immediately; this scenario requires more complex
It's not clear what "not on a website page" means.  In some sense, all content
Chrome shows in the content area is from a website.  chrome:// pages are still
websites.  Could you use clearer language?

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:42: //   synchronization with
renderer and will be implemented as the next step;
On 2013/01/25 21:24:08, dhollowa wrote:
> nit: this sounds like a TODO and should be marked as such.

Yes, please don't discuss implementation plans in comments except as TODOs. 
Comments should describe what the code does now.

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

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:276: // infobars without
the overlap cannot be determined.
This comment confused the heck out of me.

How about something like this, assuming I understand the problem (which I'm not
sure I do):

Showing instant extended suggestions causes us to temporarily hide any visible
bookmark bar and infobars.  In turn, this hiding would normally cause the
content below the suggestions to shift upwards, which looks surprising (since
from the user's perspective, we're "covering" rather than "removing" the
bookmark bar/infobars).  To prevent this, we save off the content origin here,
then once we finish laying things out, force the content to continue to display
from that origin.

Note that this avoids talking about alternative solutions (which is just
confusing).

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:339: //   hence the speical
handling of the 0 preview height case.
Again, all these comments are really confusing.  Don't assume readers know what
previews are, how instant works, or even what the "active top margin is" (which
I tried to look up and found like no documentation on, awesome).  Try and write
succinctly but for a reader who doesn't have any particular domain knowledge.  I
can't suggest an alternate set of comments in here because to be honest I simply
can't understand what any of this is doing well enough.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/contents_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.h:59: int preview_height()
const;
On 2013/01/25 21:24:08, dhollowa wrote:
> nit: since this is snake case (trivial) better to inline the implementation
here
> in the header?

Anything named unix_hacker()-style should be inlined.  Anything not inlined
should be named in CamelCase.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/infobar_container_view.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/infobar_container_view.h:18:
chrome::search::SearchModel* search_model,
On 2013/01/25 21:24:08, dhollowa wrote:
> const

The opposite (make |instant_model| non-const)

[kuan, 2013-01-29 00:17:08]

i've addressed all comments in patch set 3.  regarding the const InstantModel,
dhollowa@ will modify that in his followup cl, so i've kept the const here, so
that it would compile.

ptal.  thx.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.cc (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:40:
instant_model_->AddObserver(this);
On 2013/01/28 21:51:00, Peter Kasting wrote:
> Please pass in a non-const pointer if you're going to call this, and file a
bug
> to fix InstantModel to not allow Add/RemoveObserver() calls on a const object.

> See latest reply to David as to why I'm asking for this.

i was forced to use const InstantModel 'cos InstantController::model() returns
it as a const.  per the discussion between dhollowa@ and u, dhollowa@ will
modify the constness of InstantModel in a followup cl, at which time, he would
then remove the const here.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.cc:176: // PreviewStateChanged().
On 2013/01/28 21:51:00, Peter Kasting wrote:
> Nit: Add commentary on why you do this (i.e. what the effects of doing it/not
> doing it are).

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
File chrome/browser/infobars/infobar_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:39: // It hides infobars temporarily
if the user changes into suggestions mode when:
On 2013/01/28 21:51:00, Peter Kasting wrote:
> Readers may not know what "suggestions mode" means.  Refer them to other
places
> that fill out the terms you use here.

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:41: // - not on a website page:
immediately; this scenario requires more complex
On 2013/01/28 21:51:00, Peter Kasting wrote:
> It's not clear what "not on a website page" means.  In some sense, all content
> Chrome shows in the content area is from a website.  chrome:// pages are still
> websites.  Could you use clearer language?

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:42: //   synchronization with
renderer and will be implemented as the next step;
On 2013/01/25 21:24:08, dhollowa wrote:
> nit: this sounds like a TODO and should be marked as such.

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:42: //   synchronization with
renderer and will be implemented as the next step;
On 2013/01/28 21:51:00, Peter Kasting wrote:
> On 2013/01/25 21:24:08, dhollowa wrote:
> > nit: this sounds like a TODO and should be marked as such.
> 
> Yes, please don't discuss implementation plans in comments except as TODOs. 
> Comments should describe what the code does now.

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/infobars/in...
chrome/browser/infobars/infobar_container.h:48: // infobars only to instantly
animate them closed.  The window is canceled if a
On 2013/01/25 21:24:08, dhollowa wrote:
> nit: "The *animation* is canceled..." ?

i didn't write this comment, but hopefully i've clarified it.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
File chrome/browser/ui/gtk/infobars/infobar_container_gtk.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/infobar_container_gtk.h:41:
chrome::search::SearchModel* search_model,
On 2013/01/25 21:24:08, dhollowa wrote:
> const chrome::search::SearchModel* search_model,

per discussion, this won't be a const.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/gtk/info...
chrome/browser/ui/gtk/infobars/infobar_container_gtk.h:42: InstantModel*
instant_model,
On 2013/01/25 21:24:08, dhollowa wrote:
> const InstantModel* instant_model,

per discussion, this shouldn't be a const, but for now, i've made it a const to
compile.  dhollowa@ would fix it in his follow-up cl.

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

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:276: // infobars without
the overlap cannot be determined.
On 2013/01/28 21:51:00, Peter Kasting wrote:
> This comment confused the heck out of me.
> 
> How about something like this, assuming I understand the problem (which I'm
not
> sure I do):
> 
> Showing instant extended suggestions causes us to temporarily hide any visible
> bookmark bar and infobars.  In turn, this hiding would normally cause the
> content below the suggestions to shift upwards, which looks surprising (since
> from the user's perspective, we're "covering" rather than "removing" the
> bookmark bar/infobars).  To prevent this, we save off the content origin here,
> then once we finish laying things out, force the content to continue to
display
> from that origin.
> 
> Note that this avoids talking about alternative solutions (which is just
> confusing).

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/browser_view_layout.cc:339: //   hence the speical
handling of the 0 preview height case.
On 2013/01/28 21:51:00, Peter Kasting wrote:
> Again, all these comments are really confusing.  Don't assume readers know
what
> previews are, how instant works, or even what the "active top margin is"
(which
> I tried to look up and found like no documentation on, awesome).  Try and
write
> succinctly but for a reader who doesn't have any particular domain knowledge. 
I
> can't suggest an alternate set of comments in here because to be honest I
simply
> can't understand what any of this is doing well enough.

Done.  i apologize my attempt to explain everything in detail backfired; i've
shortened the comments, and added comments for SetActiveTopMargin in
contents_container.h.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
File chrome/browser/ui/views/frame/contents_container.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.h:59: int preview_height()
const;
On 2013/01/25 21:24:08, dhollowa wrote:
> nit: since this is snake case (trivial) better to inline the implementation
here
> in the header?

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/fr...
chrome/browser/ui/views/frame/contents_container.h:59: int preview_height()
const;
On 2013/01/28 21:51:00, Peter Kasting wrote:
> On 2013/01/25 21:24:08, dhollowa wrote:
> > nit: since this is snake case (trivial) better to inline the implementation
> here
> > in the header?
> 
> Anything named unix_hacker()-style should be inlined.  Anything not inlined
> should be named in CamelCase.

Done.

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
File chrome/browser/ui/views/infobars/infobar_container_view.h (right):

https://codereview.chromium.org/12036075/diff/3001/chrome/browser/ui/views/in...
chrome/browser/ui/views/infobars/infobar_container_view.h:18:
chrome::search::SearchModel* search_model,
On 2013/01/28 21:51:00, Peter Kasting wrote:
> On 2013/01/25 21:24:08, dhollowa wrote:
> > const
> 
> The opposite (make |instant_model| non-const)

dhollowa@ will fix the constness in InstantModel and change this to non-const in
his follow-up cl.  for now, InstantController::model() returns it as const and i
need it as const to compile.

[Peter Kasting, 2013-01-29 00:36:29]

LGTM

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

https://codereview.chromium.org/12036075/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:310: // shift up due to
hiding of bookmark and info bars.
Nit: How about just:

// Now set the contents to display at their previous origin if we just hid the
bookmark and/or infobars.

https://codereview.chromium.org/12036075/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:320: // - suggestions
completely cover the bookmark bar/infobars that are hidden.
Nit: This confused me for a while.  See if this explanation is accurate instead:

// Special case: While normally the suggestions appear to "cover" any
bookmark/infobars, if the suggestions are very short, they might not fully cover
that gap, and leaving the contents at their original height would leave an
odd-looking blank space.  In this case, we allow the contents to go ahead and
shift upward.  (We don't shift up in the zero-preview-height case since that
corresponds to when the suggestions aren't actually ready to be displayed yet.)

This also makes me wonder: Do you really need to check for a zero preview height
here, if you ensure that we don't hide the bookmark bar/infobars until the
preview is ready?

[kuan, 2013-01-29 01:00:11]

i've addressed pete's comments in patch set 4.  ptal.  thx.

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

https://codereview.chromium.org/12036075/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:310: // shift up due to
hiding of bookmark and info bars.
On 2013/01/29 00:36:29, Peter Kasting wrote:
> Nit: How about just:
> 
> // Now set the contents to display at their previous origin if we just hid the
> bookmark and/or infobars.

Done.

https://codereview.chromium.org/12036075/diff/14001/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:320: // - suggestions
completely cover the bookmark bar/infobars that are hidden.
On 2013/01/29 00:36:29, Peter Kasting wrote:
> Nit: This confused me for a while.  See if this explanation is accurate
instead:
> 
> // Special case: While normally the suggestions appear to "cover" any
> bookmark/infobars, if the suggestions are very short, they might not fully
cover
> that gap, and leaving the contents at their original height would leave an
> odd-looking blank space.  In this case, we allow the contents to go ahead and
> shift upward.  (We don't shift up in the zero-preview-height case since that
> corresponds to when the suggestions aren't actually ready to be displayed
yet.)
> 
> This also makes me wonder: Do you really need to check for a zero preview
height
> here, if you ensure that we don't hide the bookmark bar/infobars until the
> preview is ready?

Done.

The zero-preview-height case was what i tried, but failed, to explain in my
previous comments.

Layout() is triggered separately when bookmark bar and infobars are hidden.
Bookmark bar triggers Layout() from Browser class after ContentsContainer has
setup the preview for suggestions.
Infobar container triggers Layout() from itself, regardless if ContentsContainer
has setup the preview.  In this case, preview height cld be 0 in Layout() if
ContentsContainer hasn't setup the preview yet.

i slightly modified the last sentence of your comments to hopefully explain
this.

[Peter Kasting, 2013-01-29 01:03:29]

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

https://codereview.chromium.org/12036075/diff/16004/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:322: // haven't been
handled by |contents_container_| yet.
Nit: Needs trailing paren

What does "haven't been handled yet" mean?

Maybe this could be something like:

(In the zero-preview-height case the suggestions aren't actually ready yet, so
we leave the old contents origin in place until we're called back later with a
nonzero height.)

[kuan, 2013-01-29 01:09:17]

i've modified the comment per peter's suggestion in patch set 5.

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

https://codereview.chromium.org/12036075/diff/16004/chrome/browser/ui/views/f...
chrome/browser/ui/views/frame/browser_view_layout.cc:322: // haven't been
handled by |contents_container_| yet.
On 2013/01/29 01:03:29, Peter Kasting wrote:
> Nit: Needs trailing paren
> 
> What does "haven't been handled yet" mean?
> 
> Maybe this could be something like:
> 
> (In the zero-preview-height case the suggestions aren't actually ready yet, so
> we leave the old contents origin in place until we're called back later with a
> nonzero height.)

Done.

[dhollowa, 2013-01-29 01:14:09]

LGTM.  Thanks.

[commit-bot: I haz the power, 2013-01-29 01:25:09]

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

[commit-bot: I haz the power, 2013-01-29 01:44:22]

Failed to trigger a try job on linux_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-29 01:44:32]

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

[commit-bot: I haz the power, 2013-01-29 02:31:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...
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.

[kuan, 2013-01-30 13:33:51]

patch set 6, which was lgtm'ed, broke (crashed) browser_tests and
interactive_ui_tests.  the crash was caused by InfoBarContainer trying to remove
itself as observer from InstantModel in its destructor, but InstantModel had
already been freed by Browser in its TabStripEmpty(); IntantModel is owned by
Browser via InstantController via BrowserInstantController.

in patch set 7, InfobarContainer no longer observes changes to InstantModel
directly; it gets the notification via InstantPreviewControllerView, like for
bookmark bar.  this also simplifies the layout logic, in that preview is always
ready when the contents position needs to be preserved.

peter and david, i apologize for wasting your time reviewing the previous
design., which didn't crash when i ran chrome and the tests locally :(
this time, i've run tryjobs on the latest patch and they've passed; the errors
don't seem to be caused by my cl.  even though the design has changed, the
difference code-wise is not that big, i think.  ptal.  thx.

[dhollowa, 2013-01-30 17:40:42]

Yes, LGTM.  The InstantModel's lifetime is tied to the BrowerInstantController
and InstantController, hence the need for your change.  It seems less than ideal
that the BIC and IC can be deleted like this.  I'll chat with Sreeram about
possibly changing it.  Thanks Kuan.

[commit-bot: I haz the power, 2013-02-01 13:26:56]

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

[commit-bot: I haz the power, 2013-02-01 17:46:36]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-02-01 17:47:49]

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

[commit-bot: I haz the power, 2013-02-01 22:55:46]

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

[commit-bot: I haz the power, 2013-02-01 23:01:25]

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

[commit-bot: I haz the power, 2013-02-02 02:34:19]

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

[commit-bot: I haz the power, 2013-02-02 13:33:05]

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

[commit-bot: I haz the power, 2013-02-02 16:44:35]

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

[commit-bot: I haz the power, 2013-02-02 17:00:23]

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

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

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

[commit-bot: I haz the power, 2013-02-02 20:28:30]

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

[commit-bot: I haz the power, 2013-02-02 23:55:53]

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

[commit-bot: I haz the power, 2013-02-04 14:23:04]

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

[commit-bot: I haz the power, 2013-02-04 17:16:39]

Change committed as 180439