Panels: Disable auto-resize if initial size is specified during creation.

Panels: Disable auto-resize if initial size is specified during creation.

Minimum size of panels is no longer the same as the initial size as that caused the default size for panels to also act as the minimum size.

BUG=104646
TEST=PanelBrowserTest.*


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

[jennb, 2011-11-18 22:31:04]

Jian - could you review this as you're most familiar with auto-resize code?

Dave - just cc'ing you so you'll have a heads up before merging.

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:595: 
Oops. Will delete this extra line.

[Dmitry Titov, 2011-11-18 23:00:34]

drive-by:

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:239: void
EnableRendererAutoResize(RenderViewHost* render_view_host);
A comment for this second method could explain why similar-looking methods are
needed...

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:104: bool auto_resize = !(width ||
height);
I think the coding style actually prefers this style for pointers (do not
compare with NULL) but does not encourage that for sizes, times or other
non-pointer integral types. 

From readability point, it makes it less spottable that size has a sentinel
value that means something different then size (in itself an unfortunate idea,
but at least it is good when it is explicit). 

In next 2 lines you compare with 0 explicitly. What should it be?

[jianli, 2011-11-18 23:03:32]

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:81: if (auto_resizable_) {
Is this needed now? SetMaxSize seems to be always called before auto_resizable_
is changed to true from the default false value.

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:207: bool auto_resizable() { return
auto_resizable_; }
const.

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:237: void SetUpAutoResize(TabContents*
tab_contents);
'SetUpAutoResize' seems to be easily confused with SetAutoResizable. How about
making the method name more explicit, like EnableAutoResizeForTabContents?

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:104: bool auto_resize = !(width ||
height);
I find it more readable if we say "width = 0 && height == 0".

[jennb, 2011-11-18 23:26:00]

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.cc:81: if (auto_resizable_) {
On 2011/11/18 23:03:32, jianli wrote:
> Is this needed now? SetMaxSize seems to be always called before
auto_resizable_
> is changed to true from the default false value.

I'm going to leave this rather than assume the max size will never change once a
panel is created. As you pointed out, we will need a separate patch to change
max size on panel in various scenarios that we're not handling yet.

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

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:207: bool auto_resizable() { return
auto_resizable_; }
On 2011/11/18 23:03:32, jianli wrote:
> const.

Done.

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:237: void SetUpAutoResize(TabContents*
tab_contents);
On 2011/11/18 23:03:32, jianli wrote:
> 'SetUpAutoResize' seems to be easily confused with SetAutoResizable. How about
> making the method name more explicit, like EnableAutoResizeForTabContents?

Done.

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:239: void
EnableRendererAutoResize(RenderViewHost* render_view_host);
On 2011/11/18 23:00:34, Dmitry Titov wrote:
> A comment for this second method could explain why similar-looking methods are
> needed...

Sure. Though this function should go away after Dave's refactor because then it
will only require one renderer call to set up auto-resize instead of having to
use a helper to group both calls together.

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8602007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:104: bool auto_resize = !(width ||
height);
On 2011/11/18 23:00:34, Dmitry Titov wrote:
> I think the coding style actually prefers this style for pointers (do not
> compare with NULL) but does not encourage that for sizes, times or other
> non-pointer integral types. 
> 
> From readability point, it makes it less spottable that size has a sentinel
> value that means something different then size (in itself an unfortunate idea,
> but at least it is good when it is explicit). 
> 
> In next 2 lines you compare with 0 explicitly. What should it be?

Done.

[Dmitry Titov, 2011-11-18 23:48:26]

another drive-by (sorry):

http://codereview.chromium.org/8602007/diff/7002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8602007/diff/7002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:128: Panel* panel = new Panel(browser,
gfx::Rect(x, y, width, height));
Does it still make sense to pass bounds into constructor if we also explicitly
set min and max here?

Maybe we should be passing explicit min, max, default sizes into constructor and
also an 'autoresize' bit?
Alternatively, we could not pass any sizes to constructor and initialize
min=max=default=0 and then rely on explicit setters.
Either one seems better then pass one size and then set 2 others.

[jennb, 2011-11-18 23:54:01]

http://codereview.chromium.org/8602007/diff/7002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/8602007/diff/7002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:128: Panel* panel = new Panel(browser,
gfx::Rect(x, y, width, height));
On 2011/11/18 23:48:27, Dmitry Titov wrote:
> Does it still make sense to pass bounds into constructor if we also explicitly
> set min and max here?
> 
> Maybe we should be passing explicit min, max, default sizes into constructor
and
> also an 'autoresize' bit?
> Alternatively, we could not pass any sizes to constructor and initialize
> min=max=default=0 and then rely on explicit setters.
> Either one seems better then pass one size and then set 2 others.

It still makes sense as native panel needs bounds when its constructed. The
others are after-thoughts that could be set or not set. Putting them in the ctor
would mean they are all required.

[jianli, 2011-11-19 00:11:48]

lgtm

[commit-bot: I haz the power, 2011-11-19 00:16:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8602007/7002

[jennb, 2011-11-22 00:17:40]

prasadt - please see the change in panel_browser_window_gtk.cc. It fixes
mis-positioning of the panels, revealed now that the starting size of a panel
can change to smaller than initial default size.

[prasadt, 2011-11-22 01:17:39]

http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:145: int top =
bounds_.bottom() - height;
Bottom should always be the same as
panel_->manager()->GetBottomPositionForExpansionState() as far as I can tell.
And if its not, we should use,
panel_->manager()->GetBottomPositionForExpansionState().

http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:146: int left =
bounds_.right() - width;
Again, left should be the same bounds_.x() and where its not, we should use
bounds_.x() here.

[prasadt, 2011-11-22 01:33:50]

http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):

http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_gtk.cc:146: int left =
bounds_.right() - width;
On 2011/11/22 01:17:39, prasadt wrote:
> Again, left should be the same bounds_.x() and where its not, we should use
> bounds_.x() here.

I take it back. In theory it should be same as bounds_.x() but where its not we
should indeed use left.

Not sure why its starting smaller now. But thanks for the fix!

[prasadt, 2011-11-22 01:35:02]

lgtm in case commit bot needs it.

On 2011/11/22 01:33:50, prasadt wrote:
>
http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
> File chrome/browser/ui/panels/panel_browser_window_gtk.cc (right):
> 
>
http://codereview.chromium.org/8602007/diff/12001/chrome/browser/ui/panels/pa...
> chrome/browser/ui/panels/panel_browser_window_gtk.cc:146: int left =
> bounds_.right() - width;
> On 2011/11/22 01:17:39, prasadt wrote:
> > Again, left should be the same bounds_.x() and where its not, we should use
> > bounds_.x() here.
> 
> I take it back. In theory it should be same as bounds_.x() but where its not
we
> should indeed use left.
> 
> Not sure why its starting smaller now. But thanks for the fix!

[jennb, 2011-11-22 19:49:06]

dimich - latest revision, changed to use minimum size if no initial size is
given. Looks much better than using the default size and made AutoResize happy
on my local Windows machine. Re-running try jobs now. Could you LGTM this latest
change?

[Dmitry Titov, 2011-11-22 19:55:47]

good for autoresize case. What about the case when only one, width or height, is
specified - it is not auto-resize, but the non-specified size is going to be set
to minimum and fixed if I understand right. If this is true, then perhaps better
is to substitute the default size in such case as it will probably look better
in absence of autoresizing.

[jennb, 2011-11-22 23:23:27]

On 2011/11/22 19:55:47, Dmitry Titov wrote:
> good for autoresize case. What about the case when only one, width or height,
is
> specified - it is not auto-resize, but the non-specified size is going to be
set
> to minimum and fixed if I understand right. If this is true, then perhaps
better
> is to substitute the default size in such case as it will probably look better
> in absence of autoresizing.

Changed to compute a default value for the unspecified dimension based on a
ratio.

[Dmitry Titov, 2011-11-22 23:28:31]

LGTM

I'd go for a way more scientific constant, though - golden ratio for example
(http://en.wikipedia.org/wiki/Golden_ratio).  there is evidence it is improving
perception :-) But 4/3 is ok.

[jennb, 2011-11-23 00:13:51]

Try job results:
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/4642
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/4327
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/4249

[commit-bot: I haz the power, 2011-11-23 00:28:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8602007/28001

[commit-bot: I haz the power, 2011-11-23 01:54:51]

Change committed as 111283