Support minimizing the panel into 3-pixel line on Windows. Also support bringing up/down the titl...

Support minimizing the panel into 3-pixel line on Windows. Also support bringing up/down the titlebar when we move the mouse over the bottom area.

BUG=none
TEST=new browser test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91222

[Dmitry Titov, 2011-06-24 02:05:45]

One comment for start, I still need to review it.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:37: virtual void RestorePanel(bool
titlebar_only) = 0;
Hmm.

So we had 2 states (minimized and restored) and 2 methods to switch into one of
them. Now we've got 3rd state (show titlebar only).

I would suggest using 3rd method, or perhaps even have one method
(SetFrameMode?) with enum parameter (MINIMIZED, TITLE_ONLY, NORMAL?) rather then
modify the behavior of RestorePanel with a boolean.

[jianli, 2011-06-24 04:03:41]

I prefer having one method, perhaps calling it SetPanelState(PanelState)?


On Thu, Jun 23, 2011 at 7:05 PM, <dimich@chromium.org> wrote:

> One comment for start, I still need to review it.
>
>
> http://codereview.chromium.**org/7242017/diff/1/chrome/**
>
browser/ui/panels/native_**panel.h<http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/native_panel.h>
> File chrome/browser/ui/panels/**native_panel.h (right):
>
> http://codereview.chromium.**org/7242017/diff/1/chrome/**
>
browser/ui/panels/native_**panel.h#newcode37<http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/native_panel.h#newcode37>
> chrome/browser/ui/panels/**native_panel.h:37: virtual void
> RestorePanel(bool titlebar_only) = 0;
> Hmm.
>
> So we had 2 states (minimized and restored) and 2 methods to switch into
> one of them. Now we've got 3rd state (show titlebar only).
>
> I would suggest using 3rd method, or perhaps even have one method
> (SetFrameMode?) with enum parameter (MINIMIZED, TITLE_ONLY, NORMAL?)
> rather then modify the behavior of RestorePanel with a boolean.
>
>
>
http://codereview.chromium.**org/7242017/<http://codereview.chromium.org/7242...
>

[Dmitry Titov, 2011-06-24 06:17:23]

On 2011/06/24 04:03:41, jianli wrote:
> I prefer having one method, perhaps calling it SetPanelState(PanelState)?
> 

PanelState sounds a bit too generic, for example Active/Inactive is also a panel
state... Maybe SetPanelFrameConfiguration(FrameConfiguration), since this is
about the way the Frame around the Panel is shown (sometimes not leaving the
space for the Panel's content)?

[dcheng, 2011-06-24 08:13:17]

On 2011/06/24 06:17:23, Dmitry Titov wrote:
> On 2011/06/24 04:03:41, jianli wrote:
> > I prefer having one method, perhaps calling it SetPanelState(PanelState)?
> > 
> 
> PanelState sounds a bit too generic, for example Active/Inactive is also a
panel
> state... Maybe SetPanelFrameConfiguration(FrameConfiguration), since this is
> about the way the Frame around the Panel is shown (sometimes not leaving the
> space for the Panel's content)?

How about FrameState? FrameConfiguration is a lot of characters =)

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:65: protected:
I think the C++ guide prefers that data members be private. Can we just add a
protected accessor for the active panels?

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:18: SIZE_T result =
VirtualQuery(address, &mbi, sizeof(mbi));
Nit: :: -- I'm actually indifferent about this, but it's nice to be consistent.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:25: return
GetModuleHandleFromAddress(GetCurrentModuleHandle);
Nite: ::

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:64: WH_MOUSE_LL,
MouseHookProc,GetCurrentModuleHandle(), 0);
Nit: space.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:70:
UnhookWindowsHookEx(mouse_hook_);
Nit: ::

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:77: y >
(panel_manager_->work_area_.bottom() - height_to_watch);
How will this interact with multiple monitors?

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:96: return CallNextHookEx(NULL,
code, wparam, lparam);
Nit: ::

[jennb, 2011-06-27 23:18:02]

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:29:
frame->set_frame_type(views::Widget::FRAME_TYPE_FORCE_CUSTOM);
Curious - why did set_frame_type have to be moved here from the BrowserFrame
constructor?

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:171: if (expand_state_ !=
FULLY_MINIMIZED)
Should this be a DCHECK instead?

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.h:27: enum ExpandState {
This ExpandState seems like something all platforms will have to track. Move to
NativePanel or Panel class?  Also, this sounds like the proposed FrameState in
the other comments on this patch.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:255: void
TestMinimizeAndRestore() {
Could you add a comment at the top of this class explaining why all the test
logic is in this class instead of in the test?  Will save others from asking the
same question I did.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:49: PanelManager*
PanelManager::Create() {
Why only when not Windows?

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:27: // If an active panel is removed,
pending panels could put on display if we
Fix grammar in "pending planels could put on display if we have spaces".

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager_win.h (right):

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.h:14: class PanelManagerWin : public
PanelManager {
Hmmm...can we get away without platform specific panel manager?  This
implementation watches the bottom area of the screen to raise/lower all title
bars for minimized panels. If I understand correctly, it will do this even if
the mouse is at the bottom of an open panel.  Seems better to handle this within
each individual panel so that if the panel is minimized and the mouse hovers
over the bottom area of the screen where that panel is located (might not be on
minimized panel directly), then raise all titlebars of minimized panels.
Otherwise, if the mouse is in the lower portion of an open panel, leave
minimized panels alone.

[jianli, 2011-06-29 01:28:12]

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/native...
chrome/browser/ui/panels/native_panel.h:37: virtual void RestorePanel(bool
titlebar_only) = 0;
On 2011/06/24 02:05:45, Dmitry Titov wrote:
> Hmm.
> 
> So we had 2 states (minimized and restored) and 2 methods to switch into one
of
> them. Now we've got 3rd state (show titlebar only).
> 
> I would suggest using 3rd method, or perhaps even have one method
> (SetFrameMode?) with enum parameter (MINIMIZED, TITLE_ONLY, NORMAL?) rather
then
> modify the behavior of RestorePanel with a boolean.

As discussed, I changed to using one method SetPanelExpansionState. I also
changed to name the enum as ExpansionState.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:29:
frame->set_frame_type(views::Widget::FRAME_TYPE_FORCE_CUSTOM);
On 2011/06/27 23:18:02, jennb wrote:
> Curious - why did set_frame_type have to be moved here from the BrowserFrame
> constructor?

Not needed now. Reverted.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.cc:171: if (expand_state_ !=
FULLY_MINIMIZED)
On 2011/06/27 23:18:02, jennb wrote:
> Should this be a DCHECK instead?

Not needed due to logic change.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_browser_view.h (right):

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view.h:27: enum ExpandState {
On 2011/06/27 23:18:02, jennb wrote:
> This ExpandState seems like something all platforms will have to track. Move
to
> NativePanel or Panel class?  Also, this sounds like the proposed FrameState in
> the other comments on this patch.

Moved to Panel.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:255: void
TestMinimizeAndRestore() {
On 2011/06/27 23:18:02, jennb wrote:
> Could you add a comment at the top of this class explaining why all the test
> logic is in this class instead of in the test?  Will save others from asking
the
> same question I did.

Done.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:49: PanelManager*
PanelManager::Create() {
On 2011/06/27 23:18:02, jennb wrote:
> Why only when not Windows? 

Not needed. Reverted.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:27: // If an active panel is removed,
pending panels could put on display if we
On 2011/06/27 23:18:02, jennb wrote:
> Fix grammar in "pending planels could put on display if we have spaces".

Could you suggest what need to change?

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:65: protected:
On 2011/06/24 08:13:18, dcheng wrote:
> I think the C++ guide prefers that data members be private. Can we just add a
> protected accessor for the active panels?

Not needed. Reverted.

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:18: SIZE_T result =
VirtualQuery(address, &mbi, sizeof(mbi));
On 2011/06/24 08:13:18, dcheng wrote:
> Nit: :: -- I'm actually indifferent about this, but it's nice to be
consistent.

Done.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:25: return
GetModuleHandleFromAddress(GetCurrentModuleHandle);
On 2011/06/24 08:13:18, dcheng wrote:
> Nite: ::

Done.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:64: WH_MOUSE_LL,
MouseHookProc,GetCurrentModuleHandle(), 0);
On 2011/06/24 08:13:18, dcheng wrote:
> Nit: space.

Done.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:70:
UnhookWindowsHookEx(mouse_hook_);
On 2011/06/24 08:13:18, dcheng wrote:
> Nit: ::

Done.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:77: y >
(panel_manager_->work_area_.bottom() - height_to_watch);
On 2011/06/24 08:13:18, dcheng wrote:
> How will this interact with multiple monitors?

We're only showing panels in primary monitor.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.cc:96: return CallNextHookEx(NULL,
code, wparam, lparam);
On 2011/06/24 08:13:18, dcheng wrote:
> Nit: ::

Done.

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
File chrome/browser/ui/panels/panel_manager_win.h (right):

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager_win.h:14: class PanelManagerWin : public
PanelManager {
On 2011/06/27 23:18:02, jennb wrote:
> Hmmm...can we get away without platform specific panel manager?  This
> implementation watches the bottom area of the screen to raise/lower all title
> bars for minimized panels. If I understand correctly, it will do this even if
> the mouse is at the bottom of an open panel.  Seems better to handle this
within
> each individual panel so that if the panel is minimized and the mouse hovers
> over the bottom area of the screen where that panel is located (might not be
on
> minimized panel directly), then raise all titlebars of minimized panels.
> Otherwise, if the mouse is in the lower portion of an open panel, leave
> minimized panels alone. 

Done.

[jianli, 2011-06-29 18:25:13]

I uploaded a new patch to implement the logic to bring up/down the titlebar as
discussed.

[jennb, 2011-06-29 21:10:09]

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

http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:27: // If an active panel is removed,
pending panels could put on display if we
On 2011/06/29 01:28:12, jianli wrote:
> On 2011/06/27 23:18:02, jennb wrote:
> > Fix grammar in "pending planels could put on display if we have spaces".
> 
> Could you suggest what need to change?

..., pending panels will be displayed if space allows.

[jianli, 2011-06-29 21:13:08]

Fixed. New patch uploaded.

On Wed, Jun 29, 2011 at 2:10 PM, <jennb@chromium.org> wrote:

>
> http://codereview.chromium.**org/7242017/diff/1/chrome/**
>
browser/ui/panels/panel_**manager.h<http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_manager.h>
> File chrome/browser/ui/panels/**panel_manager.h (right):
>
> http://codereview.chromium.**org/7242017/diff/1/chrome/**
>
browser/ui/panels/panel_**manager.h#newcode27<http://codereview.chromium.org/7242017/diff/1/chrome/browser/ui/panels/panel_manager.h#newcode27>
> chrome/browser/ui/panels/**panel_manager.h:27: // If an active panel is
> removed, pending panels could put on display if we
> On 2011/06/29 01:28:12, jianli wrote:
>
>> On 2011/06/27 23:18:02, jennb wrote:
>> > Fix grammar in "pending planels could put on display if we have
>>
> spaces".
>
>  Could you suggest what need to change?
>>
>
> ..., pending panels will be displayed if space allows.
>
>
>
http://codereview.chromium.**org/7242017/<http://codereview.chromium.org/7242...
>

[jennb, 2011-06-29 23:17:54]

I don't see any test coverage for "bring up all titlebar" logic.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:9: #include
"chrome/browser/ui/panels/panel.h"
Gut reaction was this include felt circular. We're only including it for the
ExpansionState enum, right? Maybe that should go in some panel_constants.h
file...

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:40: virtual bool
ShouldBringUpPanelTitleBar(int mouse_x, int mouse_y) const = 0;
Could you add comments explaining what x and y are and describe the purpose of
this method?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:29: : native_panel_(NULL),
Why does native_panel_ need to be set to NULL here?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:56: if (expansion_state_ == Panel::EXPANDED)
if (expansion_state_ != Panel::MINIMIZED)

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:41: // * Fully expanded
Move these to comments on the enum values.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:46: // Should we bring up the title-bar given
the current mouse point?
Comments should explain what "bring up title-bar" means. Otherwise, these
comments simply repeat the name of the function without providing additional
info.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:66: if
(panel_->manager()->active_count())
panel_manager has not removed this closing panel yet. Panel::Close() calls
native panel close before panel_manager remove().

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:166: void
PanelBrowserView::SetPanelExpansionState(
This seems like it should really be OnExpansionStateChanged()

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:181: NOTREACHED();
NOTREACHED() is only defined for debug mode, right? That would mean height is
uninitialized in non-debug mode?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:194: mouse_y >= bounds_.y();
I'm assuming some logic makes sure mouse_y isn't way above the bounds_.y()
value?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:294:
browser_view->SetPanelExpansionState(Panel::MINIMIZED);
Should this be panel->SetPanelExpansionState()?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:272: bool
PanelManager::ShouldBringUpTitleBarForAllMinimizedPanels(
Rather than ask every panel if it should bring up title bar, should panel
manager only ask the panel containing the mouse_x point? Or, this could be
platform specific logic. Perhaps what you really want is a way to iterate
through all active panels using PanelManager API?

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_mouse_watcher_win.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_mouse_watcher_win.cc:87: new
PanelMouseWatcherWin());
could fit on above line

[jianli, 2011-06-30 01:28:33]

More tests added.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
File chrome/browser/ui/panels/native_panel.h (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:9: #include
"chrome/browser/ui/panels/panel.h"
On 2011/06/29 23:17:54, jennb wrote:
> Gut reaction was this include felt circular. We're only including it for the
> ExpansionState enum, right? Maybe that should go in some panel_constants.h
> file...

It is really not circular. Though it is kind of ugly, we do not want to move it
to a new header file that has it exposed directly. I keep it as now per our
discussion.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/na...
chrome/browser/ui/panels/native_panel.h:40: virtual bool
ShouldBringUpPanelTitleBar(int mouse_x, int mouse_y) const = 0;
On 2011/06/29 23:17:54, jennb wrote:
> Could you add comments explaining what x and y are and describe the purpose of
> this method?

Done.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:29: : native_panel_(NULL),
On 2011/06/29 23:17:54, jennb wrote:
> Why does native_panel_ need to be set to NULL here?

I think it is good to initialize the pointer here, just to be safe, in case that
people move the code to set native_panel_ out of the constructor.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:56: if (expansion_state_ == Panel::EXPANDED)
On 2011/06/29 23:17:54, jennb wrote:
> if (expansion_state_ != Panel::MINIMIZED)

If the panel state is TITLEBAR_ONLY, we still want to check if the mouse point
is inside the titlebar bounds.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:41: // * Fully expanded
On 2011/06/29 23:17:54, jennb wrote:
> Move these to comments on the enum values.

Done.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:46: // Should we bring up the title-bar given
the current mouse point?
On 2011/06/29 23:17:54, jennb wrote:
> Comments should explain what "bring up title-bar" means. Otherwise, these
> comments simply repeat the name of the function without providing additional
> info.

Done.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:166: void
PanelBrowserView::SetPanelExpansionState(
On 2011/06/29 23:17:54, jennb wrote:
> This seems like it should really be OnExpansionStateChanged()

Done. Changed to OnPanelExpansionStateChanged since we want Panel to appear in
all NativePanel methods.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:181: NOTREACHED();
On 2011/06/29 23:17:54, jennb wrote:
> NOTREACHED() is only defined for debug mode, right? That would mean height is
> uninitialized in non-debug mode?

Yes. Normally we should not reach this. But just to be safe, I set the height to
original_height_ here.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:194: mouse_y >= bounds_.y();
On 2011/06/29 23:17:54, jennb wrote:
> I'm assuming some logic makes sure mouse_y isn't way above the bounds_.y()
> value?

Do we care? I think the mouse coordinates are provided by the system and it
should be a valid number.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:294:
browser_view->SetPanelExpansionState(Panel::MINIMIZED);
On 2011/06/29 23:17:54, jennb wrote:
> Should this be panel->SetPanelExpansionState()?

Done.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:272: bool
PanelManager::ShouldBringUpTitleBarForAllMinimizedPanels(
On 2011/06/29 23:17:54, jennb wrote:
> Rather than ask every panel if it should bring up title bar, should panel
> manager only ask the panel containing the mouse_x point? Or, this could be
> platform specific logic. Perhaps what you really want is a way to iterate
> through all active panels using PanelManager API?

I've thought about providing the iteration API but I rather not give it since I
do not want a panel to talk with any other panel directly.

I also rather leave to the native panel to decide how to check mouse_x and
mouse_y in since the individual platform might have special logic.

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_mouse_watcher_win.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_mouse_watcher_win.cc:87: new
PanelMouseWatcherWin());
On 2011/06/29 23:17:54, jennb wrote:
> could fit on above line

Done.

[jennb, 2011-06-30 17:33:31]

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7242017/diff/15025/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:194: mouse_y >= bounds_.y();
On 2011/06/30 01:28:33, jianli wrote:
> On 2011/06/29 23:17:54, jennb wrote:
> > I'm assuming some logic makes sure mouse_y isn't way above the bounds_.y()
> > value?
> 
> Do we care? I think the mouse coordinates are provided by the system and it
> should be a valid number.

I meant it's not clear from looking at this code that mouse_y is limited to some
range. If some code elsewhere is taking care of ensuring that we should comment
as such here or we DCHECK it here.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:46: void
Panel::SetExpansionState(ExpansionState expansion_state) {
Nit: wonder if code would be a little easier to read if the param was called
new_state? Hard for my eye to track just an _ char.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:30: // The panel is in fully expanded with both
title-bar and the client-area.
s/is in/is

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:330: return gfx::Size();
Don't we have to ensure there is a minimum width - at least for a close button,
wrench and a few chars of title text?

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:66: if
(panel_->manager()->active_count())
Should check for active count is 1? There must not be test coverage to check if
mouse watcher is properly started and stopped. ;-)  Maybe easier for mouse
watcher to keep its own ref count?

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:167: Panel::ExpansionState
expansion_state) {
Variable was named 'state' in .h. Should match names.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:271: (panel_->expansion_state()
!= Panel::EXPANDED) ? Panel::EXPANDED
What if user clicks on the title bar in title-bar-only state?

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:296:
panel1->SetExpansionState(Panel::MINIMIZED);
Also check mouse watcher started?

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:356: panel1->Close();
Also check mouse watcher still active after each close, until panel 3 is closed?

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.h (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.h:33: virtual virtual void
OnPanelExpansionStateChanged(
virtual virtual

[jianli, 2011-06-30 18:22:36]

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:46: void
Panel::SetExpansionState(ExpansionState expansion_state) {
On 2011/06/30 17:33:31, jennb wrote:
> Nit: wonder if code would be a little easier to read if the param was called
> new_state? Hard for my eye to track just an _ char.

I rename it to new_expansion_state.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:30: // The panel is in fully expanded with both
title-bar and the client-area.
On 2011/06/30 17:33:31, jennb wrote:
> s/is in/is

Done.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_frame_view.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_frame_view.cc:330: return gfx::Size();
On 2011/06/30 17:33:31, jennb wrote:
> Don't we have to ensure there is a minimum width - at least for a close
button,
> wrench and a few chars of title text?

Added comment for doing thing like this.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:66: if
(panel_->manager()->active_count())
On 2011/06/30 17:33:31, jennb wrote:
> Should check for active count is 1? There must not be test coverage to check
if
> mouse watcher is properly started and stopped. ;-)  Maybe easier for mouse
> watcher to keep its own ref count?

Done. Also added test coverage.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:167: Panel::ExpansionState
expansion_state) {
On 2011/06/30 17:33:31, jennb wrote:
> Variable was named 'state' in .h. Should match names. 

Done.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:271: (panel_->expansion_state()
!= Panel::EXPANDED) ? Panel::EXPANDED
On 2011/06/30 17:33:31, jennb wrote:
> What if user clicks on the title bar in title-bar-only state?

The panel will be fully expanded.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view_browsertest.cc (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:296:
panel1->SetExpansionState(Panel::MINIMIZED);
On 2011/06/30 17:33:31, jennb wrote:
> Also check mouse watcher started?

Done.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view_browsertest.cc:356: panel1->Close();
On 2011/06/30 17:33:31, jennb wrote:
> Also check mouse watcher still active after each close, until panel 3 is
closed?

Done.

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.h (right):

http://codereview.chromium.org/7242017/diff/14010/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.h:33: virtual virtual void
OnPanelExpansionStateChanged(
On 2011/06/30 17:33:31, jennb wrote:
> virtual virtual

Done.

[jennb, 2011-06-30 18:26:14]

LGTM

[Dmitry Titov, 2011-06-30 18:43:30]

LGTM from me too, need to update comment on
NativePanel::ShouldBringUpPanelTitleBar - discussed offline.