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()

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

[Andrei, 2012-03-14 18:34:56]

On 2012/03/12 18:08:55, jianli wrote:
>
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.
> 

Changed.

>
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.
> 

Done.

>
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.
> 

Reformulated the comment.

>
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?
> 

Put in the correct logic.

>
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.
> 

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.
> 

Done.

>
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.
> 

It is more mnemonic this way, I think. Anyway, that's changed now.

>
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?
> 

The panel is created in the docked strip. I specifically wanted to test the
removing and adding logic when the panel is minimized, to make sure the count is
maintained properly. MovePaneToStrip() would probably be a no-op.

>
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
> 

Changed to a named const.

>
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
> 

Done.

>
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?

Done. 

On 2012/03/12 18:26:27, jennb wrote:
>
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?
> 

Increasing and decreasing the count from various parts of the code has been
pretty unreliable. I have found several bugs with it already, and it is prone to
bugs in general.

The "Update" method is bullet-proof: you can call at at any time and from any
place.

Since 
- this method is called on a UI action (like adding, removing or changing panel
state) there is no danger of losing much time here;
- there are not that many panels in the docked strip (probably around 10 at
max... usually 2 or 3), so the difference won't be ever noticeable, and 
- we iterate through them on multiple occasions anyway, 

I think the tradeoff is clearly justified. 

>
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."
> 

Reworded.

>
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.
> 

Done.

>
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).
> 

Done.

>
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...

So many things have changed :) After merging with Jian's patch it does not any
more -- changed to a different panel.

[Andrei, 2012-03-14 18:37:17]

Changed test to use initialized panels - this way it passes

[jennb, 2012-03-14 20:11:40]

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
chrome/browser/ui/panels/docked_panel_strip.cc:223: 
nit: delete blank line

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
chrome/browser/ui/panels/docked_panel_strip.cc:529: if
(prev_minimized_panel_count > 0 &&  minimized_panel_count_ == 0)
else if?

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

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:173: // But sicne this method may be
called for panels in other strips
typo: sicne

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:194: // We are not supposed to resize
panels in the overflow.
Overflow panels - same logic as for OnWindowAutoResized. We still need to track
their size.

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right):

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:368: // TEST FAILS WHEN
THIS LINE IS UNCOMMENTED
Delete these 2 lines.

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:373: Panel* panel7;
initialize var to avoid linux compile error

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:376: if (num_panels == 7)
panel7 = panel;
single line style not allowed

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:384: }
Formatting error.

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:386:
EXPECT_EQ(num_panels, num_panels_to_add);
reverse order of params

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:387: LOG(INFO) << "Panel
counts: docked strip: " << docked_strip->num_panels() <<
Delete before commit

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:397:
WaitForLayoutModeChanged(panel7, PanelStrip::IN_OVERFLOW);
What is the purpose of this wait? At the end of your for loop, every panel
should be in their final strip already.

http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:403: EXPECT_EQ(0,
docked_strip->num_temporary_layout_panels());
This would be true at the end of your for-loop because you've explicitly done
RemovePanel() after creating the panel. Move this line up after line 386 and you
can skip adding docked_strip->num_temporary_layout_panels() in lines 392 and
399.

[jianli, 2012-03-14 22:41:36]

http://codereview.chromium.org/9699036/diff/5003/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right):

http://codereview.chromium.org/9699036/diff/5003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:369: //
params.wait_for_fully_created = false;
Normally we should use the default true value for wait_for_fully_created. That
is, we do want to wait till the window is created. Only a very few tests rely on
setting this to false. Could you please remove comments here?

http://codereview.chromium.org/9699036/diff/5003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:373: Panel* panel7 =
NULL;
Why do we need to check 7th panel specially?

Can we split for loop into 2, like 1st for loop to create docked panels, and 2nd
for loop to create overflow panels? Do we also want to WaitForLayoutModeChanged
for each overflow panel?

http://codereview.chromium.org/9699036/diff/5003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:376: if (num_panels == 7)
panel7 = panel;
nit: split into 2 lines.

[Andrei, 2012-03-14 22:43:46]

On 2012/03/14 20:11:40, jennb wrote:
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
> File chrome/browser/ui/panels/docked_panel_strip.cc (right):
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
> chrome/browser/ui/panels/docked_panel_strip.cc:223: 
> nit: delete blank line
> 

Ok. 

>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/doc...
> chrome/browser/ui/panels/docked_panel_strip.cc:529: if
> (prev_minimized_panel_count > 0 &&  minimized_panel_count_ == 0)
> else if?
> 

Done.

>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> File chrome/browser/ui/panels/panel_manager.cc (right):
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_manager.cc:173: // But sicne this method may be
> called for panels in other strips
> typo: sicne
> 

Fixed.

>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_manager.cc:194: // We are not supposed to
resize
> panels in the overflow.
> Overflow panels - same logic as for OnWindowAutoResized. We still need to
track
> their size.
> 

Done.

>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right):
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:368: // TEST FAILS WHEN
> THIS LINE IS UNCOMMENTED
> Delete these 2 lines.
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:373: Panel* panel7;
> initialize var to avoid linux compile error
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:376: if (num_panels ==
7)
> panel7 = panel;
> single line style not allowed
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:384: }
> Formatting error.
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:386:
> EXPECT_EQ(num_panels, num_panels_to_add);
> reverse order of params
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:387: LOG(INFO) <<
"Panel
> counts: docked strip: " << docked_strip->num_panels() <<
> Delete before commit
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:397:
> WaitForLayoutModeChanged(panel7, PanelStrip::IN_OVERFLOW);
> What is the purpose of this wait? At the end of your for loop, every panel
> should be in their final strip already.
> 
>
http://codereview.chromium.org/9699036/diff/2001/chrome/browser/ui/panels/pan...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:403: EXPECT_EQ(0,
> docked_strip->num_temporary_layout_panels());
> This would be true at the end of your for-loop because you've explicitly done
> RemovePanel() after creating the panel. Move this line up after line 386 and
you
> can skip adding docked_strip->num_temporary_layout_panels() in lines 392 and
> 399.

Redid the test to work around the has_temp_layout issue and still test
minimizing panels that are in test layout.

[jennb, 2012-03-14 23:27:59]

lgtm

http://codereview.chromium.org/9699036/diff/16/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right):

http://codereview.chromium.org/9699036/diff/16/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:366: CreatePanelParams
params("Test", gfx::Rect(0, 0, 100, 100), SHOW_AS_INACTIVE);
Add comment explaining why test does not want to wait for fully created.

http://codereview.chromium.org/9699036/diff/16/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:367: 
nit: delete blank line

[commit-bot: I haz the power, 2012-03-15 16:47:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aburago@chromium.org/9699036/3009

[commit-bot: I haz the power, 2012-03-15 19:24:54]

Change committed as 126966