Panel Overflow on Mac.

Panel Overflow on Mac.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120791

[Dmitry Titov, 2012-01-25 07:28:32]

This is first patch, does not have correct rendering of titlebars yet and no
overflow indicator.

[Dmitry Titov, 2012-01-25 17:26:38]

Please don't review it yet. I'll fix trybot issues and rename couple of things.

[Dmitry Titov, 2012-02-04 01:29:33]

I think this can be landed. I've implemented minimal support for overflow and
enabled it on Mac. I had to add a new NativePanel::ApplyVisualStyleForStrip()
method in order to make one-time changes to visual style when panel moves
between strips.

Still todo, for subsequent patches:
- add more code to OSX version of ApplyVisualStyleForStrip, to remove shadow in
overflow for example.
- Fix rendering of panels in overflow (currently shows close buttons for all of
them)
- Add overflow indicator

[jennb, 2012-02-07 00:15:31]

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:366: if (old_state ==
Panel::IN_OVERFLOW) {
Note that we do a lot of adjustments for a state change from overflow here (also
in OverflowPanelStrip). Would it be better to adjust visual style of panel here
rather than AddPanel so you don't have to call it on every panel being added?

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.h (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.h:109: // "IN_OVERFLOW"
state.
Should take actual state name out of comment as we're changing the name and that
it's an expansion state.

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:141: // Methods to
support "IN_OVERFLOW" mode of the panel.
ditto

[Dmitry Titov, 2012-02-07 00:28:26]

Also enabled tests that don't depend on overflow counter.

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:366: if (old_state ==
Panel::IN_OVERFLOW) {
On 2012/02/07 00:15:31, jennb wrote:
> Note that we do a lot of adjustments for a state change from overflow here
(also
> in OverflowPanelStrip). Would it be better to adjust visual style of panel
here
> rather than AddPanel so you don't have to call it on every panel being added?

I think we'll change this soon because going from strip to strip will be
separated from changing the expansion states in the DockPanelStrip, right?
Also, the semantics of the method I'm adding is more of "apply style specific
for the particular strip', basically 'onAddedToStrip'... So I call it form
AddPanel - to better follow the implied semantics. I had 2 methods in earlier
version of the patch - EnterOverflow/ExitOverflow but it was too weird,
actually, because it wasn't clear what should be done in them, while I can see
useful things we can do on a panel when it goes from one strip to another.

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.h (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.h:109: // "IN_OVERFLOW"
state.
On 2012/02/07 00:15:31, jennb wrote:
> Should take actual state name out of comment as we're changing the name and
that
> it's an expansion state.

Done.

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.h (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.h:141: // Methods to
support "IN_OVERFLOW" mode of the panel.
On 2012/02/07 00:15:31, jennb wrote:
> ditto

Done.

[jennb, 2012-02-07 00:50:31]

LGTM unless Prasad has a complaint about test enabling.

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9129013/diff/11001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:366: if (old_state ==
Panel::IN_OVERFLOW) {
On 2012/02/07 00:28:26, Dmitry Titov wrote:
> On 2012/02/07 00:15:31, jennb wrote:
> > Note that we do a lot of adjustments for a state change from overflow here
> (also
> > in OverflowPanelStrip). Would it be better to adjust visual style of panel
> here
> > rather than AddPanel so you don't have to call it on every panel being
added?
> 
> I think we'll change this soon because going from strip to strip will be
> separated from changing the expansion states in the DockPanelStrip, right?
> Also, the semantics of the method I'm adding is more of "apply style specific
> for the particular strip', basically 'onAddedToStrip'... So I call it form
> AddPanel - to better follow the implied semantics. I had 2 methods in earlier
> version of the patch - EnterOverflow/ExitOverflow but it was too weird,
> actually, because it wasn't clear what should be done in them, while I can see
> useful things we can do on a panel when it goes from one strip to another.

The plan is to have Panel::MoveToStrip(new_strip). Maybe we'll move all this
logic in there in that patch.

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

http://codereview.chromium.org/9129013/diff/19020/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:190: #if defined(OS_WIN)
|| defined(OS_MACOSX)
I wonder if this will make it hard on Prasad to enable tests for Linux. Will he
have to further break these into additional subsections?

[Dmitry Titov, 2012-02-07 00:53:09]

http://codereview.chromium.org/9129013/diff/19020/chrome/browser/ui/panels/pa...
> chrome/browser/ui/panels/panel_overflow_browsertest.cc:190: #if
defined(OS_WIN)
> || defined(OS_MACOSX)
> I wonder if this will make it hard on Prasad to enable tests for Linux. Will
he
> have to further break these into additional subsections?

I suspect he will have the same set of tests enabled in his initial overflow
patch :-) But if he needs to disable one or two more, it's always possible to
move them out from this 'block'.

[prasadt, 2012-02-07 00:54:21]

lgtm for Jenn's query.

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

http://codereview.chromium.org/9129013/diff/19020/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:190: #if defined(OS_WIN)
|| defined(OS_MACOSX)
On 2012/02/07 00:50:31, jennb wrote:
> I wonder if this will make it hard on Prasad to enable tests for Linux. Will
he
> have to further break these into additional subsections?

That's fine. I'll just fix-up as necessary. Shouldn't be bad.

[commit-bot: I haz the power, 2012-02-07 01:03:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dimich@chromium.org/9129013/19020

[commit-bot: I haz the power, 2012-02-07 04:34:25]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120677, job name
was 9129013-19020 (retry) (previous was lost) (previous was lost) (previous was
lost).