Fix handling of minimized panel count

Fix handling of minimized panel count

Increments and decrements of minimized panel count in the docking strip would not balance out in certain scenarios, e.g.:
- if you add a panel that is already minimized (the count was incremented before the panel is added, sometimes causing a DCHECK to fire)
- if you minimize a panel that is in "temporary layout"
- if a panel's expansion state is changed when it is not in the docked strip (that is very hard to guard against, since a panel can jump into the overflow strip as a side effect of many operations)

Changed the handling of minimized panel count to be more bullet-proof; added a test

BUG=113682
TEST=PanelOverflowBrowserTest::AddMinimizedTillOverflow()

[jianli, 2012-03-12 18:08:55]

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.cc:436: for (Panels::const_iterator
i_panel = panels_.begin();
nit: normally we use iter or panel_iter.

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.h:104: int minimized_panel_count() {
return minimized_panel_count_; }
nit: add const modifier.

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

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:179: // jump from strip to strip
virtually at any moment.
Your comment seems not to be consistent with if check for docked_strip.

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:187: // DCHECK(panel->panel_strip() ==
docked_strip_.get());
Why is DCHECK commented out?

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:192: // DCHECK(panel->panel_strip() ==
docked_strip_.get());
ditto.

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

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:328:
EXPECT_FALSE(overflow_strip->overflow_indicator());
nit: this can be removed since we do not care about any change to the overflow
indicator in this test.

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:336: Panel* panel =
last_panel = CreatePanelWithParams(params);
nit: seems that one variable |panel| is enough.

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:341:
docked_strip->AddPanel(panel);
Should we call PanelManager::MovePanelToStrip?

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:345:
EXPECT_EQ(num_panels, (unsigned int)10);
nit: 10u

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:346:
EXPECT_EQ(num_panels, (unsigned int)
nit: better to use static_cast

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:351:
WaitForLayoutModeChanged(last_panel, PanelStrip::IN_OVERFLOW);
Should we also check if docked_strip->num_temporary_layout_panels() is 0?

[jennb, 2012-03-12 18:26:27]

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.cc:433: void
DockedPanelStrip::UpdateMinimizedPanelCount() {
Way more efficient to incr/decr a count than re-counting all panels. Is there a
way to ensure accuracy without losing efficiency?

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

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:177: // For panels outside of the
docked strip changing state is a no-op
Maybe reword to say "...but this method may be called while the panel is not in
the docked strip, so we need to check for that condition."

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

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:335: for (; num_panels <
10; ++num_panels) {
nit: suggest making the 10 a defined literal or a var.

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:342:
EXPECT_EQ(panel->expansion_state(), Panel::MINIMIZED);
reverse order. it's EXPECT_EQ(expected_value, actual_value).

http://codereview.chromium.org/9547007/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:351:
WaitForLayoutModeChanged(last_panel, PanelStrip::IN_OVERFLOW);
When you do RemovePanel/AddPanel, the AddPanel will ensure the panel fits in the
dock. I'm surprised the last_panel still moves to overflow...