Panel strip refactor cleanup.

Panel strip refactor cleanup.

* Cleanup test-only includes.
* Move some logic from panels into panel strip.
* Split DragPanels test into three tests.
* SetExpansionState on panel to move in/out of overflow.
* Added API to panel/native_panel to control whether app icon is visible.

BUG=106227
TEST=Panel*Test.*


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

[jennb, 2011-12-06 23:45:17]

Not ready for review yet. Just sending this so you have an idea of where I'm
cleaning up. I am still looking for a way to get rid of
remove_delays_for_testing so I can remove the test-only include in
panel_manager.h.

[jennb, 2011-12-07 20:04:10]

Ready for review.

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

http://codereview.chromium.org/8827011/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:882: 
Oops. will delete this extra line.

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

http://codereview.chromium.org/8827011/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:656: DCHECK(panel->temporary_layout());
Oops. will move this  dcheck into the loop in case panel no longer a valid
pointer.

[jianli, 2011-12-07 22:13:26]

Can you also add the logic to fix the problem that the newly created overflow
panel can be dragged when it is not moved into the overflow strip?

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.h:281: bool temporary_layout_;
Probably better to say has_temporary_layout_. The name "temporary_layout_" makes
me think this stores the temporary layout info, instead of a boolean flag. Or,
maybe we can call it as layout_finalized_.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:530: if (base::TimeTicks::Now() -
mouse_pressed_time_ >
Do we want to skip expanding an overflow panel if it is a long click? If so,
please update the comment. Otherwise, please fix the code.

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

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:92: void
SetMouseWatcher(PanelMouseWatcher* watcher);
Since this is used only for testing purpose, can you move it inside UNIT_TEST
guard?

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_overflow_strip.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:48: if
(panel->temporary_layout()) {
Could you please add comment to explain why we insert panel differently, like:
 // The newly created panel that has unfinalied layout should
be added to the back. For other cases, it should be inserted to the front.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_strip.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:199: panel->expansion_state() ==
Panel::MINIMIZED)
indentation.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:461: // the panel that pops up the
titlebar to attract   attention.
extra spaces after "attract"

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:636: // Move panels to overflow in
reverse to maintain their order.
Better to add more comment to explain that we do not have enough space due to
the shrunken work area or growing panel.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:637: for (size_t overflow_index =
panels_.size() - 1;
Either add DCHECK to ensure panel_index is not 0, or cast overflow_index to int.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:646:
overflow_panel->restored_size().width() >= display_area_.x()) {
4 more spaces before this line.

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

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:126: // the panel strip to the overflow
area because the panel could not fit
overflow area => overflow strip

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:128: // in the panel strip, even if it
would overflow, then moved to the overflow
Can you remove "even if it would overflow" since it seems to be quite confusing
to me? Consider adding "temporarily" after "in the panel strip".

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:131: void
MovePanelToOverflowAfterDelay(Panel* panel);
DelayedMovePanelToOverflow?

[jennb, 2011-12-07 23:35:40]

As discussed offline, draggable change postponed to separate patch.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.h:281: bool temporary_layout_;
On 2011/12/07 22:13:26, jianli wrote:
> Probably better to say has_temporary_layout_. The name "temporary_layout_"
makes
> me think this stores the temporary layout info, instead of a boolean flag. Or,
> maybe we can call it as layout_finalized_.

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:530: if (base::TimeTicks::Now() -
mouse_pressed_time_ >
On 2011/12/07 22:13:26, jianli wrote:
> Do we want to skip expanding an overflow panel if it is a long click? If so,
> please update the comment. Otherwise, please fix the code.

Will update the comment. All long clicks will be ignored.

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

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:92: void
SetMouseWatcher(PanelMouseWatcher* watcher);
On 2011/12/07 22:13:26, jianli wrote:
> Since this is used only for testing purpose, can you move it inside UNIT_TEST
> guard?

Moved to private and made tests friends of this class. Cannot have decl in ifdef
UNIT_TEST but body not-inline.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_overflow_strip.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:48: if
(panel->temporary_layout()) {
On 2011/12/07 22:13:26, jianli wrote:
> Could you please add comment to explain why we insert panel differently, like:
>  // The newly created panel that has unfinalied layout should
> be added to the back. For other cases, it should be inserted to the front.

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_strip.cc (right):

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:199: panel->expansion_state() ==
Panel::MINIMIZED)
On 2011/12/07 22:13:26, jianli wrote:
> indentation.

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:461: // the panel that pops up the
titlebar to attract   attention.
On 2011/12/07 22:13:26, jianli wrote:
> extra spaces after "attract"

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:636: // Move panels to overflow in
reverse to maintain their order.
On 2011/12/07 22:13:26, jianli wrote:
> Better to add more comment to explain that we do not have enough space due to
> the shrunken work area or growing panel.

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:637: for (size_t overflow_index =
panels_.size() - 1;
On 2011/12/07 22:13:26, jianli wrote:
> Either add DCHECK to ensure panel_index is not 0, or cast overflow_index to
int.

Done.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:646:
overflow_panel->restored_size().width() >= display_area_.x()) {
On 2011/12/07 22:13:26, jianli wrote:
> 4 more spaces before this line.

Done.

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

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:128: // in the panel strip, even if it
would overflow, then moved to the overflow
On 2011/12/07 22:13:26, jianli wrote:
> Can you remove "even if it would overflow" since it seems to be quite
confusing
> to me? Consider adding "temporarily" after "in the panel strip".

Reworded comment.

http://codereview.chromium.org/8827011/diff/5001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:131: void
MovePanelToOverflowAfterDelay(Panel* panel);
On 2011/12/07 22:13:26, jianli wrote:
> DelayedMovePanelToOverflow?

Done.

[jianli, 2011-12-08 00:27:54]

LGTM w/ a couple nits.

http://codereview.chromium.org/8827011/diff/9002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/8827011/diff/9002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:529: // Ignore long clicks.
Treated as a canceled click to be consistent with Mac.
cancelled?

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

http://codereview.chromium.org/8827011/diff/9002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.h:112: friend class PanelBrowserTest;  //
for SetMouseWatcher()
As discussed, we can do sth. similar to SetWorkAreaForTesting.

[commit-bot: I haz the power, 2011-12-08 00:56:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8827011/19001

[commit-bot: I haz the power, 2011-12-08 02:23:08]

Can't apply patch for file chrome/browser/ui/panels/panel_browser_view.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/panels/panel_browser_view.cc
Hunk #1 FAILED at 9.
Hunk #2 succeeded at 117 (offset -33 lines).
Hunk #3 succeeded at 468 (offset -51 lines).
Hunk #4 succeeded at 477 (offset -51 lines).
Hunk #5 succeeded at 506 (offset -51 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/panels/panel_browser_view.cc.rej

[commit-bot: I haz the power, 2011-12-08 20:26:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8827011/24001

[commit-bot: I haz the power, 2011-12-08 23:24:39]

Change committed as 113691