Refactor PanelManager to create PanelStrip.

Refactor PanelManager to create PanelStrip.

* Added TestPanelMouseWatcher utility to help tests simulate mouse movements. 
* Separated out PanelBrowserTest.MinimizeRestore into 3 tests to make it easier to tell which part is failing when it fails.

BUG=None
TEST=Panel*Test.*

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

[jennb, 2011-11-30 18:04:27]

There you go, a new patch from scratch that just rips panel manager in two. I
tried really hard not to do any cleanup. No new functionality (e.g. overflow
logic). Should be fairly easy for you to review.

I intentionally kept changes in the tests to a minimum, which left some ugly
stubs in panel manager that we can clean up later.

Ignore the previous patch and only look at this one.

[jianli, 2011-11-30 19:27:01]

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

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:68: void MoveMouse(gfx::Point
position) {
const gfx::Point&

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:519:
EXPECT_EQ(panel->max_size().width(), bounds.width());
Why not:
  EXPECT_EQ(panel->min_size(), panel->GetBounds().size());

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:965:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, MinimizeRestore2) {
MinimizeRestore2 => MinimizeRestore2Panels

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:978:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, MinimizeRestore3) {
MinimizeRestore3 =>  MinimizeRestore3Panels

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:1535:
EXPECT_EQ(panel->min_size().width(), panel->GetBounds().width());
ditto.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:19: #include
"chrome/browser/ui/panels/panel_mouse_watcher.h"
Why do we need these 2 includes for UNIT_TEST? It seems panel_strip.h is needed
to access some PanelStrip methods in this file? Can you either change the tests
to use these PanelStrip methods directly or move these PanelManager method
declarations out of UNIT_TEST guard and move the implementation to .cc file?

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_strip.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:20: // This class manages a group of
panels displayed in a strip.
Probably more detail to the comment like:
  in a strip attached to the bottom of the screen.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:135: gfx::Rect strip_bounds_;
Would strip_area_ sounds better? The suffix |bounds| reminds me the bounds for a
window or control.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/tes...
File chrome/browser/ui/panels/test_panel_mouse_watcher.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/tes...
chrome/browser/ui/panels/test_panel_mouse_watcher.h:16: ~TestPanelMouseWatcher()
{
virtual.

[jennb, 2011-11-30 19:55:11]

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

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:68: void MoveMouse(gfx::Point
position) {
On 2011/11/30 19:27:01, jianli wrote:
> const gfx::Point&

Done.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:519:
EXPECT_EQ(panel->max_size().width(), bounds.width());
On 2011/11/30 19:27:01, jianli wrote:
> Why not:
>   EXPECT_EQ(panel->min_size(), panel->GetBounds().size());

It didn't work. I tried that first.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:965:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, MinimizeRestore2) {
On 2011/11/30 19:27:01, jianli wrote:
> MinimizeRestore2 => MinimizeRestore2Panels

Done.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:978:
IN_PROC_BROWSER_TEST_F(PanelBrowserTest, MinimizeRestore3) {
On 2011/11/30 19:27:01, jianli wrote:
> MinimizeRestore3 =>  MinimizeRestore3Panels

Done.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:19: #include
"chrome/browser/ui/panels/panel_mouse_watcher.h"
On 2011/11/30 19:27:01, jianli wrote:
> Why do we need these 2 includes for UNIT_TEST? It seems panel_strip.h is
needed
> to access some PanelStrip methods in this file? Can you either change the
tests
> to use these PanelStrip methods directly or move these PanelManager method
> declarations out of UNIT_TEST guard and move the implementation to .cc file?

Aren't we using UNIT_TEST guard and inline in .h was to keep the code from
existing in regular builds? Existing tests should not know that panel strips
exist. Each of the occurrences below violate abstractions, but I am putting off
that cleanup to a separate patch.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_strip.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:20: // This class manages a group of
panels displayed in a strip.
On 2011/11/30 19:27:01, jianli wrote:
> Probably more detail to the comment like:
>   in a strip attached to the bottom of the screen.

Added more details, but I purposely avoided limiting this to being docked along
the bottom of the screen. The strip merely positions panels within its bounds.
Exception that I want to cleanup in a separate patch is the title-only panels
appearing above an auto-hiding taskbar. Then this becomes a generic strip of
panels for minimized/title-only/expanded state. We could even dock it to the top
of the screen. Eventually, we could allow the entire strip to be positioned by
the user and perhaps even oriented vertically. Would be nice if the overflow
strip could be managed likewise.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:135: gfx::Rect strip_bounds_;
On 2011/11/30 19:27:01, jianli wrote:
> Would strip_area_ sounds better? The suffix |bounds| reminds me the bounds for
a
> window or control.

I think of the strip like a containing view and the panels are subviews within
its bounds.

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/tes...
File chrome/browser/ui/panels/test_panel_mouse_watcher.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/tes...
chrome/browser/ui/panels/test_panel_mouse_watcher.h:16: ~TestPanelMouseWatcher()
{
On 2011/11/30 19:27:01, jianli wrote:
> virtual.

Done and created separate .cc file to make clang happy.

[jianli, 2011-11-30 20:37:31]

lgtm

[Dmitry Titov, 2011-11-30 20:38:45]

lgtm with 2 nits

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

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:40:
auto_hiding_desktop_bar_(AutoHidingDesktopBar::Create(this))),
why not just initialize those in the body of ctor, as before?

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_manager.h (right):

http://codereview.chromium.org/8744005/diff/6001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:20: #endif
having UNIT_TEST around whole #include can significantly change the compiled
code. It is scary. Could we just drop UNIT_TEST here? Or add TODO that it's
temporary and you will clean it up later?