Issue 8776035: Add PanelOverflowStrip to handle panel overflow.

Issue 8776035: Add PanelOverflowStrip to handle panel overflow. (Closed)

Created:
9 years ago by jianli

Modified:
9 years ago

Reviewers:
jennb, Dmitry Titov

CC:
chromium-reviews, jennb, Dmitry Titov, jianli, prasadt, dcheng, Paweł Hajdan Jr.

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Add PanelOverflowStrip to handle panel overflow. BUG=none TEST=overflow test Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112833

Patch Set 4 : Fix linux build on trybot #

Total comments: 1

Patch Set 5 : Prepare for final landing #

Created: 9 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+522 lines, -107 lines)			Patch
M	chrome/browser/ui/panels/panel.h	View	1 2	1 chunk	+12 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel.cc	View	1 2 3 4	5 chunks	+29 lines, -26 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_frame_view.h	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_frame_view.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_view.h	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_view.cc	View	1 2	6 chunks	+49 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_browser_view_browsertest.cc	View	1	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/ui/panels/panel_browsertest.cc	View	1 2	3 chunks	+27 lines, -14 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_manager.h	View	1 2	6 chunks	+7 lines, -13 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_manager.cc	View	1 2	9 chunks	+16 lines, -15 lines	0 comments	Download
A	chrome/browser/ui/panels/panel_overflow_strip.h	View	1 2 3 4	1 chunk	+101 lines, -0 lines	0 comments	Download
A	chrome/browser/ui/panels/panel_overflow_strip.cc	View	1 2 3 4	1 chunk	+220 lines, -0 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_strip.h	View	1 2	5 chunks	+8 lines, -7 lines	0 comments	Download
M	chrome/browser/ui/panels/panel_strip.cc	View	1 2 3	8 chunks	+46 lines, -22 lines	0 comments	Download
M	chrome/chrome_browser.gypi	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download

Messages

Total messages: 7 (0 generated)

Expand Messages | Collapse Messages

Dmitry Titov

lgtm from me with a nit, you mostly need a nod from Jenn. http://codereview.chromium.org/8776035/diff/1/chrome/browser/ui/panels/panel_browser_frame_view.cc File ...

9 years ago (2011-12-02 01:56:20 UTC) #2

jennb

Sending this half first for the platform-independent layer so you can get started. Nothing major. ...

9 years ago (2011-12-02 19:15:00 UTC) #3

jennb

And the rest of it... http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/base_panel_browser_test.cc File chrome/browser/ui/panels/base_panel_browser_test.cc (right): http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/base_panel_browser_test.cc#newcode318 chrome/browser/ui/panels/base_panel_browser_test.cc:318: // Note that overflow ...

9 years ago (2011-12-02 21:16:58 UTC) #4

jennb

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/panel_browsertest.cc File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/panel_browsertest.cc#newcode118 chrome/browser/ui/panels/panel_browsertest.cc:118: Panel* panel4 = CreatePanelWithBounds( Looks like we shouldn't watch ...

9 years ago (2011-12-02 22:28:57 UTC) #5

jianli

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/base_panel_browser_test.cc File chrome/browser/ui/panels/base_panel_browser_test.cc (right): http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/base_panel_browser_test.cc#newcode318 chrome/browser/ui/panels/base_panel_browser_test.cc:318: // Note that overflow panel will not be shown ...

9 years ago (2011-12-02 23:23:46 UTC) #6

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/bas...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/bas...
chrome/browser/ui/panels/base_panel_browser_test.cc:318: // Note that overflow
panel will not be shown as active.
On 2011/12/02 21:16:58, jennb wrote:
> Let's not allow tests create panels that start out in overflow state as that's
> not possible in real usage.

Removed.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:15: #include
"chrome/browser/ui/panels/panel_overflow_strip.h"
On 2011/12/02 19:15:00, jennb wrote:
> Don't need this.

It is needed now after I added Panel::MoveOutOfOverflow.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:170: native_panel_->ShowPanelInactive();
On 2011/12/02 19:15:00, jennb wrote:
> nit: call ShowInactive() rather than direct to native_panel_ to keep code safe
> in case ShowInactive needs to do more some day.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:191: // Don't activate the panel if it is in
overflow state.
On 2011/12/02 19:15:00, jennb wrote:
> Activate() is the way an app can force it's window to be focused. Overflow
> panels should move out of overflow and activate, which doesn't require a code
> change here as SetExpansionState to EXPANDED should move panel out of
overflow.

Done. Called the newly added method MoveOutOfOverflow.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:119: // TODO(jianli): this is
just a temporary hack to check if we need to show
On 2011/12/02 21:16:58, jennb wrote:
> Could you create a crbug for this and put # here?
> 
> Long term, I'm guessing this is the API you wanted in the Panel that the
> overflow and panel strip calls from AddPanel?

Bug created. Yes, I think we need to put it in native panel.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browser_view.cc:121: int panel_strip_area_left =
On 2011/12/02 21:16:58, jennb wrote:
> Isn't it easier to just do:
> 
> ShowOrHidePanelAppIcon(panel->expansion_state() != IN_OVERFLOW) ?
> 
> nit: personally, I find DoAOrB(bool) method names hard to understand. It's
like
> when someone asks: Do you want A or B? Yes.

I rather keep it this way since we're going to clean this up.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:118: Panel* panel4 =
CreatePanelWithBounds(
On 2011/12/02 22:28:57, jennb wrote:
> Looks like we shouldn't watch for remove_delays_for_testing in AddPanel.
> Instead, test code should watch for state_changed notification to wait to be
> notified that panel is in overflow state.

Will fix in another patch.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:83: gfx::Rect
overflow_area(adjusted_work_area_);
On 2011/12/02 19:15:00, jennb wrote:
> Does overflow strip allow for any margins at the top and/or bottom of the work
> area?  I can't find the UI mocks that show the +N in overflow to check.

No per the mocks.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:109:
panel_overflow_strip_->Remove(panel);
On 2011/12/02 21:16:58, jennb wrote:
> nit: DCHECK this returns true?

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_manager.cc:202: return
panel_strip_->num_panels()+ panel_overflow_strip_->num_panels();
On 2011/12/02 19:15:00, jennb wrote:
> space before +

Done.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:16: const int
kOverflowAreaOverWidth = 200;
On 2011/12/02 21:16:58, jennb wrote:
> kOverflowStripExpandedWidth? kOverflowStripShowingTitlesWidth?  (everywhere
> else, you call it titles-showing or showing-titles. "OverWidth" doesn't convey
> any meaning to me.

Fixed. I mean kOverflowAreaHoverWidth indeed.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:18: // Maximium number of
overflow panels allowed to show.
On 2011/12/02 21:16:58, jennb wrote:
> to be shown.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:48:
panels_.insert(panels_.begin(), panel);
On 2011/12/02 21:16:58, jennb wrote:
> nit: not for this patch. vector is inefficient as every element is copied back
a
> slot every time we add/remove anything to/from the front of overflow.

Added TODO for this.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:58: for (Panels::iterator iter
= panels_.begin(); iter != panels_.end();
On 2011/12/02 21:16:58, jennb wrote:
> nit: would be easier to read if you use find() then delete found panel rather
> than for loop. This for loop made me think you were removing more than one
> panel.

I need to also find the index for the panel.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:89: size_t panel_index =
FindPanelIndex(panel);
On 2011/12/02 21:16:58, jennb wrote:
> Should make this method a no-op. Instead, in AddPanel(), call Refresh directly
> as you already know if the panel is added to the front or the back of the
strip.
> Nowhere else will call SetExpansionState(IN_OVERFLOW) so it should be safe to
> make the change.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:96: DoRefresh(0, panels_.size()
- 1);
On 2011/12/02 21:16:58, jennb wrote:
> nit: There's only a few panels visible in overflow. Would be easier to read if
> entire list is refreshed rather than save a few cycles.
> 
> The refresh logic and the way its broken up into subrs is very hard for me to
> follow. Seems like it should be as simple as:
> 
> Refresh():
> int bottom_position = display_bottom;
> for first panel to last {
>   if (current panel is within MaxVisible limit) {
>     figure out bottom, height, x and y
>     set width based on icon-only vs expanded width
>     SetPanelBounds(new bounds)
>     advance bottom_position;
>   } else {
>     make panel invisible somehow
>   }
> }
> 
> (Note, I'm following the pattern you set in panel_strip::Rearrange().)

I rather do a full layout computation to avoid any potential edge case. I also
like to update PanelStrip to follow this pattern.

Also, we might have a lot panels in overflow area, only first 6 of them are
visible.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:145: bounds.set_width(0);
On 2011/12/02 21:16:58, jennb wrote:
> Using size(0, 0) seems wrong as that means panels can have a size that is less
> than its min_size. Any other way to hide a panel? [Ok to wait til later patch
to
> find a better way.]

This is the only way for now.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:153: bool show_overflow_titles
=  ShouldShowOverflowTitles(mouse_position);
On 2011/12/02 21:16:58, jennb wrote:
> extra space after =

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:165: mouse_position.x() <=
display_area_.x() + width &&
On 2011/12/02 21:16:58, jennb wrote:
> display_area_.right()

width could be either kOverflowAreaHoverWidth or display_area_.width().

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:179: // We need to bring all
overflow panels to the top of z-order again since
On 2011/12/02 21:16:58, jennb wrote:
> This comment makes no sense to me.

Changed.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:205:
overflow_hover_animator_start_width_ > overflow_hover_animator_end_width_;
On 2011/12/02 21:16:58, jennb wrote:
> is this the same as current_width == display_area_.width()?

Yes. Thanks for suggesting the simple way to check for it.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.cc:207: // Update each overflow
panel.
On 2011/12/02 21:16:58, jennb wrote:
> Could you replace all this with a call to Refresh() when current_width ==
> overflow_hover_animator_end_width_? Maybe add a parameter to Refresh() for
> whether the panels should animate their bounds changes.

I think adding another parameter to Refresh will make it harder to understand. I
prefer to keep the animation logic separate from the refresh logic.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_overflow_strip.h (right):

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:15: namespace gfx {
On 2011/12/02 21:16:58, jennb wrote:
> Looks like I forgot to put this in panel_strip.h. Could you add?

Indeed it is not needed since I include gfx/rect.h now.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:28: typedef std::vector<Panel*>
Panels;
On 2011/12/02 21:16:58, jennb wrote:
> IWYU: #include <vector>

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:45: // Updates the restored size
of the given panel due to the preferred content
On 2011/12/02 21:16:58, jennb wrote:
> Don't need this in overflow strip. Rather, change panel_strip to stop after
> updating restored size if panel is not in the strip (expansion_state
> IN_OVERFLOW).

Removed. This is a leftover.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:49: // called when a panel's
expansion state changes.
On 2011/12/02 21:16:58, jennb wrote:
> nit: Called

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:56: bool mouse_watcher_needed()
const;
On 2011/12/02 21:16:58, jennb wrote:
> Delete, not used. (or I couldn't find it's usage). 

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:58: Panel* first_panel() const;
On 2011/12/02 21:16:58, jennb wrote:
> nit: FirstPanel(). I thought only inline accessors use the underscore style

Move the code here so we don't need to change the name.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:66: virtual void
OnMouseMove(const gfx::Point& mouse_position) OVERRIDE;
On 2011/12/02 21:16:58, jennb wrote:
> Could you make the one in panel_strip.h private too? Thx.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:72: size_t FindPanelIndex(Panel*
panel) const;
On 2011/12/02 21:16:58, jennb wrote:
> Can get rid of this. See comment in .cc file.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:75: // Empty bounds will be
returned if this is not possible due to no enough
On 2011/12/02 21:16:58, jennb wrote:
> typo: no enough

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:79: const gfx::Size&
restored_size) const;
On 2011/12/02 21:16:58, jennb wrote:
> restored_size is a concept that is only for the panel strip. Do you mean some
> other size here?

Removed. This is a leftover.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:81: // Used to pop up the titles
for overflow panels, when the mouse hovers over
On 2011/12/02 21:16:58, jennb wrote:
> nit: no comma needed in sentence.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:92: // The overflow area where
panels are iconified due to no available space
On 2011/12/02 21:16:58, jennb wrote:
> s/no available/insufficient

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_strip.h:93: // in the normal docking
area.
On 2011/12/02 21:16:58, jennb wrote:
> s/docking area/panel strip  - as we call it panel strip everywhere else.

Done.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:135: // TODO(jianli): Enable overflow
area support for other platforms.
On 2011/12/02 19:15:00, jennb wrote:
> Could you file a bug and put bug # here so we don't lose track of this TODO?

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:355: gfx::Size size =
panel->restored_size();
On 2011/12/02 19:15:00, jennb wrote:
> Should only have to modify height. Panels coming from overflow already had
their
> bounds corrected in AddPanel, which is called before this method.

I rather update both width and height since this trivial additional overhead
will guarantee we will pick up the restored size under any kind of circumstance.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:372: NOTREACHED();
On 2011/12/02 19:15:00, jennb wrote:
> nit: Don't need DCHECK up above when we have this NOTREACHED here.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:376: int bottom =
GetBottomPositionForExpansionState(panel->expansion_state());
On 2011/12/02 19:15:00, jennb wrote:
> nit: Use a local var for panel->expansion_state() as you use it here and in
the
> switch.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:594: void PanelStrip::Rearrange() {
On 2011/12/02 21:16:58, jennb wrote:
> Need to #if windows the overflow logic here too to keep mac/linux behavior the
> same.

Done.

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

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:14: #include
"chrome/browser/ui/panels/panel.h"
On 2011/12/02 21:16:58, jennb wrote:
> Could you move this before panel_mouse_watcher_observer.h for me? I misordered
> it in my patch. Thx.

Done.

http://codereview.chromium.org/8776035/diff/3002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.h:32: // |bounds| is in screen coordinates.
On 2011/12/02 21:16:58, jennb wrote:
> Could you cleanup |bounds| for me? Thx.

Done.

jennb

9 years ago (2011-12-03 00:06:53 UTC) #7

LGTM

plus fixing the includes in panel_overflow_strip.h that were put in the wrong
file or left out.

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

http://codereview.chromium.org/8776035/diff/12001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_strip.cc:94: Panel* panel,
Panel::ExpansionState old_state) {
DCHECK state is IN_OVERFLOW?

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

http://codereview.chromium.org/8776035/diff/3006/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:603: new_bounds.set_x(x);
Leave this line the way it was, but move the guard to be above 'int x' line.
That way we only need to remove the guard later and not miss fixing this line.

Expand Messages | Collapse Messages