Fix DCHECK() in infobar animation.

Fix DCHECK() in infobar animation.

The core of the problem here is that painting does not always happen synchronously with layout, and the old code was not always forcing re-layout to occur if the animation progressed in the meantime.  This change forces re-layout to occur any time the animation progresses or the target height is changed, and makes that the one time when the tab and bar heights are recalculated -- all other places then use the calculated values.

This also tweaks which functions are in the cross-platform versus platform-specific code as well as making a few other small cleanups.

Finally, this re-enables a test on Mac that seems to be passing now.

BUG=60990, 75451
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80272

[Peter Kasting, 2011-04-01 17:05:20]

This is my take on Sheridan's http://codereview.chromium.org/6675033/ .  It's
broadly similar, but makes some changes in precisely where the different pieces
of logic are triggered that I hope make it simpler and clearer.

[Sheridan Rawlins, 2011-04-01 18:41:48]

Does animation Reset need attention too?

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browse...

http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browse...

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_infobar_apitest.cc (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_infobar_apitest.cc:9: #if
defined(TOOLKIT_VIEWS) || defined(OS_MACOSX)
Curious: is TOOLKIT_VIEWS enabled on both linux_views and windows?

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar.h (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar.h:50: void OnSizeChanged();
private?

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar.h:55: virtual void
PlatformSpecificOnSizeChanged() {}
Move implementations to implementation file for this non-interface class.

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar_view.cc (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar_view.cc:368: InvalidateLayout();
InvalidateLayout only needs_layout, but doesn't cause Layout AFAIK.  Does this
need to notify container_?

[Peter Kasting, 2011-04-01 19:07:15]

I don't understand your animation Reset() question.  Reset() is only called in
InfoBar functions, which all do the right thing, and in ExtensionInfoBar, which
only does it when the height is 0 anyway (and besides which it immediately calls
SetTargetHeight()).

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_infobar_apitest.cc (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_infobar_apitest.cc:9: #if
defined(TOOLKIT_VIEWS) || defined(OS_MACOSX)
On 2011/04/01 18:41:48, Sheridan Rawlins wrote:
> Curious: is TOOLKIT_VIEWS enabled on both linux_views and windows?

Yes.

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar.h (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar.h:50: void OnSizeChanged();
On 2011/04/01 18:41:48, Sheridan Rawlins wrote:
> private?

No, because InfoBarView::SetTargetHeight() calls it.

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar.h:55: virtual void
PlatformSpecificOnSizeChanged() {}
On 2011/04/01 18:41:48, Sheridan Rawlins wrote:
> Move implementations to implementation file for this non-interface class.

That's not a style rule I can find.  Virtual functions should generally not be
inlined, but except for constructors/destructors, it's OK to do so for empty
bodies (since they truly don't generate any significant code), and doing so
makes it much clearer that by default these do nothing.

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar_view.cc (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar_view.cc:368: InvalidateLayout();
On 2011/04/01 18:41:48, Sheridan Rawlins wrote:
> InvalidateLayout only needs_layout, but doesn't cause Layout AFAIK.  Does this
> need to notify container_?

We will notify the container immediately upon return, see
InfoBar::OnSizeChanged().

[Peter Kasting, 2011-04-01 20:29:16]

New patch up.  The previous patch had two problems:
* The |fill_path| calculation relied on height(), which wouldn't be set until
after the function returned.
* Calculating the paths in the animation callback meant that they were not
recalculated when outside code changed the infobar width.

Both problems were fixed by moving the path calculations to Layout(), which is a
reasonable place to set the paths anyway.

Part of the reason I made this mistake is that I erroneously thought the various
"XXXSizeChanged()" functions would get called in all the places where the size
changes.  After thinking about whether I wanted to rearchitect the set of calls
here, I decided what I had was the right model, but I could make the
functionality clearer with some better function names, e.g. OnSizeChanged() ->
RecalculateHeight().  This patch includes those changes.

[Sheridan Rawlins, 2011-04-01 20:36:56]

Regarding the animation Reset calls the links I sent were from two files related
to translation infobars, which don't look like they cause Layout afterwards.

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
File chrome/browser/ui/views/infobars/infobar.h (right):

http://codereview.chromium.org/6788014/diff/5002/chrome/browser/ui/views/info...
chrome/browser/ui/views/infobars/infobar.h:55: virtual void
PlatformSpecificOnSizeChanged() {}
On 2011/04/01 19:07:15, Peter Kasting wrote:
> On 2011/04/01 18:41:48, Sheridan Rawlins wrote:
> > Move implementations to implementation file for this non-interface class.
> 
> That's not a style rule I can find.  Virtual functions should generally not be
> inlined, but except for constructors/destructors, it's OK to do so for empty
> bodies (since they truly don't generate any significant code), and doing so
> makes it much clearer that by default these do nothing.

Thanks, sorry - I had asked a peer who thought all methods should be in impl,
but I realize we do accessors all over the place.  Comment retracted.

[Sheridan Rawlins, 2011-04-01 20:38:41]

On 2011/04/01 20:29:16, Peter Kasting wrote:
> Both problems were fixed by moving the path calculations to Layout(), which is
a
> reasonable place to set the paths anyway.

I still don't get why OnBoundsChanged is not the appropriate place to adjust
paths/heights.  Isn't that method the definitive moment at which your sizes are
actually changed (as opposed to the other calls which are asking essentially for
preferred values?

[Peter Kasting, 2011-04-01 20:48:09]

On 2011/04/01 20:36:56, Sheridan Rawlins wrote:
> Regarding the animation Reset calls the links I sent were from two files
related
> to translation infobars, which don't look like they cause Layout afterwards.

Sorry, I missed the links (scanned past them as my eye assumed they were
codereview links).  Those calls reset the background color animation, which is a
totally different animation that doesn't have any effect on the infobar size.

On 2011/04/01 20:38:41, Sheridan Rawlins wrote:
> On 2011/04/01 20:29:16, Peter Kasting wrote:
> > Both problems were fixed by moving the path calculations to Layout(), which
is
> a
> > reasonable place to set the paths anyway.
> 
> I still don't get why OnBoundsChanged is not the appropriate place to adjust
> paths/heights.  Isn't that method the definitive moment at which your sizes
are
> actually changed (as opposed to the other calls which are asking essentially
for
> preferred values?

There's little practical difference between doing this in OnBoundsChanged() and
Layout().  Doing it in Layout() has three small advantages:
* It's more consistent with how most of the rest of the codebase works --
generally, we do all recalculations related to bounds in Layout() functions. 
OnBoundsChanged() is more special-purpose.  sky can correct me if I'm wrong
here.
* Because we need to lay out the infobar contents in Layout(), this puts all of
the layout-related changes in one function instead of split in two.
* Related: We avoid having to override more of the API calls.

[Sheridan Rawlins, 2011-04-02 00:58:25]

Thanks for looking into and allaying my concerns.

LGTM.

On 2011/04/01 20:48:09, Peter Kasting wrote:
> On 2011/04/01 20:36:56, Sheridan Rawlins wrote:
> > Regarding the animation Reset calls the links I sent were from two files
> related
> > to translation infobars, which don't look like they cause Layout afterwards.
> 
> Sorry, I missed the links (scanned past them as my eye assumed they were
> codereview links).  Those calls reset the background color animation, which is
a
> totally different animation that doesn't have any effect on the infobar size.
> 
> On 2011/04/01 20:38:41, Sheridan Rawlins wrote:
> > On 2011/04/01 20:29:16, Peter Kasting wrote:
> > > Both problems were fixed by moving the path calculations to Layout(),
which
> is
> > a
> > > reasonable place to set the paths anyway.
> > 
> > I still don't get why OnBoundsChanged is not the appropriate place to adjust
> > paths/heights.  Isn't that method the definitive moment at which your sizes
> are
> > actually changed (as opposed to the other calls which are asking essentially
> for
> > preferred values?
> 
> There's little practical difference between doing this in OnBoundsChanged()
and
> Layout().  Doing it in Layout() has three small advantages:
> * It's more consistent with how most of the rest of the codebase works --
> generally, we do all recalculations related to bounds in Layout() functions. 
> OnBoundsChanged() is more special-purpose.  sky can correct me if I'm wrong
> here.
> * Because we need to lay out the infobar contents in Layout(), this puts all
of
> the layout-related changes in one function instead of split in two.
> * Related: We avoid having to override more of the API calls.