Add test cases for panel overflow handling.

Add test cases for panel overflow handling.

BUG=none
TEST=new tests


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

[jennb, 2011-12-08 23:52:00]

First off, thanks for adding tests. These have already proven helpful in
catching bugs.

I found the style used in the original overflow test to be much more readable.
You could follow each action in the test step by step. I also like what was
covered in the original test, please keep it, but move it into the body of the
test. (I don't think the test accesses anything private anymore.)

The new overflow tests cover too much per test. For example, Closing overflow
panel tests has also taken over the code from the original test for closing
normal panels. Make CloseOverflow test focus on cases involving closing an
overflow panel. I recognize there is setup cost involved with creating lots of
panels. You could reduce this by setting the testing work area to a smaller
width in each test.

I'm not sure the helper PanelData struct and PanelDataList improve code clarity.
The old style was wordier, but it was obvious what was being checked at each
stage.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:403: const
PanelManager::Panels& panels =
Use std::<vector> instead?

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:404:
PanelManager::GetInstance()->panel_strip()->panels();
Any way to get all panels and look for any panels whose bounds overlap balloons
to reposition the balloons? Balloons need to avoid overflow panels too. 
Probably needs to be reworked in a separate patch.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/base_p...
File chrome/browser/ui/panels/base_panel_browser_test.h (right):

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/base_p...
chrome/browser/ui/panels/base_panel_browser_test.h:19: // Encapsulates all the
info we care about a panel when we do the testing.
If this is all for overflow, then create a PanelOverflowTest class and put this
stuff with it. Perhaps even put all overflow tests in its own
panel_overflow_browsertest.cc file.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:239: for (int i = 0; i <
panel_overflow_strip_->num_panels(); ++i)
Using an iterator will reduce amount of code that needs to be changed when you
optimize away from vector.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:33: typedef std::vector<Panel*> Panels;
Can remove typedef now.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.cc:104: if (panels_.empty() ||
start_index == panels_.size())
Why do we need these extra checks? The condition in the for loop already catches
if there's nothing to do.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.cc:162: top_visible_panel =
num_panels() >= max_visible_panels_ ?
Why switch to num_panels()? Elsewhere in file, panels_.size() is used.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.h:61: bool
TestShouldShowOverflowTitles(const gfx::Point& mouse_position) const {
Use a TestPanelMouseWatcher instance and simulate mouse move instead of adding
API. See other tests in panel_browsertest.cc for examples.

[jianli, 2011-12-09 22:39:07]

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
File chrome/browser/notifications/balloon_collection_impl.cc (right):

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:403: const
PanelManager::Panels& panels =
On 2011/12/08 23:52:00, jennb wrote:
> Use std::<vector> instead?

Done.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/notifications/ba...
chrome/browser/notifications/balloon_collection_impl.cc:404:
PanelManager::GetInstance()->panel_strip()->panels();
On 2011/12/08 23:52:00, jennb wrote:
> Any way to get all panels and look for any panels whose bounds overlap
balloons
> to reposition the balloons? Balloons need to avoid overflow panels too. 
> Probably needs to be reworked in a separate patch.

I will rework on balloon interaction with overflow panels in a separate patch.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/base_p...
File chrome/browser/ui/panels/base_panel_browser_test.h (right):

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/base_p...
chrome/browser/ui/panels/base_panel_browser_test.h:19: // Encapsulates all the
info we care about a panel when we do the testing.
On 2011/12/08 23:52:00, jennb wrote:
> If this is all for overflow, then create a PanelOverflowTest class and put
this
> stuff with it. Perhaps even put all overflow tests in its own
> panel_overflow_browsertest.cc file.

Done.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.cc:239: for (int i = 0; i <
panel_overflow_strip_->num_panels(); ++i)
On 2011/12/08 23:52:00, jennb wrote:
> Using an iterator will reduce amount of code that needs to be changed when you
> optimize away from vector.

Done.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_manager.h:33: typedef std::vector<Panel*> Panels;
On 2011/12/08 23:52:00, jennb wrote:
> Can remove typedef now.

Done.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.cc:104: if (panels_.empty() ||
start_index == panels_.size())
On 2011/12/08 23:52:00, jennb wrote:
> Why do we need these extra checks? The condition in the for loop already
catches
> if there's nothing to do.

We need this because when last is being removed, start_index is panels_.size() -
1. However, removing that panel causes panels_.size() to be one less and thus
end_index which is panels_.size() - 1 will be also one less.

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.cc:162: top_visible_panel =
num_panels() >= max_visible_panels_ ?
On 2011/12/08 23:52:00, jennb wrote:
> Why switch to num_panels()? Elsewhere in file, panels_.size() is used.

To avoid writing sth like "static_cast<int>(panels_.size())" due to the complain
about signed/unsigned comparison.

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

http://codereview.chromium.org/8872044/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_strip.h:61: bool
TestShouldShowOverflowTitles(const gfx::Point& mouse_position) const {
On 2011/12/08 23:52:00, jennb wrote:
> Use a TestPanelMouseWatcher instance and simulate mouse move instead of adding
> API. See other tests in panel_browsertest.cc for examples.

I've already used TestPanelMouseWatcher that. But unfortunately it is not enough
to cover the cases I want to test. I want to check if the expanded overflow area
is still shown when the mouse moves to any other point within this area.

[jennb, 2011-12-10 00:42:45]

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:400:
MessageLoopForUI::current()->RunAllPending();
Think we don't really need RunAllPending here as each observer is called
immediately.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:403: std::string
BasePanelBrowserTest::GetPanelName(int index) {
MakePanelName?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
File chrome/browser/ui/panels/overflow_panel_browsertest.cc (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:22: const int
kMaxVisibleOverflow = 3;
nit: kMaxVisibleOverflowForTesting (so I don't go looking in
panel_overflow_strip.h when I see it in the code)

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:25: // We want to verify
unfinished sentence?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:29: Panel::ExpansionState
expansion_state;
prefix the things that you're checking with expected_?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:105: return
BasePanelDataList::remove(panel);
Why override if only calling base class?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:122:
(*this)[kMaxVisibleOverflow].visible = false;
Ah, so this is why you use deque, instead of std::list, which would have saved
you from reimplementing remove().

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:191: const int*
panel_widthes) {
typo: widths

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:196: for (i = 0; i <
num_normal_panels; ++i) {
i already initialized to 0

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:205: for (; i <
num_normal_panels + num_overflow_panels; ++i) {
nit: sum once and use var in conditional

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:216: // TODO(jianli):
remove the guard when overflow support is enabled on other
Or just #if this entire file for OS_WIN.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:292:
IN_PROC_BROWSER_TEST_F(OverflowPanelBrowserTest,
Log bug for this one and comment here?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:298: // Create 2 overflow
panels without waiting them to be moved to overflow.
grammar: waiting them

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:300: "Panel2",
gfx::Rect(0, 0, 255, 200), SHOW_AS_INACTIVE);
Does it need to be SHOW_AS_INACTIVE for this test to pass?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:337: Panel* panel3 =
overflow_panel_data_list[1].panel;
isn't panel3 at index 0, and panel4 at index 1?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:353:
EXPECT_EQ(normal_panel_data_list, GetAllNormalPanelData());
It doesn't look like these actually check the current expansion_state of the
panels, rather it checks that the test code sets the same expansion state each
time it builds the list. Ditto for visible and active. It only checks that the
test code computed the correct visible/active values.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:522:
WaitForPanelActiveState(panel3, SHOW_AS_ACTIVE);
Should be waiting for expansion state change to IN_OVERFLOW/EXPANDED for various
panels throughout this test.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:612:
EXPECT_TRUE(panel_overflow_strip->TestShouldShowOverflowTitles(
Should be able to use MouseMove() in this test if you can get current width of
overflow strip.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:626:
EXPECT_EQ(hover_width, panel3->GetBounds().width());
Could iterate over all panels in overflow strip to ensure width is same as strip
width and that strip width is bigger than before MouseMove.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:641:
gfx::Point(iconified_bounds5.x() + hover_width + 1,
instead of iconified.x+hover_width, could you just do
panel5->GetBounds().right()?

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_overflow_strip.h (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_strip.h:67: static const int
kOverflowAreaHoverWidth = 200;
Don't need this here if overflow strip has a notion of
current_width/height/bounds with a getter for unit_test. Might also make
overflow strip logic more straightforward, too, as discussed offline.

http://codereview.chromium.org/8872044/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:551: 'browser/ui/panels/overflow_panel_browsertest.cc',
Nit: name this panel_overflow_browsertest.cc? lines  up nicer with
panel_overflow_strip in dir listings.

[jianli, 2011-12-13 23:23:40]

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:400:
MessageLoopForUI::current()->RunAllPending();
On 2011/12/10 00:42:46, jennb wrote:
> Think we don't really need RunAllPending here as each observer is called
> immediately.

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:403: std::string
BasePanelBrowserTest::GetPanelName(int index) {
On 2011/12/10 00:42:46, jennb wrote:
> MakePanelName?

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
File chrome/browser/ui/panels/overflow_panel_browsertest.cc (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:22: const int
kMaxVisibleOverflow = 3;
On 2011/12/10 00:42:46, jennb wrote:
> nit: kMaxVisibleOverflowForTesting (so I don't go looking in
> panel_overflow_strip.h when I see it in the code)

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:25: // We want to verify
On 2011/12/10 00:42:46, jennb wrote:
> unfinished sentence?

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:29: Panel::ExpansionState
expansion_state;
On 2011/12/10 00:42:46, jennb wrote:
> prefix the things that you're checking with expected_?

I rather prefix the struct variable with expected_. See below.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:105: return
BasePanelDataList::remove(panel);
On 2011/12/10 00:42:46, jennb wrote:
> Why override if only calling base class?

Removed.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:122:
(*this)[kMaxVisibleOverflow].visible = false;
On 2011/12/10 00:42:46, jennb wrote:
> Ah, so this is why you use deque, instead of std::list, which would have saved
> you from reimplementing remove().

Yes, using list will save us from reimplmenting remove, but it will make us not
able to access the element at specific location directly.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:191: const int*
panel_widthes) {
On 2011/12/10 00:42:46, jennb wrote:
> typo: widths

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:196: for (i = 0; i <
num_normal_panels; ++i) {
On 2011/12/10 00:42:46, jennb wrote:
> i already initialized to 0

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:205: for (; i <
num_normal_panels + num_overflow_panels; ++i) {
On 2011/12/10 00:42:46, jennb wrote:
> nit: sum once and use var in conditional

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:216: // TODO(jianli):
remove the guard when overflow support is enabled on other
On 2011/12/10 00:42:46, jennb wrote:
> Or just #if this entire file for OS_WIN.

I would like to have this file compiled also for other platforms, not we do not
run the tests now.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:292:
IN_PROC_BROWSER_TEST_F(OverflowPanelBrowserTest,
On 2011/12/10 00:42:46, jennb wrote:
> Log bug for this one and comment here?

Done. Comment added at the line "#define MAYBE_CreatePanelOnDelayedOverflow
FAILS_CreatePanelOnDelayedOverflow"

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:298: // Create 2 overflow
panels without waiting them to be moved to overflow.
On 2011/12/10 00:42:46, jennb wrote:
> grammar: waiting them

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:300: "Panel2",
gfx::Rect(0, 0, 255, 200), SHOW_AS_INACTIVE);
On 2011/12/10 00:42:46, jennb wrote:
> Does it need to be SHOW_AS_INACTIVE for this test to pass? 

No. Change to SHOW_AS_ACTIVE.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:337: Panel* panel3 =
overflow_panel_data_list[1].panel;
On 2011/12/10 00:42:46, jennb wrote:
> isn't panel3 at index 0, and panel4 at index 1?

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:353:
EXPECT_EQ(normal_panel_data_list, GetAllNormalPanelData());
On 2011/12/10 00:42:46, jennb wrote:
> It doesn't look like these actually check the current expansion_state of the
> panels, rather it checks that the test code sets the same expansion state each
> time it builds the list. Ditto for visible and active. It only checks that the
> test code computed the correct visible/active values.

I added the check before we perform the 1st close action.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:522:
WaitForPanelActiveState(panel3, SHOW_AS_ACTIVE);
On 2011/12/10 00:42:46, jennb wrote:
> Should be waiting for expansion state change to IN_OVERFLOW/EXPANDED for
various
> panels throughout this test.

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:612:
EXPECT_TRUE(panel_overflow_strip->TestShouldShowOverflowTitles(
On 2011/12/10 00:42:46, jennb wrote:
> Should be able to use MouseMove() in this test if you can get current width of
> overflow strip.

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:626:
EXPECT_EQ(hover_width, panel3->GetBounds().width());
On 2011/12/10 00:42:46, jennb wrote:
> Could iterate over all panels in overflow strip to ensure width is same as
strip
> width and that strip width is bigger than before MouseMove.

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_browsertest.cc:641:
gfx::Point(iconified_bounds5.x() + hover_width + 1,
On 2011/12/10 00:42:46, jennb wrote:
> instead of iconified.x+hover_width, could you just do
> panel5->GetBounds().right()?

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_overflow_strip.h (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_strip.h:67: static const int
kOverflowAreaHoverWidth = 200;
On 2011/12/10 00:42:46, jennb wrote:
> Don't need this here if overflow strip has a notion of
> current_width/height/bounds with a getter for unit_test. Might also make
> overflow strip logic more straightforward, too, as discussed offline.

Done.

http://codereview.chromium.org/8872044/diff/10001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8872044/diff/10001/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:551: 'browser/ui/panels/overflow_panel_browsertest.cc',
On 2011/12/10 00:42:46, jennb wrote:
> Nit: name this panel_overflow_browsertest.cc? lines  up nicer with
> panel_overflow_strip in dir listings.

Done.

[jennb, 2011-12-14 00:55:29]

I'll take a more detailed look at the tests after these major points are fixed
as they'll change the code quite a bit. Would be good if you can find a way to
make it less likely that an indexing error will occur. Both the previous
revision and this revision suffer from indexing errors.

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

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:310: Panel* panel2 =
CreatePanelWithBounds("Panel2", gfx::Rect(0, 0, 255, 200));
CreatePanelWithBounds invokes MessageLoop RunAllPending because it waits on
signals, which may give the panel enough time to move to overflow before this
code has a chance to continue as the timeouts are shortened in tests. You need
to manually create the overflow panels instead of using the base class helper
func to keep all this executing without interruptions.

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:362:
EXPECT_EQ(expected_normal_panel_data_list, GetAllNormalPanelData());
You're still not comparing the actual values inside the Panel objects. Building
the expected values in the few lines above simply repeats the logic in
GetAllNormalPanelData() and GetAllOverflowPanelData() where you also construct
expected values in the expansion_state and visible fields. Look at the two
push_back() methods above and you'll see the actual Panel data always gets
over-written by expected values. push_front() is also  like that.

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:375:
expected_overflow_panel_data_list.remove(panels[6]);
list does not have index 6. Indexing mistakes throughout code below.

[jianli, 2011-12-14 22:36:19]

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

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:310: Panel* panel2 =
CreatePanelWithBounds("Panel2", gfx::Rect(0, 0, 255, 200));
On 2011/12/14 00:55:29, jennb wrote:
> CreatePanelWithBounds invokes MessageLoop RunAllPending because it waits on
> signals, which may give the panel enough time to move to overflow before this
> code has a chance to continue as the timeouts are shortened in tests. You need
> to manually create the overflow panels instead of using the base class helper
> func to keep all this executing without interruptions.

Done. Added wait_for_fully_created to CreatePanelParams.

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:362:
EXPECT_EQ(expected_normal_panel_data_list, GetAllNormalPanelData());
On 2011/12/14 00:55:29, jennb wrote:
> You're still not comparing the actual values inside the Panel objects.
Building
> the expected values in the few lines above simply repeats the logic in
> GetAllNormalPanelData() and GetAllOverflowPanelData() where you also construct
> expected values in the expansion_state and visible fields. Look at the two
> push_back() methods above and you'll see the actual Panel data always gets
> over-written by expected values. push_front() is also  like that.

I changed to construct the expected list each time when we do the verification.

http://codereview.chromium.org/8872044/diff/2002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:375:
expected_overflow_panel_data_list.remove(panels[6]);
On 2011/12/14 00:55:29, jennb wrote:
> list does not have index 6. Indexing mistakes throughout code below.

Both panels and expected_overflow_panel_data_list are filled with the panel data
before Panel6 is closed. So we need to adjust expected_overflow_panel_data_list
to remove Panel6's data from it. However, I changed to construct the expected
list each time when we do the verification.

[jennb, 2011-12-15 01:40:27]

LGTM

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_overflow_browsertest.cc (right):

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:257: "Panel3",
gfx::Rect(0, 0, 255, 200), SHOW_AS_INACTIVE);
smaller size

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:490:
panel_overflow_strip->set_max_visible_panels(3);
Delete. Already done in setup. Can also delete panel_overflow_strip var. Check
rest of file too.

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:588: // Activate an
overflow-on-overflow panel. Not any normal panel is swapped
Grammar: Not any

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_overflow_strip.cc (right):

http://codereview.chromium.org/8872044/diff/21013/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_strip.cc:140: if
(are_overflow_titles_shown_) {
Could combine this with conditional in else-if:

are_overflow_titles_shown || ... < max_visible

[commit-bot: I haz the power, 2011-12-15 01:59:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/8872044/28004

[commit-bot: I haz the power, 2011-12-15 03:25:22]

Change committed as 114590