Support dragging panels to stack and snap.

Support dragging panels to stack and snap.

BUG=159637
TEST=new tests
TBR=thakis@chromium.org

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

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

[Dmitry Titov, 2013-01-09 02:29:50]

Big patch... Few first comments:

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:15: class NativePanelStack {
This needs a class comment. What this class is for, how used, ownership etc.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.cc (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.cc:784: // Then try the extension manifest name.
At last, app name is used.
This is unrelated change. Can it be moved to a separate patch?

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.cc:822: void Panel::MoveBy(const gfx::Vector2d&
delta_origin) {
Naming: MoveByInstantly would better reflect what this is doing.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:77: // Used to remember the
top panel, if different from |panel|, for use when
Could you improve this comment, it's not clear waht "for use when restore it"
means, w/o context.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:81: PanelPlacement() :
panel(NULL) { }
initialize top_panel(NULL) too

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_stack_view.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.h:16: class PanelStackView :
public NativePanelStack,
Class comment.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.h:41:
scoped_ptr<StackedPanelCollection> stack_;
naming: stack_collection_ is better, although a bit longer.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_view.cc:593:
UpdateWindowAttribute(GWL_EXSTYLE, 0, WS_EX_APPWINDOW, false);
Please add a comment here, to explain why this is done.

[jianli, 2013-01-09 21:00:59]

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:15: class NativePanelStack {
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> This needs a class comment. What this class is for, how used, ownership etc.

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.cc (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.cc:784: // Then try the extension manifest name.
At last, app name is used.
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> This is unrelated change. Can it be moved to a separate patch?

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.cc:822: void Panel::MoveBy(const gfx::Vector2d&
delta_origin) {
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> Naming: MoveByInstantly would better reflect what this is doing.

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:77: // Used to remember the
top panel, if different from |panel|, for use when
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> Could you improve this comment, it's not clear waht "for use when restore it"
> means, w/o context.

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:81: PanelPlacement() :
panel(NULL) { }
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> initialize top_panel(NULL) too

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_stack_view.h (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.h:16: class PanelStackView :
public NativePanelStack,
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> Class comment.

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.h:41:
scoped_ptr<StackedPanelCollection> stack_;
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> naming: stack_collection_ is better, although a bit longer.

Done.

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_view.cc (right):

https://codereview.chromium.org/11669018/diff/34001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_view.cc:593:
UpdateWindowAttribute(GWL_EXSTYLE, 0, WS_EX_APPWINDOW, false);
On 2013/01/09 02:29:50, Dmitry Titov wrote:
> Please add a comment here, to explain why this is done.

Done.

[jianli, 2013-01-09 23:56:11]

Daniel, could you please also take a look?

[Dmitry Titov, 2013-01-10 00:16:24]

More feedback (sorry it comes in pieces, but the patch is big :-)

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/detached_panel_collection.cc (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/detached_panel_collection.cc:207:
panel->UpdateMinimizeRestoreButtonVisibility();
This is where I would add a new method on Panel,
UpdateOnCollectionChange(new_collection_type) and put both calls to
UpdateMinimizeRestoreButtonVisibility() and UpdateStackingProperty() in it.
Otherwise we'll end up calling the same pack of Update...() methods from every
Collection... 
Also, I'd call it from the same place that calls
PanelColelction::UpdatePanelOnColelctionChange().

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel.h:82: virtual void
UpdatePanelStackingProperty() = 0;
This is needed to connect/disconnect the panel from the native stack
implementation. So it feels like a method on the NativePanelStack rather then on
the NativePanel.
Should rather be NativePanelStack::OnPanelAddedOrRemoved, if that method will
get a Panel* argument.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:29: virtual void Close() = 0;
Could you add a comment about who and when calls this Close(). This would be
useful to understanding the lifetime of the object.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:30: virtual void
OnPanelAddedOrRemoved() = 0;
This feels like it should get a Panel* argument, then we can remove
UpdatePanelStackingProperty from NativePanel

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:32: virtual void SetBounds(const
gfx::Rect& bounds) = 0;
SetBounds - also a comment would be useful about who calls that.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:363: std::vector<Panel*>
PanelManager::free_floating_panels() const {
Naming: since it's not a simple accessor, it should be named LikeThis, and if it
returns Detached and Stacked panels, lets call it GetDetachedAndStackedPanels
instead of introducing new notion of "floating panels".

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:62: gfx::NativeWindow
GetNativeWindow() const;
lets get native_stack() accessor and then call GetNativeWindow from that. It's
weird to ask for NativeWindow from platfrom-independent PanelCollection.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:89: NativePanelStack*
native_stack_;  // Weak, own us.
own -> owns

[jianli, 2013-01-10 02:05:37]

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/detached_panel_collection.cc (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/detached_panel_collection.cc:207:
panel->UpdateMinimizeRestoreButtonVisibility();
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> This is where I would add a new method on Panel,
> UpdateOnCollectionChange(new_collection_type) and put both calls to
> UpdateMinimizeRestoreButtonVisibility() and UpdateStackingProperty() in it.
> Otherwise we'll end up calling the same pack of Update...() methods from every
> Collection... 
> Also, I'd call it from the same place that calls
> PanelColelction::UpdatePanelOnColelctionChange().

Changed to adding new method OnCollectionChanged() as suggested.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel.h:82: virtual void
UpdatePanelStackingProperty() = 0;
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> This is needed to connect/disconnect the panel from the native stack
> implementation. So it feels like a method on the NativePanelStack rather then
on
> the NativePanel.
> Should rather be NativePanelStack::OnPanelAddedOrRemoved, if that method will
> get a Panel* argument.

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:29: virtual void Close() = 0;
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> Could you add a comment about who and when calls this Close(). This would be
> useful to understanding the lifetime of the object.

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:30: virtual void
OnPanelAddedOrRemoved() = 0;
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> This feels like it should get a Panel* argument, then we can remove
> UpdatePanelStackingProperty from NativePanel

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:32: virtual void SetBounds(const
gfx::Rect& bounds) = 0;
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> SetBounds - also a comment would be useful about who calls that.

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:363: std::vector<Panel*>
PanelManager::free_floating_panels() const {
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> Naming: since it's not a simple accessor, it should be named LikeThis, and if
it
> returns Detached and Stacked panels, lets call it GetDetachedAndStackedPanels
> instead of introducing new notion of "floating panels".

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:62: gfx::NativeWindow
GetNativeWindow() const;
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> lets get native_stack() accessor and then call GetNativeWindow from that. It's
> weird to ask for NativeWindow from platfrom-independent PanelCollection.

Done.

https://codereview.chromium.org/11669018/diff/44003/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:89: NativePanelStack*
native_stack_;  // Weak, own us.
On 2013/01/10 00:16:24, Dmitry Titov wrote:
> own -> owns

Done.

[dcheng, 2013-01-10 22:39:03]

Some concerns about object ownership and lifetime issues. Part of it is the fact
that I'm not as familiar with panels code anymore, but the code has also gotten
pretty complex: some of the views stuff self-deletes when the widget is closed
(I assume), but there are other objects that are not in the views hierarchy that
have unclear ownership (StackPanelCollection in particular; maybe it
self-deletes as well, but I can't figure that out from this patch).

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/base_panel_browser_test.cc:20: #include
"chrome/browser/ui/panels/stacked_panel_collection.h"
Nit: I don't think we actually try to deref a StackedPanelCollection pointer in
this module, so it's OK to leave this include out (we just take a pointer and
pass it to MovePanelToCollection, so we shouldn't need the definition)

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:19: friend class
StackedPanelCollection;
Isn't typical convention to put these in an explicit private: section?

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:24: static NativePanelStack*
Create(StackedPanelCollection* stack);
I know we don't do it elsewhere at the moment, but we may want to consider using
scoped_ptr where applicable here. In particular, we could probably change this
to be:
static scoped_ptr<NativePanelStack> Create(scoped_ptr<StackedPanelCollection>
stack); to indicate the ownership transfers, rather than depending on comments.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.h:182: StackedPanelCollection* stack() const;
Do we want to be returning a non-const pointer here? If so, maybe the method
shouldn't be marked const (although there's some inconsistencies here already,
such as on line 89 and 90, it seems better to not mark pointers to mutable class
state as being returned by const functions).

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_collection.h:32: // The panel is added and placed
at top position (currently oly used by
Nit: typo, s/oly/only

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:31: // The minimum overlap, in
pixeles, between two panels such that they can be
s/pixeles/pixels.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:31: // The minimum overlap, in
pixeles, between two panels such that they can be
It's not very clear to me what these two constants mean. Which one controls when
they snap together? Or do they both control it? How about when unstacking? It
seems like one constant is primarily used for vertical distance and another for
horizontal overlap, but that's not particularly clear from just looking at these
two constants.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:49: : (bounds2.right() -
bounds1.x());
Do you think we could reuse gfx::Rect::Intersect here?

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:512:
kGluePanelsOverlapThreshold) {
Similar checks to this seem to be repeated in several places (line 665, 682),
perhaps moving this to a helper function would be appropriate?

Ordering of parameters seems to differ sometimes, so I guess that could be a
problem, but in the current code, it's a little hard to understand how the order
of parameters affects things (e.g. how does GetHorizontalOverlap(x, y) differ
from GetHorizontalOverlap(y, x)? If they're overlapping, shouldn't it return the
same thing either way?).

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:720: // to reverse the order
when coping to the local list.
s/coping/copying

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.h:79: // colleciton.
Nit: typo, s/colleciton/collection

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:252: return stack;
Consider returning scoped_ptr<StackedPanelCollection> since PanelManager
(apparently?) doesn't own the created stack.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:258: stack->CloseAll();
How is |stack| freed? I see several references to RemoveStack in the dragging
controller, but I don't see any deletes.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.cc:18: native_stack_ =
NativePanelStack::Create(this);
If native_stack_ owns us, who deletes native_stack_? It appears it self-deletes
as a side effect of being a views widget, but that is really hard to follow.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:13: #include
"ui/gfx/native_widget_types.h"
Do we still need this #include?

[jianli, 2013-01-10 23:23:23]

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/base_panel_browser_test.cc:20: #include
"chrome/browser/ui/panels/stacked_panel_collection.h"
On 2013/01/10 22:39:03, dcheng wrote:
> Nit: I don't think we actually try to deref a StackedPanelCollection pointer
in
> this module, so it's OK to leave this include out (we just take a pointer and
> pass it to MovePanelToCollection, so we shouldn't need the definition)

It is needed by the compiler since BasePanelBrowserTest::CreateStackedPanel
takes StackedPanelCollection pointer and then converts to PanelCollection
pointer.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:19: friend class
StackedPanelCollection;
On 2013/01/10 22:39:03, dcheng wrote:
> Isn't typical convention to put these in an explicit private: section?

Done.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:24: static NativePanelStack*
Create(StackedPanelCollection* stack);
On 2013/01/10 22:39:03, dcheng wrote:
> I know we don't do it elsewhere at the moment, but we may want to consider
using
> scoped_ptr where applicable here. In particular, we could probably change this
> to be:
> static scoped_ptr<NativePanelStack> Create(scoped_ptr<StackedPanelCollection>
> stack); to indicate the ownership transfers, rather than depending on
comments.

No, we can't do that since the lifetime of NativePanelStack is managed by itself
and it will be destructed after Close() is called. I updated the comment to make
it more clear.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel.h:182: StackedPanelCollection* stack() const;
On 2013/01/10 22:39:03, dcheng wrote:
> Do we want to be returning a non-const pointer here? If so, maybe the method
> shouldn't be marked const (although there's some inconsistencies here already,
> such as on line 89 and 90, it seems better to not mark pointers to mutable
class
> state as being returned by const functions).

Yes we need non-const pointer since we need to call non-const method. I do not
want to remove const method modifier since this is simply a getter and it should
not mean to have something that can update any data in Panel.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_collection.h:32: // The panel is added and placed
at top position (currently oly used by
On 2013/01/10 22:39:03, dcheng wrote:
> Nit: typo, s/oly/only

Done.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:31: // The minimum overlap, in
pixeles, between two panels such that they can be
On 2013/01/10 22:39:03, dcheng wrote:
> s/pixeles/pixels.

Done.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:31: // The minimum overlap, in
pixeles, between two panels such that they can be
On 2013/01/10 22:39:03, dcheng wrote:
> It's not very clear to me what these two constants mean. Which one controls
when
> they snap together? Or do they both control it? How about when unstacking? It
> seems like one constant is primarily used for vertical distance and another
for
> horizontal overlap, but that's not particularly clear from just looking at
these
> two constants.

These 2 constants are needed by both vertical stacking and horizontal snapping.
I've updated comments here to make it more clear.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:49: : (bounds2.right() -
bounds1.x());
On 2013/01/10 22:39:03, dcheng wrote:
> Do you think we could reuse gfx::Rect::Intersect here?

Not really since we do not really mean 2 rectangles intersect.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:512:
kGluePanelsOverlapThreshold) {
On 2013/01/10 22:39:03, dcheng wrote:
> Similar checks to this seem to be repeated in several places (line 665, 682),
> perhaps moving this to a helper function would be appropriate?
> 
> Ordering of parameters seems to differ sometimes, so I guess that could be a
> problem, but in the current code, it's a little hard to understand how the
order
> of parameters affects things (e.g. how does GetHorizontalOverlap(x, y) differ
> from GetHorizontalOverlap(y, x)? If they're overlapping, shouldn't it return
the
> same thing either way?).

The ordering does matter to compute the horizontal or vertical distance.
However, the ordering does not matter for compute the overlap.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:720: // to reverse the order
when coping to the local list.
On 2013/01/10 22:39:03, dcheng wrote:
> s/coping/copying

Done.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.h:79: // colleciton.
On 2013/01/10 22:39:03, dcheng wrote:
> Nit: typo, s/colleciton/collection

Done.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_manager.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:252: return stack;
On 2013/01/10 22:39:03, dcheng wrote:
> Consider returning scoped_ptr<StackedPanelCollection> since PanelManager
> (apparently?) doesn't own the created stack.

See my comment below.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_manager.cc:258: stack->CloseAll();
On 2013/01/10 22:39:03, dcheng wrote:
> How is |stack| freed? I see several references to RemoveStack in the dragging
> controller, but I don't see any deletes.

When a stack is not longer needed, PanelManager::RemoveStack will be called. It
then calls StackedPanelCollection::CloseAll which will notify the native stack
to close. NativePanel::Close will do all the cleanup and then delete itself and
its owned StackedPanelCollection.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.cc (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.cc:18: native_stack_ =
NativePanelStack::Create(this);
On 2013/01/10 22:39:03, dcheng wrote:
> If native_stack_ owns us, who deletes native_stack_? It appears it
self-deletes
> as a side effect of being a views widget, but that is really hard to follow.

The native stack, that is PanelStackView, is deleted in
PanelStackView::DeleteDelegate, which is managed by views widget infrastructure.
It is similar to what we do with NativePanel and Panel.

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/stacked_panel_collection.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/stacked_panel_collection.h:13: #include
"ui/gfx/native_widget_types.h"
On 2013/01/10 22:39:03, dcheng wrote:
> Do we still need this #include?

Removed.

[dcheng, 2013-01-10 23:43:01]

One quick comment (I'll take a more thorough pass through the code later)

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/native_panel_stack.h (right):

https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
chrome/browser/ui/panels/native_panel_stack.h:24: static NativePanelStack*
Create(StackedPanelCollection* stack);
On 2013/01/10 23:23:23, jianli wrote:
> On 2013/01/10 22:39:03, dcheng wrote:
> > I know we don't do it elsewhere at the moment, but we may want to consider
> using
> > scoped_ptr where applicable here. In particular, we could probably change
this
> > to be:
> > static scoped_ptr<NativePanelStack>
Create(scoped_ptr<StackedPanelCollection>
> > stack); to indicate the ownership transfers, rather than depending on
> comments.
> 
> No, we can't do that since the lifetime of NativePanelStack is managed by
itself
> and it will be destructed after Close() is called. I updated the comment to
make
> it more clear.

At the very least, StackedPanelCollection should still be passed by scoped_ptr
since a NativePanelStack should and will take ownership of it.

[jianli, 2013-01-11 01:33:42]

On 2013/01/10 23:43:01, dcheng wrote:
> One quick comment (I'll take a more thorough pass through the code later)
> 
>
https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
> File chrome/browser/ui/panels/native_panel_stack.h (right):
> 
>
https://codereview.chromium.org/11669018/diff/40007/chrome/browser/ui/panels/...
> chrome/browser/ui/panels/native_panel_stack.h:24: static NativePanelStack*
> Create(StackedPanelCollection* stack);
> On 2013/01/10 23:23:23, jianli wrote:
> > On 2013/01/10 22:39:03, dcheng wrote:
> > > I know we don't do it elsewhere at the moment, but we may want to consider
> > using
> > > scoped_ptr where applicable here. In particular, we could probably change
> this
> > > to be:
> > > static scoped_ptr<NativePanelStack>
> Create(scoped_ptr<StackedPanelCollection>
> > > stack); to indicate the ownership transfers, rather than depending on
> > comments.
> > 
> > No, we can't do that since the lifetime of NativePanelStack is managed by
> itself
> > and it will be destructed after Close() is called. I updated the comment to
> make
> > it more clear.
> 
> At the very least, StackedPanelCollection should still be passed by scoped_ptr
> since a NativePanelStack should and will take ownership of it.

Done.

[Dmitry Titov, 2013-01-14 23:28:40]

Great! My few last comments:

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:28: // together. This is one
of two criteria needed for both stacking and snapping.
"This is one of two criteria needed for both stacking and snapping." is not
clear - why it is important to say that? Maybe remove this sentence?

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:70: int
GetVerticalDistance(const gfx::Rect& top_bounds,
these functions use top_bounds/bottom_bounds while the functions above use
bounds1/bounds2.
Can we use bounds1/2 here as well? Otherwise, if the functions do expect
right>=left, then it's unclear why you need abs()...

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:86: if (*iter == panel)
lets put the body of this for() into {}, it'll read better this way.
Alternatively, it's probably better to have a single loop and a bool,
"panel_and_below", initially false, then becoming true as iter crosses panel,
then having an if(panel_and_below) {...}

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:148: // has to been done first
and we should continue processing it for the case
"to been" -> "to be"

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:150: if
(!TryUnstackFromTop(target_position))
Why it is important to have this if() here instead of calling those
sequentially? If needed, comment would be good.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:166:
TrySnap(&target_position);
It is possible that TryDetach() will succeed and we immediately can snap.
However we return before tryign to snap...

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:346: gfx::Vector2d
delta_origin =
lets just call it 'delta'. I understand you meant 'the delta by which the origin
will move' but it also can be read like 'origin of delta' which is confusing...

Same for couple of places below which use delta_origin.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:378: bool
PanelDragController::TryUnstackFromTop(const gfx::Point& target_position) {
Lets add a comment for this function, what exactly is "UnstackingFromTop". Same
for "UnstackingFromBottom".

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:436: DetachedPanelCollection*
detached_collection =
Here there is an assumption that unstaking happens always into
DetachedPanelCollection. But it also can be moved right into Dock... I think
it's ok because you will check TryDock later anyways, but maybe a comment here
would help.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:743: // Note that if the stack
is the original collection, do not remove it now.
Why? could be useful to add a reason to keep it, for a better comment

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_stack_view.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.cc:65: #if defined(OS_WIN) &&
!defined(USE_AURA)
I think USE_AURA is only different that we should call GetAcceleratedWidget,
which returns AcceleratedWidget which is typdef HWND... We need to add AURA
support because it's becoming a new default on WIN soon. But this can be done
separately.

[dcheng, 2013-01-14 23:34:01]

LGTM with some caveats. This is a really large patch and I'm not really sure
about every bit of it still, so I'm not sure my lgtm means much =)

Also, I wonder if it'd be possible to split out the new tests in a future patch
so we have a series of tests for stack-specific dragging behavior. That test is
getting really large. Maybe it doesn't make sense to group the tests like that
though.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_browsertest.cc:1561: // Move the mouse a
little bit. Expect only P2 moves horizontally.
Nit: I think all these comments on the expected results can be helpful, since
these tests are quite long. However, it might be useful to document why P2
should only move horizontally (in this instance, I assume it's because its top
edge is glued to the bottom of P1).

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_browsertest.cc:2089:
ASSERT_TRUE(panel_manager->num_stacks() == 0 || stack->num_panels() == 0);
I feel like this test condition could be made more precise.

Why might it "still" exist? Is there a race here on when the empty stack is
cleaned up? If the stack isn't cleaned up until the drag ends, why isn't it safe
to always just check stack->num_panels() == 0 here? Or is this future proofing
to make sure that implementations on different OSes can use the same test (in
that case, I'd feel better to get rid of the || until we need it but that's just
a personal opinion)?

[jianli, 2013-01-15 01:43:43]

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_browsertest.cc:1561: // Move the mouse a
little bit. Expect only P2 moves horizontally.
On 2013/01/14 23:34:01, dcheng wrote:
> Nit: I think all these comments on the expected results can be helpful, since
> these tests are quite long. However, it might be useful to document why P2
> should only move horizontally (in this instance, I assume it's because its top
> edge is glued to the bottom of P1).

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_browsertest.cc:2089:
ASSERT_TRUE(panel_manager->num_stacks() == 0 || stack->num_panels() == 0);
On 2013/01/14 23:34:01, dcheng wrote:
> I feel like this test condition could be made more precise.
> 
> Why might it "still" exist? Is there a race here on when the empty stack is
> cleaned up? If the stack isn't cleaned up until the drag ends, why isn't it
safe
> to always just check stack->num_panels() == 0 here? Or is this future proofing
> to make sure that implementations on different OSes can use the same test (in
> that case, I'd feel better to get rid of the || until we need it but that's
just
> a personal opinion)?

Changed to check only for stack->num_panels() == 0.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:28: // together. This is one
of two criteria needed for both stacking and snapping.
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> "This is one of two criteria needed for both stacking and snapping." is not
> clear - why it is important to say that? Maybe remove this sentence?

This is added per dcheng's comment. To make it more clear, I removed this extra
comment and shared one comment for both constants.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:70: int
GetVerticalDistance(const gfx::Rect& top_bounds,
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> these functions use top_bounds/bottom_bounds while the functions above use
> bounds1/bounds2.
> Can we use bounds1/2 here as well? Otherwise, if the functions do expect
> right>=left, then it's unclear why you need abs()...

We do need abs because bottom_bounds.y() might be above or below
top_bounds.bottom(). We can't use bounds1 and bounds2 since this function is to
compute the distance between the bottom edge of |top_bounds| and the top edge of
|bottom_bounds|. Added comment.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:86: if (*iter == panel)
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> lets put the body of this for() into {}, it'll read better this way.
> Alternatively, it's probably better to have a single loop and a bool,
> "panel_and_below", initially false, then becoming true as iter crosses panel,
> then having an if(panel_and_below) {...}

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:148: // has to been done first
and we should continue processing it for the case
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> "to been" -> "to be"

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:150: if
(!TryUnstackFromTop(target_position))
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> Why it is important to have this if() here instead of calling those
> sequentially? If needed, comment would be good.

Certainly we call these two TryUnstack*** sequentially. But since only one of
them might be applicable, I thought using if was a bit small optimization. Added
comment.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:166:
TrySnap(&target_position);
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> It is possible that TryDetach() will succeed and we immediately can snap.
> However we return before tryign to snap...

Good thought. Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:346: gfx::Vector2d
delta_origin =
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> lets just call it 'delta'. I understand you meant 'the delta by which the
origin
> will move' but it also can be read like 'origin of delta' which is
confusing...
> 
> Same for couple of places below which use delta_origin.

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:378: bool
PanelDragController::TryUnstackFromTop(const gfx::Point& target_position) {
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> Lets add a comment for this function, what exactly is "UnstackingFromTop".
Same
> for "UnstackingFromBottom".

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:436: DetachedPanelCollection*
detached_collection =
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> Here there is an assumption that unstaking happens always into
> DetachedPanelCollection. But it also can be moved right into Dock... I think
> it's ok because you will check TryDock later anyways, but maybe a comment here
> would help.

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/panels/...
chrome/browser/ui/panels/panel_drag_controller.cc:743: // Note that if the stack
is the original collection, do not remove it now.
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> Why? could be useful to add a reason to keep it, for a better comment

Done.

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/views/p...
File chrome/browser/ui/views/panels/panel_stack_view.cc (right):

https://codereview.chromium.org/11669018/diff/52001/chrome/browser/ui/views/p...
chrome/browser/ui/views/panels/panel_stack_view.cc:65: #if defined(OS_WIN) &&
!defined(USE_AURA)
On 2013/01/14 23:28:40, Dmitry Titov wrote:
> I think USE_AURA is only different that we should call GetAcceleratedWidget,
> which returns AcceleratedWidget which is typdef HWND... We need to add AURA
> support because it's becoming a new default on WIN soon. But this can be done
> separately.

I will do it in a separate patch since there might be some other issues I need
to fix in order to support win_aura.

[Dmitry Titov, 2013-01-15 19:21:35]

LGTM

[commit-bot: I haz the power, 2013-01-15 19:58:42]

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

[commit-bot: I haz the power, 2013-01-15 19:58:51]

Presubmit check for 11669018-69001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.2s to calculate.

[jianli, 2013-01-15 20:01:12]

Adding Nico as TBR for gypi change.

[commit-bot: I haz the power, 2013-01-15 20:02:48]

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

[Nico, 2013-01-15 20:04:15]

lgtm, but please have tracking bugs for feature stuff like this and a BUG= line
in the future (see cpu's recent email to chromium-dev)

[jianli, 2013-01-15 20:46:34]

On 2013/01/15 20:04:15, Nico wrote:
> lgtm, but please have tracking bugs for feature stuff like this and a BUG=
line
> in the future (see cpu's recent email to chromium-dev)

We do have a tracking bug for this and I forgot to include it. Updated the
description to add it. Thanks.

[commit-bot: I haz the power, 2013-01-15 23:51:16]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-01-15 23:53:58]

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

[commit-bot: I haz the power, 2013-01-15 23:58:43]

Change committed as 177017

[commit-bot: I haz the power, 2013-01-16 00:35:33]

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

[commit-bot: I haz the power, 2013-01-16 03:22:33]

Change committed as 177069