Add methods needed for overflow panels to NativePanel interface.

Add methods needed for overflow panels to NativePanel interface.

Add methods to NativePanel. Also provide dummy implementation for all platforms.

BUG=none
TEST=no test due to no new functionity

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

[jennb, 2011-11-24 01:16:28]

Thanks for sending this out early so we can get an idea of the planned design.

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:81: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
Is this the size when not hovering on the overflow area? Can you describe the
flow for how the overflow logic will figure out the size for each panel? I had
thought the overflow strip would figure out the bounds and size for each panel
and then call SetPanelBounds().

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:87: // Controls if the panel icon should
appear in the taskbar or the dock.
Does this concept apply to Mac? If not, then just say "desktop bar" as that's
what the enum calls it.

[jianli, 2011-11-24 01:47:10]

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:81: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
On 2011/11/24 01:16:28, jennb wrote:
> Is this the size when not hovering on the overflow area? Can you describe the
> flow for how the overflow logic will figure out the size for each panel? I had
> thought the overflow strip would figure out the bounds and size for each panel
> and then call SetPanelBounds().

This method only intends to return platform-specific size for iconified panel.
The OverflowStrip::Layout will use this size to figure out the bounds per mouse
hover over or not. I will put comment in the panel_overflow_strip.h that is
going to send out in another patch for review.

http://codereview.chromium.org/8681025/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:87: // Controls if the panel icon should
appear in the taskbar or the dock.
On 2011/11/24 01:16:28, jennb wrote:
> Does this concept apply to Mac? If not, then just say "desktop bar" as that's
> what the enum calls it.

Yes.

[Dmitry Titov, 2011-11-24 02:21:48]

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

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:85: virtual void
EnsurePanelFullyVisible() = 0;
BringToTopWithoutActivation?

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:87: // Controls if the panel icon should
appear in the taskbar or the dock.
I wonder how we would implement this... Is it possible to just turn on/off? I am
not sure it is possible on mac... It has to be an actual system window minimized
by OS means or an app... So it probably will never be implemented for Mac...
Don't know about Linux.

Also, how about naming it SetPanelIconInDesktopBarState(PanelIconState state)?

[jianli, 2011-11-24 02:39:53]

On Wed, Nov 23, 2011 at 6:21 PM, <dimich@chromium.org> wrote:

>
> http://codereview.chromium.**org/8681025/diff/1009/chrome/**
>
browser/ui/panels/native_**panel.h<http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/native_panel.h>
> File chrome/browser/ui/panels/**native_panel.h (right):
>
> http://codereview.chromium.**org/8681025/diff/1009/chrome/**
>
browser/ui/panels/native_**panel.h#newcode85<http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/native_panel.h#newcode85>
> chrome/browser/ui/panels/**native_panel.h:85: virtual void
> EnsurePanelFullyVisible() = 0;
> BringToTopWithoutActivation?
>

This is what I called it previously. Jenn suggested sth like EnsureVisible.

>
> http://codereview.chromium.**org/8681025/diff/1009/chrome/**
>
browser/ui/panels/native_**panel.h#newcode87<http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/native_panel.h#newcode87>
>
> chrome/browser/ui/panels/**native_panel.h:87: // Controls if the panel
> icon should appear in the taskbar or the dock.
> I wonder how we would implement this... Is it possible to just turn
> on/off? I am not sure it is possible on mac... It has to be an actual
> system window minimized by OS means or an app... So it probably will
> never be implemented for Mac... Don't know about Linux.
>
> Also, how about naming it SetPanelIconInDesktopBarState(**PanelIconState
> state)?
>
>
http://codereview.chromium.**org/8681025/<http://codereview.chromium.org/8681...
>

[prasadt, 2011-11-24 03:37:19]

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

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:38: };
This doesn't apply to Linux as panels don't have presence in the taskbar. If it
doesn't apply for Mac either (which seems to be the case per dimich's comment),
may be we should just move this into windows implementation of NativePanel.

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:81: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
When I see iconfiy, minimizing to task bar is what comes to mind. Would
GetOverflowPanelSize() be a better name?

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:88: virtual void
UpdatePanelIconInDesktopBar(PanelIconState state) = 0;
Same comment as for PanelIconState.

Appearance/disappearance of panel icon in the taskbar would be a side effect of
some operation, like tear off or overflow, and can be handled as such in Window
NativePanel implementation.

[jennb, 2011-11-28 19:02:01]

Maybe what we need is a brief design overflow of how OverflowStrip will
interface with Panels.

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

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:35: enum PanelIconState {
Would it violate coding style to move this enum right before the method that
uses it?

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:81: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
Consider determining the size based on what you need to show. E.g. the favicon
and the title is shown, so compute the size in the OverflowStrip by adding
TitleOnlyHeight with icon size and whatever padding between the two. Then maybe
we don't need a new API method here at all.

[Dmitry Titov, 2011-11-30 23:27:01]

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

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:81: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
On 2011/11/28 19:02:01, jennb wrote:
> Consider determining the size based on what you need to show. E.g. the favicon
> and the title is shown, so compute the size in the OverflowStrip by adding
> TitleOnlyHeight with icon size and whatever padding between the two. Then
maybe
> we don't need a new API method here at all.

It seems we need this method for the same reason as TitleOnlyHeight(). The
platforms have different padding and borders, and even icon size may not be
constant.

http://codereview.chromium.org/8681025/diff/1009/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:87: // Controls if the panel icon should
appear in the taskbar or the dock.
I have an idea about it - you seem to need this method to do whatever OS-specifc
adjustments needs to be done to the representatino of native panel when it goes
to/from overflow bar. Why not have EnterOverflow/ExitOverflow() and let platform
decide actual semantics of update (Win can update taskbar for example while
others can ignore the thing or change layout of titlebar)

[jianli, 2011-12-01 02:07:12]

Removed UpdatePanelInDesktopBar as discussed.

[Dmitry Titov, 2011-12-01 02:17:21]

lgtm

[prasadt, 2011-12-01 03:41:36]

lgtm

I still think iconified in GetIconifiedPanelSize is confusing but I don't have
any great suggestions. Here are a few for you to consider:

GetOverflowPanelSize which I already suggested.

If this is the overflow panel size in contracted mode when mouse is not hovering
over it, we could call it:

GetOverflowPanelShrunkSize or
GetOverflowPanelContractedSize or
GetOverflowPanelUnexpandedSize

[jianli, 2011-12-01 18:35:06]

On 2011/12/01 03:41:36, prasadt wrote:
> lgtm
> 
> I still think iconified in GetIconifiedPanelSize is confusing but I don't have
> any great suggestions. Here are a few for you to consider:
> 
> GetOverflowPanelSize which I already suggested.
> 
> If this is the overflow panel size in contracted mode when mouse is not
hovering
> over it, we could call it:
> 
> GetOverflowPanelShrunkSize or
> GetOverflowPanelContractedSize or
> GetOverflowPanelUnexpandedSize

I rather keep the name as it is now. I am thinking of moving the size of hovered
overflow panel, which is a fixed value in PanelOverflowStrip, to native panel in
the future and then we can use the name GetOverflowPanelSize.

[jennb, 2011-12-01 18:45:24]

LGTM

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

http://codereview.chromium.org/8681025/diff/8001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:76: virtual gfx::Size
GetIconifiedPanelSize() const = 0;
Agree with prasadt that on confusion as to what an iconified panel is. Does this
mean IconOnlySize()?

http://codereview.chromium.org/8681025/diff/8001/chrome/browser/ui/panels/nat...
chrome/browser/ui/panels/native_panel.h:80: virtual void
EnsurePanelFullyVisible() = 0;
I don't care if you want to call this BringToTopWithoutActivation. I don't even
remember asking for the name change to EnsureVisible.

[Dmitry Titov, 2011-12-01 18:58:52]

I'd propose we land it. It's a trivial patch that is already there for a week.
If we find a better names for things, it's a very easy cleanup.

The name is a bikeshedding here. As we change design to add a some other
indicator (message counter?) it may change semantics. It's not important what
exactly the view of the panel in overflow bar includes, baking it into name is
exposing actual implementation. I'd say it's "SizeForOverflowModeMinimized"
which is horrible. We might not have good name before we actually implement a
bit more of the stuff.

Lets move forward.