Add Panel::SetPanelBoundsInstantly for all platforms.

Add Panel::SetPanelBoundsInstantly for all platforms.

This is needed for panel overflow handling. PanelOverflowStrip (to be added in another patch) will control the hover animation for all overflow panels so that they can be synchronized well. SetPanelBoundsInstantly tells the panel to change bounds instantly without animation.

BUG=none
TEST=new test

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

[Dmitry Titov, 2011-12-01 19:59:51]

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:139: SetPanelBounds(bounds, true);  // use
animation.
We tend to do enums instead of bools if callsites have to pass literal value -
because at callsite, it's not clear what 'true' means w/o lookup at the method
signature.

Could you change it to enum? Or even better, have NativePanel::SetPanelBounds()
with existing semantics (animated) and new NativePanel::SetPanelBoundsInstant().

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:219: // Passing true to |animate| if bounds
animation is required.
I think this comment says as much as the type and name of the parameter. Remove
comment?

[jianli, 2011-12-01 20:24:07]

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel.cc
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:139: SetPanelBounds(bounds, true);  // use
animation.
On 2011/12/01 19:59:51, Dmitry Titov wrote:
> We tend to do enums instead of bools if callsites have to pass literal value -
> because at callsite, it's not clear what 'true' means w/o lookup at the method
> signature.
> 
> Could you change it to enum? Or even better, have
NativePanel::SetPanelBounds()
> with existing semantics (animated) and new
NativePanel::SetPanelBoundsInstant().

Done.

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8775022/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:219: // Passing true to |animate| if bounds
animation is required.
On 2011/12/01 19:59:51, Dmitry Titov wrote:
> I think this comment says as much as the type and name of the parameter.
Remove
> comment?

Done.

[jennb, 2011-12-01 20:52:37]

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:115: void
PanelBrowserWindowCocoa::SetPanelBoundsInstantly(const gfx::Rect& bounds) {
Could you use the same pattern here as you did for Win and GTK with the common
code in a subr?

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:895:
EXPECT_FALSE(panel_testing->IsAnimatingBounds());
This check is already at the end of WaitForBoundsAnimationFinished.

[Dmitry Titov, 2011-12-01 20:52:50]

lgtm

I'd be glad to see no explicit passing of 'true' or 'false' in internal methods,
but it can be cleaned up later. Could you please create a Pri-3 bug for code
cleanup with link to this CR and description?

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:112: [controller_
setPanelFrame:frame animate:true];
should be animate:YES in Obj-C code

http://codereview.chromium.org/8775022/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:122: [controller_
setPanelFrame:frame animate:false];
animate:NO

[prasadt, 2011-12-01 22:21:44]

lgtm

One suggestion to consider.

http://codereview.chromium.org/8775022/diff/3002/chrome/browser/ui/panels/nat...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8775022/diff/3002/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:41: virtual void
SetPanelBoundsInstantly(const gfx::Rect& bounds) = 0;
Why a separate method? Why not a single method that takes as second parameter?

[Dmitry Titov, 2011-12-01 22:28:26]

On 2011/12/01 22:21:44, prasadt wrote:
> lgtm
> 
> One suggestion to consider.
> 
>
http://codereview.chromium.org/8775022/diff/3002/chrome/browser/ui/panels/nat...
> File chrome/browser/ui/panels/native_panel.h (right):
> 
>
http://codereview.chromium.org/8775022/diff/3002/chrome/browser/ui/panels/nat...
> chrome/browser/ui/panels/native_panel.h:41: virtual void
> SetPanelBoundsInstantly(const gfx::Rect& bounds) = 0;
> Why a separate method? Why not a single method that takes as second parameter?

To avoid having a method with a bool param that has to be passed a literal at
callsites. Or creating an enum.