Move IN_OVERFLOW from Panel::ExpansionState to new enum.

Move IN_OVERFLOW from Panel::ExpansionState to new enum.

BUG=NONE
TEST=existing tests

[jennb, 2012-01-19 18:14:26]

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:35: enum StripOwner {
PanelLayout? More informative than StripOwner.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:36: // The panel is docked to the bottom of the
screen. The user might only be
For this comment and the next, I would drop the second sentence describing what
the user can/cannot do.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:42: // The panel is put into the overflow area
due to no space available in the
s/no space available/insufficient space

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:43: // normal display area.
s/normal/docked

"normal" is vague and open to interpretation.

[jennb, 2012-01-19 18:41:17]

A couple more minor things.

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

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:82: bool visible, bool
active) {
visible param not needed

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:91:
push_back(PanelData(panel, Panel::IN_OVERFLOW, Panel::EXPANDED,
Some tests will be putting minimized panels into overflow.

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

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_strip.cc:107: if (panel->expansion_state() ==
Panel::TITLE_ONLY ||
I expected to see this in the new On...Changed() handler like it used to be, but
I like this better as it's consistent with the opposite logic being in
DoRemove(). Nice move.

[jianli, 2012-01-19 22:24:40]

Also fixed the problem that clicking an overflow panel does not bring it out of
overflow. Added a new test to cover it.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel.h
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:35: enum StripOwner {
On 2012/01/19 18:14:26, jennb wrote:
> PanelLayout? More informative than StripOwner.

Renamed to LayoutState, per discussion.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:36: // The panel is docked to the bottom of the
screen. The user might only be
On 2012/01/19 18:14:26, jennb wrote:
> For this comment and the next, I would drop the second sentence describing
what
> the user can/cannot do.

Done.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:42: // The panel is put into the overflow area
due to no space available in the
On 2012/01/19 18:14:26, jennb wrote:
> s/no space available/insufficient space

Done.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel....
chrome/browser/ui/panels/panel.h:43: // normal display area.
On 2012/01/19 18:14:26, jennb wrote:
> s/normal/docked
> 
> "normal" is vague and open to interpretation.

Done.

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

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:82: bool visible, bool
active) {
On 2012/01/19 18:41:21, jennb wrote:
> visible param not needed

The overflow panel might be hidden and it comes out of overflow later. So I
think it will be nice if we also verify the visible property.

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:91:
push_back(PanelData(panel, Panel::IN_OVERFLOW, Panel::EXPANDED,
On 2012/01/19 18:41:21, jennb wrote:
> Some tests will be putting minimized panels into overflow.

Yes, but we don't care how the minimized state is saved. We only care the
previously minimized panel comes out the overflow as minimized.

[jennb, 2012-01-19 23:44:06]

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

http://codereview.chromium.org/9195003/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:82: bool visible, bool
active) {
On 2012/01/19 22:24:41, jianli wrote:
> On 2012/01/19 18:41:21, jennb wrote:
> > visible param not needed
> 
> The overflow panel might be hidden and it comes out of overflow later. So I
> think it will be nice if we also verify the visible property.

Aren't all panels in the NormalPanelDataList visible? I don't see when we would
set this to anything other than true.

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:82: if (layout_state_ == DOCKED &&
expansion_state_ == EXPANDED)
should be layout state != IN_OVERFLOW? (I can see restored_size_ applying to
DETACHED and DOCKED).

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:94: if (layout_state_ == DOCKED &&
expansion_state_ == EXPANDED)
Ditto

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:79: class
NormalPanelDataList : public PanelDataList {
nit: Rename DockedPanelDataList ?

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:91:
push_back(PanelData(panel, Panel::IN_OVERFLOW, Panel::EXPANDED,
Should comment here that expected expansion state is ignored when comparing
PanelData, or maybe we should pass in expected expansion state and change
PanelData to always compare that field, regardless of layout state.

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
panel->layout_state());
This dcheck may fail. I saw code that called SetExpansionState(EXPANDED)
followed by SetLayoutState(DOCKED). If you really want this dcheck, it should be
in SetExpansionState() instead.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:468: panel->layout_state() ==
Panel::DOCKED &&
Instead of adding DOCKED checks in this and the next section, add 

if (!DOCKED) return;

 at line 464.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:702: if (expansion_state_to_restore ==
Panel::MINIMIZED ||
Change this to !EXPANDED? Then we don't need two checks.

[jianli, 2012-01-20 01:16:07]

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:82: if (layout_state_ == DOCKED &&
expansion_state_ == EXPANDED)
On 2012/01/19 23:44:09, jennb wrote:
> should be layout state != IN_OVERFLOW? (I can see restored_size_ applying to
> DETACHED and DOCKED).

Done.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel.cc:94: if (layout_state_ == DOCKED &&
expansion_state_ == EXPANDED)
On 2012/01/19 23:44:09, jennb wrote:
> Ditto

Done.

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:79: class
NormalPanelDataList : public PanelDataList {
On 2012/01/19 23:44:09, jennb wrote:
> nit: Rename DockedPanelDataList ?

Done.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:91:
push_back(PanelData(panel, Panel::IN_OVERFLOW, Panel::EXPANDED,
On 2012/01/19 23:44:09, jennb wrote:
> Should comment here that expected expansion state is ignored when comparing
> PanelData, or maybe we should pass in expected expansion state and change
> PanelData to always compare that field, regardless of layout state. 

Added comment.

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
panel->layout_state());
On 2012/01/19 23:44:09, jennb wrote:
> This dcheck may fail. I saw code that called SetExpansionState(EXPANDED)
> followed by SetLayoutState(DOCKED). If you really want this dcheck, it should
be
> in SetExpansionState() instead.

It will not fail since PanelManager::OnPanelExpansionStateChanged will only call
this when the layout state is DOCKED. Calling SetExpansionState(...) before
SetLayoutState(DOCKED) for overflow panel will only update the internal state
without doing any further work, like changing the minimized panel count and
updating the bounds. The further work mentioned above will be performed only
when we flip the layout state from IN_OVERFLOW to DOCKED.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:468: panel->layout_state() ==
Panel::DOCKED &&
On 2012/01/19 23:44:09, jennb wrote:
> Instead of adding DOCKED checks in this and the next section, add 
> 
> if (!DOCKED) return;
> 
>  at line 464.

We also need to call Rearrange if an overflow panel is shrunken. Rewrote the
code here.

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:702: if (expansion_state_to_restore ==
Panel::MINIMIZED ||
On 2012/01/19 23:44:09, jennb wrote:
> Change this to !EXPANDED? Then we don't need two checks.

Done.

[jennb, 2012-01-20 02:01:53]

Also, I forgot to ask if you checked every place that looks at expansion state
to see if they need to also check layout state? For example, a check for is
MINIMIZED used to never be true for an overflow panel, but now it can be. I'm
worried that there may be places we forgot to update in this layout state
change.

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

http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
panel->layout_state());
On 2012/01/20 01:16:07, jianli wrote:
> On 2012/01/19 23:44:09, jennb wrote:
> > This dcheck may fail. I saw code that called SetExpansionState(EXPANDED)
> > followed by SetLayoutState(DOCKED). If you really want this dcheck, it
should
> be
> > in SetExpansionState() instead.
> 
> It will not fail since PanelManager::OnPanelExpansionStateChanged will only
call
> this when the layout state is DOCKED. Calling SetExpansionState(...) before
> SetLayoutState(DOCKED) for overflow panel will only update the internal state
> without doing any further work, like changing the minimized panel count and
> updating the bounds. The further work mentioned above will be performed only
> when we flip the layout state from IN_OVERFLOW to DOCKED.
> 

Updating the expansion state (a piece of layout data that is 'owned' by the
panel_strip) when the panel is not docked and not telling the panel_strip seems
error prone. What about changing the order so that SetLayoutState(DOCKED) is
always called before SetExpansionState(EXPANDED)?

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

http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_overflow_browsertest.cc:82: bool visible, bool
active) {
Still need to drop this 'visible' param and change line 84 to always pass true
for expected visibility.

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

http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:488: // If the panel is in overflow,
rearrange if panel's width is smaller becuase
typo: becuase

[jianli, 2012-01-20 02:15:19]

On Thu, Jan 19, 2012 at 6:01 PM, <jennb@chromium.org> wrote:

> Also, I forgot to ask if you checked every place that looks at expansion
> state
> to see if they need to also check layout state? For example, a check for is
> MINIMIZED used to never be true for an overflow panel, but now it can be.
> I'm
> worried that there may be places we forgot to update in this layout state
> change.
>
>
>
> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc>
> File chrome/browser/ui/panels/**panel_strip.cc (right):
>
> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>
browser/ui/panels/panel_strip.**cc#newcode383<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc#newcode383>
> chrome/browser/ui/panels/**panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
> panel->layout_state());
> On 2012/01/20 01:16:07, jianli wrote:
>
>> On 2012/01/19 23:44:09, jennb wrote:
>> > This dcheck may fail. I saw code that called
>>
> SetExpansionState(EXPANDED)
>
>> > followed by SetLayoutState(DOCKED). If you really want this dcheck,
>>
> it should
>
>> be
>> > in SetExpansionState() instead.
>>
>
>  It will not fail since PanelManager::**OnPanelExpansionStateChanged will
>>
> only call
>
>> this when the layout state is DOCKED. Calling SetExpansionState(...)
>>
> before
>
>> SetLayoutState(DOCKED) for overflow panel will only update the
>>
> internal state
>
>> without doing any further work, like changing the minimized panel
>>
> count and
>
>> updating the bounds. The further work mentioned above will be
>>
> performed only
>
>> when we flip the layout state from IN_OVERFLOW to DOCKED.
>>
>
>
> Updating the expansion state (a piece of layout data that is 'owned' by
> the panel_strip) when the panel is not docked and not telling the
> panel_strip seems error prone. What about changing the order so that
> SetLayoutState(DOCKED) is always called before
> SetExpansionState(EXPANDED)?
>

Yes, it sounds bad when we try to update the expansion state that is
docked's data when the panel is not docked. However, I do not want to
simply revert the order of SetExpansionState(...)
and SetLayoutState(DOCKED) because it could call SetPanelBounds twice in
some scenarios. For example, the original expansion state is MINIMIZED and
we want to bring it from OVERFLOW to DOCKED in EXPANDED state.

How about a combo call, like MakePanelDocked(Panel::ExpansionState state)?



>
> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>
browser/ui/panels/panel_**overflow_browsertest.cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc>
> File chrome/browser/ui/panels/**panel_overflow_browsertest.cc (right):
>
> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>
browser/ui/panels/panel_**overflow_browsertest.cc#**newcode82<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc#newcode82>
>
> chrome/browser/ui/panels/**panel_overflow_browsertest.cc:**82: bool
> visible,
> bool active) {
> Still need to drop this 'visible' param and change line 84 to always
> pass true for expected visibility.
>
> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc>
> File chrome/browser/ui/panels/**panel_strip.cc (right):
>
> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>
browser/ui/panels/panel_strip.**cc#newcode488<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc#newcode488>
> chrome/browser/ui/panels/**panel_strip.cc:488: // If the panel is in
> overflow, rearrange if panel's width is smaller becuase
> typo: becuase
>
>
http://codereview.chromium.**org/9195003/<http://codereview.chromium.org/9195...
>

[jennb, 2012-01-20 02:21:20]

I like the idea of combining a layout plus expansion state change into a
single call to avoid excessive bounds changing. Be sure to comment the
combined func to explain why we're doing this. I wonder if it should be
SetLayoutAndExpansionState(layout, expansion) to make it clear that we're
just changing those 2 things in one call?

On Thu, Jan 19, 2012 at 6:14 PM, Jian Li <jianli@chromium.org> wrote:

>
>
> On Thu, Jan 19, 2012 at 6:01 PM, <jennb@chromium.org> wrote:
>
>> Also, I forgot to ask if you checked every place that looks at expansion
>> state
>> to see if they need to also check layout state? For example, a check for
>> is
>> MINIMIZED used to never be true for an overflow panel, but now it can be.
>> I'm
>> worried that there may be places we forgot to update in this layout state
>> change.
>>
>>
>>
>> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc>
>> File chrome/browser/ui/panels/**panel_strip.cc (right):
>>
>> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>>
browser/ui/panels/panel_strip.**cc#newcode383<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc#newcode383>
>> chrome/browser/ui/panels/**panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
>> panel->layout_state());
>> On 2012/01/20 01:16:07, jianli wrote:
>>
>>> On 2012/01/19 23:44:09, jennb wrote:
>>> > This dcheck may fail. I saw code that called
>>>
>> SetExpansionState(EXPANDED)
>>
>>> > followed by SetLayoutState(DOCKED). If you really want this dcheck,
>>>
>> it should
>>
>>> be
>>> > in SetExpansionState() instead.
>>>
>>
>>  It will not fail since PanelManager::**OnPanelExpansionStateChanged will
>>>
>> only call
>>
>>> this when the layout state is DOCKED. Calling SetExpansionState(...)
>>>
>> before
>>
>>> SetLayoutState(DOCKED) for overflow panel will only update the
>>>
>> internal state
>>
>>> without doing any further work, like changing the minimized panel
>>>
>> count and
>>
>>> updating the bounds. The further work mentioned above will be
>>>
>> performed only
>>
>>> when we flip the layout state from IN_OVERFLOW to DOCKED.
>>>
>>
>>
>> Updating the expansion state (a piece of layout data that is 'owned' by
>> the panel_strip) when the panel is not docked and not telling the
>> panel_strip seems error prone. What about changing the order so that
>> SetLayoutState(DOCKED) is always called before
>> SetExpansionState(EXPANDED)?
>>
>
> Yes, it sounds bad when we try to update the expansion state that is
> docked's data when the panel is not docked. However, I do not want to
> simply revert the order of SetExpansionState(...)
> and SetLayoutState(DOCKED) because it could call SetPanelBounds twice in
> some scenarios. For example, the original expansion state is MINIMIZED and
> we want to bring it from OVERFLOW to DOCKED in EXPANDED state.
>
> How about a combo call, like MakePanelDocked(Panel::ExpansionState state)?
>
>
>
>>
>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>
browser/ui/panels/panel_**overflow_browsertest.cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc>
>> File chrome/browser/ui/panels/**panel_overflow_browsertest.cc (right):
>>
>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>
browser/ui/panels/panel_**overflow_browsertest.cc#**newcode82<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc#newcode82>
>>
>> chrome/browser/ui/panels/**panel_overflow_browsertest.cc:**82: bool
>> visible,
>> bool active) {
>> Still need to drop this 'visible' param and change line 84 to always
>> pass true for expected visibility.
>>
>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc>
>> File chrome/browser/ui/panels/**panel_strip.cc (right):
>>
>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>
browser/ui/panels/panel_strip.**cc#newcode488<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc#newcode488>
>> chrome/browser/ui/panels/**panel_strip.cc:488: // If the panel is in
>> overflow, rearrange if panel's width is smaller becuase
>> typo: becuase
>>
>>
http://codereview.chromium.**org/9195003/<http://codereview.chromium.org/9195...
>>
>
>

[jianli, 2012-01-20 02:32:56]

On Thu, Jan 19, 2012 at 6:21 PM, Jenn Braithwaite (胡慧鋒)
<jennb@chromium.org>wrote:

> I like the idea of combining a layout plus expansion state change into a
> single call to avoid excessive bounds changing. Be sure to comment the
> combined func to explain why we're doing this. I wonder if it should be
> SetLayoutAndExpansionState(layout, expansion) to make it clear that we're
> just changing those 2 things in one call?


Since expansion state is part of docked panel's data and we only want to
set the new expansion state when we switch from overflow/detached to
docked, I think we only need to provide a specific combo function for
setting docked state.

>
>
> On Thu, Jan 19, 2012 at 6:14 PM, Jian Li <jianli@chromium.org> wrote:
>
>>
>>
>> On Thu, Jan 19, 2012 at 6:01 PM, <jennb@chromium.org> wrote:
>>
>>> Also, I forgot to ask if you checked every place that looks at expansion
>>> state
>>> to see if they need to also check layout state? For example, a check for
>>> is
>>> MINIMIZED used to never be true for an overflow panel, but now it can
>>> be. I'm
>>> worried that there may be places we forgot to update in this layout state
>>> change.
>>>
>>>
>>>
>>> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>>>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc>
>>> File chrome/browser/ui/panels/**panel_strip.cc (right):
>>>
>>> http://codereview.chromium.**org/9195003/diff/4003/chrome/**
>>>
browser/ui/panels/panel_strip.**cc#newcode383<http://codereview.chromium.org/9195003/diff/4003/chrome/browser/ui/panels/panel_strip.cc#newcode383>
>>> chrome/browser/ui/panels/**panel_strip.cc:383: DCHECK_EQ(Panel::DOCKED,
>>> panel->layout_state());
>>> On 2012/01/20 01:16:07, jianli wrote:
>>>
>>>> On 2012/01/19 23:44:09, jennb wrote:
>>>> > This dcheck may fail. I saw code that called
>>>>
>>> SetExpansionState(EXPANDED)
>>>
>>>> > followed by SetLayoutState(DOCKED). If you really want this dcheck,
>>>>
>>> it should
>>>
>>>> be
>>>> > in SetExpansionState() instead.
>>>>
>>>
>>>  It will not fail since PanelManager::**OnPanelExpansionStateChanged
>>>> will
>>>>
>>> only call
>>>
>>>> this when the layout state is DOCKED. Calling SetExpansionState(...)
>>>>
>>> before
>>>
>>>> SetLayoutState(DOCKED) for overflow panel will only update the
>>>>
>>> internal state
>>>
>>>> without doing any further work, like changing the minimized panel
>>>>
>>> count and
>>>
>>>> updating the bounds. The further work mentioned above will be
>>>>
>>> performed only
>>>
>>>> when we flip the layout state from IN_OVERFLOW to DOCKED.
>>>>
>>>
>>>
>>> Updating the expansion state (a piece of layout data that is 'owned' by
>>> the panel_strip) when the panel is not docked and not telling the
>>> panel_strip seems error prone. What about changing the order so that
>>> SetLayoutState(DOCKED) is always called before
>>> SetExpansionState(EXPANDED)?
>>>
>>
>> Yes, it sounds bad when we try to update the expansion state that is
>> docked's data when the panel is not docked. However, I do not want to
>> simply revert the order of SetExpansionState(...)
>> and SetLayoutState(DOCKED) because it could call SetPanelBounds twice in
>> some scenarios. For example, the original expansion state is MINIMIZED and
>> we want to bring it from OVERFLOW to DOCKED in EXPANDED state.
>>
>> How about a combo call, like MakePanelDocked(Panel::ExpansionState state)?
>>
>>
>>
>>>
>>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>>
browser/ui/panels/panel_**overflow_browsertest.cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc>
>>> File chrome/browser/ui/panels/**panel_overflow_browsertest.cc (right):
>>>
>>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>>
browser/ui/panels/panel_**overflow_browsertest.cc#**newcode82<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_overflow_browsertest.cc#newcode82>
>>>
>>> chrome/browser/ui/panels/**panel_overflow_browsertest.cc:**82: bool
>>> visible,
>>> bool active) {
>>> Still need to drop this 'visible' param and change line 84 to always
>>> pass true for expected visibility.
>>>
>>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>>
browser/ui/panels/panel_strip.**cc<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc>
>>> File chrome/browser/ui/panels/**panel_strip.cc (right):
>>>
>>> http://codereview.chromium.**org/9195003/diff/2007/chrome/**
>>>
browser/ui/panels/panel_strip.**cc#newcode488<http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/panel_strip.cc#newcode488>
>>> chrome/browser/ui/panels/**panel_strip.cc:488: // If the panel is in
>>> overflow, rearrange if panel's width is smaller becuase
>>> typo: becuase
>>>
>>>
http://codereview.chromium.**org/9195003/<http://codereview.chromium.org/9195...
>>>
>>
>>
>

[prasadt, 2012-01-20 04:43:08]

drive-by

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:182: Panel::LayoutState old_state) {
Indentation off by a space.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:184:
panel_overflow_strip_->OnPanelLayoutStateChanged(panel, old_state);
This may be cleaner if the strips just listened to the notification fired by
Panel.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:190:
panel_strip_->OnPanelExpansionStateChanged(panel, old_state);
Same comment as above. PanelStrip could just listen to the notification. The
logic would seem like it belongs in PanelStrip.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_strip.cc (right):

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_strip.cc:359: Panel::LayoutState old_state) {
Indentation off by a space.

http://codereview.chromium.org/9195003/diff/23/chrome/common/chrome_notificat...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/9195003/diff/23/chrome/common/chrome_notificat...
chrome/common/chrome_notification_types.h:972:
NOTIFICATION_PANEL_CHANGED_LAYOUT_STATE,
Not in alphabetical order.

[jennb, 2012-01-20 23:28:59]

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:184:
panel_overflow_strip_->OnPanelLayoutStateChanged(panel, old_state);
On 2012/01/20 04:43:08, prasadt wrote:
> This may be cleaner if the strips just listened to the notification fired by
> Panel.

Agree that this needs to be cleaned up. Not in this patch though. The registrar
may be overkill. Maybe part of the layout manager refactor. Can discuss more
f2f.

[jianli, 2012-01-20 23:33:08]

Also checked and fixed all the usages of expansion_state() and fixed visible
parameter in PanelData.

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

http://codereview.chromium.org/9195003/diff/2007/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_strip.cc:488: // If the panel is in overflow,
rearrange if panel's width is smaller becuase
On 2012/01/20 02:01:54, jennb wrote:
> typo: becuase

Done.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:182: Panel::LayoutState old_state) {
On 2012/01/20 04:43:08, prasadt wrote:
> Indentation off by a space.

Done.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:184:
panel_overflow_strip_->OnPanelLayoutStateChanged(panel, old_state);
On 2012/01/20 04:43:08, prasadt wrote:
> This may be cleaner if the strips just listened to the notification fired by
> Panel.

Yes, it might be. But for our usage, I think it would be simpler to notify the
strip directly, instead of incurring overhead with the notification scheme.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_manager.cc:190:
panel_strip_->OnPanelExpansionStateChanged(panel, old_state);
On 2012/01/20 04:43:08, prasadt wrote:
> Same comment as above. PanelStrip could just listen to the notification. The
> logic would seem like it belongs in PanelStrip.

ExpansonState now solely belongs to PanelStrip. I removed this method since we
now call directly into PanelStrip for any expansion state changes.

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
File chrome/browser/ui/panels/panel_strip.cc (right):

http://codereview.chromium.org/9195003/diff/23/chrome/browser/ui/panels/panel...
chrome/browser/ui/panels/panel_strip.cc:359: Panel::LayoutState old_state) {
On 2012/01/20 04:43:08, prasadt wrote:
> Indentation off by a space.

Done.

http://codereview.chromium.org/9195003/diff/23/chrome/common/chrome_notificat...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/9195003/diff/23/chrome/common/chrome_notificat...
chrome/common/chrome_notification_types.h:972:
NOTIFICATION_PANEL_CHANGED_LAYOUT_STATE,
On 2012/01/20 04:43:08, prasadt wrote:
> Not in alphabetical order.

I do not think there is a requirement for this. I rather put notification for
layout state change before expansion state change since the former one takes
higher priority.