Refactor intra-strip panel drags by introducing PanelDragController.

Refactor intra-strip panel drags by introducing PanelDragController.

Changes:
1) Add PanelDragController that is created and owned by PanelManager. Currently it only contains the logic for intra-strip drags.
2) Add dragging related base methods to PanelStrip.
3) PanelManager dragging methods will now call PanelDragController and then delegate to PanelStrip methods.
4) Add delta_y parameter to Drag method.
5) Remove is_draggble from Panel and replace it with PanelStrip::CanDragPanel.

BUG=none
TEST=new tests to cover detached panels


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

[jennb, 2012-02-16 22:31:00]

Initial round of comments, mostly about class interface/interactions.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.h:58: Panel* dragging_panel_;
This is already tracked in drag controller. Do we really need to track it again
in both this strip and the docked strip?

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.h:62: gfx::Point
dragging_panel_original_position_;
What is position? the x,y coordinates of the panel's bounds? Would it make sense
to simply store original bounds?

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/overflow_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/overflow_panel_strip.cc:190: // Nothing to do.
NOTREACHED() for all three methods? Will ensure we don't accidentally call into
the overflow strip.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_drag_controller.h:21: bool CanDrag(Panel* panel)
const;
Doesn't seem like we need this here as the current panel strip can be asked
directly if a panel can be dragged.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_drag_controller.h:30: Panel* dragging_panel_;
Seems like panel's original bounds and original strip should also be tracked
here.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_manager.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_manager.h:50: bool CanDrag(Panel* panel) const;
Do we want to allow panels to know about drag controller? If so, then we can
leave these methods out of PanelManager and have panel call the drag controller
methods directly.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_strip.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_strip.h:71: // DragPanel or StartDraggingPanel is
called.
Not sure I understand the comment because there's no mouse position passed to
StartDraggingPanel() nor DragPanel().

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/chrome_tests....
chrome/chrome_tests.gypi:569: 'browser/ui/panels/detached_panel_browsertest.cc',
should also add this at the other place where overflow tests are added in this
file for exclusion.

[jianli, 2012-02-16 23:05:52]

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.h:58: Panel* dragging_panel_;
On 2012/02/16 22:31:00, jennb wrote:
> This is already tracked in drag controller. Do we really need to track it
again
> in both this strip and the docked strip?

They're served for different purpose. PanelDragController::dragging_panel_
represents the panel being dragged, no matter whether it is intra-strip or
inter-strip. It is always valid when there is a drag.
PanelStrip::dragging_panel_ denotes the panel being dragged inside this strip.
It will only be valid when the drag enters this strip.

Though we could still try to use draagging_panel_ from drag controller, but it
would require checking if the strip that the dragging is on is this strip, which
is a bit complicated. I rather have each strip know about all the info to
process intra-strip drags.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.h:62: gfx::Point
dragging_panel_original_position_;
On 2012/02/16 22:31:00, jennb wrote:
> What is position? the x,y coordinates of the panel's bounds? Would it make
sense
> to simply store original bounds?

The origin (x, y coordinates) of the panel's bounds. We only intend to use the
origin. So I rather not to save extra info that was not used.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/overflow_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/overflow_panel_strip.cc:190: // Nothing to do.
On 2012/02/16 22:31:00, jennb wrote:
> NOTREACHED() for all three methods? Will ensure we don't accidentally call
into
> the overflow strip.

Done.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_drag_controller.h:21: bool CanDrag(Panel* panel)
const;
On 2012/02/16 22:31:00, jennb wrote:
> Doesn't seem like we need this here as the current panel strip can be asked
> directly if a panel can be dragged.

Done.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_drag_controller.h:30: Panel* dragging_panel_;
On 2012/02/16 22:31:00, jennb wrote:
> Seems like panel's original bounds and original strip should also be tracked
> here.

Yes, it will be added when we handle inter-strip drags.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_manager.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_manager.h:50: bool CanDrag(Panel* panel) const;
On 2012/02/16 22:31:00, jennb wrote:
> Do we want to allow panels to know about drag controller? If so, then we can
> leave these methods out of PanelManager and have panel call the drag
controller
> methods directly.

Panels should only know about PanelManager, not drag controller.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
File chrome/browser/ui/panels/panel_strip.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/browser/ui/pa...
chrome/browser/ui/panels/panel_strip.h:71: // DragPanel or StartDraggingPanel is
called.
On 2012/02/16 22:31:00, jennb wrote:
> Not sure I understand the comment because there's no mouse position passed to
> StartDraggingPanel() nor DragPanel().

We only care about how much the mouse has been moved since last time DragPanel
or StartDraggingPanel is called. The underlying code might choose to apply or
not apply these delta values to the origin of the panel being dragged. I've
updated the comment to make it clearer.

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/chrome_tests....
File chrome/chrome_tests.gypi (right):

https://chromiumcodereview.appspot.com/9403035/diff/5001/chrome/chrome_tests....
chrome/chrome_tests.gypi:569: 'browser/ui/panels/detached_panel_browsertest.cc',
On 2012/02/16 22:31:00, jennb wrote:
> should also add this at the other place where overflow tests are added in this
> file for exclusion.

Done.

[Andrei, 2012-02-17 00:08:18]

https://chromiumcodereview.appspot.com/9403035/diff/5003/chrome/browser/ui/pa...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/5003/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.cc:110: }
It looks like cancelling the drag will happen very differently depending on
whether this panel changed strips during drag or not. If the drag was within one
strip, you simply call EndDraggingPanel() with cancelled=true. If strips were
changed, the controller has to remove/add it to where it started from, and to
use its own copy of dragging panel's original position. Is that so? Maybe we can
have the controller do the job in both cases?

[jianli, 2012-02-17 00:15:12]

https://chromiumcodereview.appspot.com/9403035/diff/5003/chrome/browser/ui/pa...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/5003/chrome/browser/ui/pa...
chrome/browser/ui/panels/detached_panel_strip.cc:110: }
On 2012/02/17 00:08:18, Andrei wrote:
> It looks like cancelling the drag will happen very differently depending on
> whether this panel changed strips during drag or not. If the drag was within
one
> strip, you simply call EndDraggingPanel() with cancelled=true. If strips were
> changed, the controller has to remove/add it to where it started from, and to
> use its own copy of dragging panel's original position. Is that so? Maybe we
can
> have the controller do the job in both cases?

Yes. Inter-strip drags are not included in this patch and I only assume the
drags are within this trip. When we add handling for inter-strip drags,
PanelDragController might do the job for both cases.

[jennb, 2012-02-17 21:28:45]

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/detached_panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_browsertest.cc:31: EXPECT_EQ(2,
detached_strip->num_panels());
Also check panel_manager->panels() includes the detached panels. Would have
caught the oversight if we had coverage. ;-)

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_browsertest.cc:103: panel->Close();
Close() is async and will not have immediate effect on num_panels.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.cc:92: void
DetachedPanelStrip::DragPanel(int delta_x, int delta_y) {
Maybe we should pass the dragging_panel into this method and EndDraggingPanel?

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.cc:97: new_bounds.Offset(delta_x,
delta_y);
Do we need to adjust the deltas to keep the panel on screen? Mouse position
could be on screen, but actual panel might be partially off screen.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.h:59: gfx::Point
dragging_panel_original_position_;
This feels like it belongs in drag controller. 

Also, which coordinate system?

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:214: refresh_layout = false;
Keep this 'return true'. No need for refresh_layout temp var.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:226:
panel_manager_->OnPanelRemoved(panel);
Move this right after erase(iter) so notification is only sent out for regular
docked panels. Don't change existing behavior in this patch.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:509: Panel* dragging_panel =
shouldn't this be:

if (state == title_only && drag_controller()->dragging_panel() == panel)
  return true?

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel.cc:208: if
(manager()->drag_controller()->CanRemovePanel(this))
Continue as normal with close here. When panelmanger::remove() is called,
panelmanager can then check with drag controller. All that delayed remove logic
should be hidden from the regular panel close logic.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browsertest.cc:806: panel->Close();
Close is async so this will always continue with num_panels unchanged. May have
to rethink how to test delayed removal.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:20: DCHECK(!dragging_panel_);
Also DCHECK panel->panel_strip()->CanDragPanel(panel)?

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:48: bool can_remove = (panel
!= dragging_panel_) &&
if (panel == dragging_panel_ || is_a_docked_panel)
   put in pending list
   return false

return true

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:60: (*iter)->Close();
The existing logic allows Close() while dragging. Only the remove() part is
delayed. Is it really necessary to change the existing design? After all,
Close() might not actually close a window.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.h:15: // Responsible for handling
drags initiated for all panel strips, including
s/panel strips/panels  ?

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:268: std::vector<Panel*>
PanelManager::panels() const {
Need to include detached panels here.

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

https://chromiumcodereview.appspot.com/9403035/diff/13001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:402: return
windowShim_->panel()->manager()->CanDrag(windowShim_->panel());
Wow, this looks ugly. We should leave draggable() on Panel class and have it
simply call panel_strip_->CanDrag(this). Get rid of CanDrag from panelmanager
class, too.

[jianli, 2012-02-17 23:52:56]

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
File chrome/browser/ui/panels/detached_panel_browsertest.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_browsertest.cc:31: EXPECT_EQ(2,
detached_strip->num_panels());
On 2012/02/17 21:28:45, jennb wrote:
> Also check panel_manager->panels() includes the detached panels. Would have
> caught the oversight if we had coverage. ;-)

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_browsertest.cc:103: panel->Close();
On 2012/02/17 21:28:45, jennb wrote:
> Close() is async and will not have immediate effect on num_panels.

Removed. Will include in future patch.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
File chrome/browser/ui/panels/detached_panel_strip.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:92: void
DetachedPanelStrip::DragPanel(int delta_x, int delta_y) {
On 2012/02/17 21:28:45, jennb wrote:
> Maybe we should pass the dragging_panel into this method and EndDraggingPanel?

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:97: new_bounds.Offset(delta_x,
delta_y);
On 2012/02/17 21:28:45, jennb wrote:
> Do we need to adjust the deltas to keep the panel on screen? Mouse position
> could be on screen, but actual panel might be partially off screen.

The detached panel should be allowed at any position, even some of its area is
off-screen.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
File chrome/browser/ui/panels/detached_panel_strip.h (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.h:59: gfx::Point
dragging_panel_original_position_;
On 2012/02/17 21:28:45, jennb wrote:
> This feels like it belongs in drag controller. 
> 
> Also, which coordinate system?

Done.

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

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:214: refresh_layout = false;
On 2012/02/17 21:28:45, jennb wrote:
> Keep this 'return true'. No need for refresh_layout temp var.

Per discussion, remove the change from this patch.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:226:
panel_manager_->OnPanelRemoved(panel);
On 2012/02/17 21:28:45, jennb wrote:
> Move this right after erase(iter) so notification is only sent out for regular
> docked panels. Don't change existing behavior in this patch.

Per discussion, remove the change from this patch.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:509: Panel* dragging_panel =
On 2012/02/17 21:28:45, jennb wrote:
> shouldn't this be:
> 
> if (state == title_only && drag_controller()->dragging_panel() == panel)
>   return true?
> 

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:208: if
(manager()->drag_controller()->CanRemovePanel(this))
On 2012/02/17 21:28:45, jennb wrote:
> Continue as normal with close here. When panelmanger::remove() is called,
> panelmanager can then check with drag controller. All that delayed remove
logic
> should be hidden from the regular panel close logic.

Per discussion, removed.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browsertest.cc:806: panel->Close();
On 2012/02/17 21:28:45, jennb wrote:
> Close is async so this will always continue with num_panels unchanged. May
have
> to rethink how to test delayed removal.

ditto.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:20: DCHECK(!dragging_panel_);
On 2012/02/17 21:28:45, jennb wrote:
> Also DCHECK panel->panel_strip()->CanDragPanel(panel)?

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:48: bool can_remove = (panel
!= dragging_panel_) &&
On 2012/02/17 21:28:45, jennb wrote:
> if (panel == dragging_panel_ || is_a_docked_panel)
>    put in pending list
>    return false
> 
> return true

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:60: (*iter)->Close();
On 2012/02/17 21:28:45, jennb wrote:
> The existing logic allows Close() while dragging. Only the remove() part is
> delayed. Is it really necessary to change the existing design? After all,
> Close() might not actually close a window.

Per discussion, I remove it from this patch.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:15: // Responsible for handling
drags initiated for all panel strips, including
On 2012/02/17 21:28:45, jennb wrote:
> s/panel strips/panels  ?

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_manager.cc (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_manager.cc:268: std::vector<Panel*>
PanelManager::panels() const {
On 2012/02/17 21:28:45, jennb wrote:
> Need to include detached panels here.

Done.

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/9403035/diff/13001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:402: return
windowShim_->panel()->manager()->CanDrag(windowShim_->panel());
On 2012/02/17 21:28:45, jennb wrote:
> Wow, this looks ugly. We should leave draggable() on Panel class and have it
> simply call panel_strip_->CanDrag(this). Get rid of CanDrag from panelmanager
> class, too.

Done.

[Andrei, 2012-02-18 19:41:30]

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:113:
overflow_strip_->SetDisplayArea(overflow_area);
Should not we take care of the detached strip here as well? (I am not 100% sure
where you patch stops, and what is planned for the future, but) since you have
introduced the detached strip member, and use it in some functions, you need to
keep it in a consistent state.

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:179:
docked_strip_->OnPanelExpansionStateChanged(panel);
Should it be panel->panel_strip()->OnPanelExpansionStateChanged(panel) ?

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:188:
docked_strip_->ResizePanelWindow(panel, preferred_window_size);
Same as above for this and the next 3 functions

[Andrei, 2012-02-18 21:31:12]

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:151:
overflow_strip_->OnFullScreenModeChanged(is_full_screen_);
Notify the detached strip here as well

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:247:
docked_strip_->OnAutoHidingDesktopBarVisibilityChanged(alignment, visibility);
May other strips, detached and overflow, need to know as well?

[jianli, 2012-02-21 19:14:32]

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:179:
docked_strip_->OnPanelExpansionStateChanged(panel);
On 2012/02/18 19:41:31, Andrei wrote:
> Should it be panel->panel_strip()->OnPanelExpansionStateChanged(panel) ?

Yes, we need to fix this. I rather fix it later when we can really create a
detached panel for testing.

https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_manager.cc:188:
docked_strip_->ResizePanelWindow(panel, preferred_window_size);
On 2012/02/18 19:41:31, Andrei wrote:
> Same as above for this and the next 3 functions

Yes. But I rather delay the fix as mentioned above.

[Andrei, 2012-02-21 19:19:43]

On 2012/02/21 19:14:32, jianli wrote:
>
https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
> File chrome/browser/ui/panels/panel_manager.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
> chrome/browser/ui/panels/panel_manager.cc:179:
> docked_strip_->OnPanelExpansionStateChanged(panel);
> On 2012/02/18 19:41:31, Andrei wrote:
> > Should it be panel->panel_strip()->OnPanelExpansionStateChanged(panel) ?
> 
> Yes, we need to fix this. I rather fix it later when we can really create a
> detached panel for testing.
> 
>
https://chromiumcodereview.appspot.com/9403035/diff/13029/chrome/browser/ui/p...
> chrome/browser/ui/panels/panel_manager.cc:188:
> docked_strip_->ResizePanelWindow(panel, preferred_window_size);
> On 2012/02/18 19:41:31, Andrei wrote:
> > Same as above for this and the next 3 functions
> 
> Yes. But I rather delay the fix as mentioned above.

Ok, LGTM!

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

LGTM

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

http://codereview.chromium.org/9403035/diff/13029/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:221: if
(panel_manager_->drag_controller()->dragging_panel()) {
nit: should there be a IsDragging()/DragInProgress() method on drag controller?

http://codereview.chromium.org/9403035/diff/13029/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

http://codereview.chromium.org/9403035/diff/13029/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:24:
dragging_panel_original_position_ = panel->GetBounds().origin();
nit: Should we save the position before calling StartDraggingPanel? In case the
strip changes the bounds in that method...

[commit-bot: I haz the power, 2012-02-21 21:05:13]

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

[commit-bot: I haz the power, 2012-02-21 22:12:11]

Try job failure for 9403035-21007 (retry) on linux_rel for step "ui_tests".
It's a second try, previously, step "ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-21 22:18:43]

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

[commit-bot: I haz the power, 2012-02-21 23:46:02]

Change committed as 122914