Support detaching/attaching panels via inter-strip drags.

Support detaching/attaching panels via inter-strip drags.

BUG=none
TEST=new tests


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

[jennb, 2012-03-01 00:33:38]

I'll send this off now even though I haven't gotten through the entire patch. I
expect it will change significantly from these comments (and from fixing test
failures) so will look more thoroughly in the next revision.

Overall comment is to pay more attention to where logic belongs - drag
controller vs panel manager vs a particular panel strip.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/docked_panel_strip.cc:190: // Set panel properties for
this strip.
Wish this method and AddPanels() could be consolidated more. It's easy to mess
up panel properties with it being set in both methods.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/docked_panel_strip.cc:204: gfx::Rect
new_bounds(panel->GetBounds());
Won't refresh layout update the panel bounds correctly? No need for these 3
lines.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/docked_panel_strip.h:142: Panel* GetNextPanel(const
Panels::iterator& iter) const;
Is this just a wrapper for iter++?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/docked_panel_strip.h:171: Panel*
panel_to_restore_after_on_drag_cancelled_;
I don't understand the comment for this var.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:14: class
PanelDragBrowserTest : public BasePanelBrowserTest {
Is this file supposed to contain all panel dragging tests? We currently have
dragging tests in 4 different files, plus this one makes 5. Please consolidate.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:48:
EXPECT_EQ(PanelStrip::IN_OVERFLOW, panel->panel_strip()->type());
Redundant as WaitForLayoutModeChanged already checks this.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:76: gfx::Point
small_drag_delta(-5, -10);
Could you write these tests in such a way that they will not break if the offset
amount to trigger a detach is changed?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:357:
IN_PROC_BROWSER_TEST_F(PanelDragBrowserTest, DetachInOverflow) {
s/InOverflow/WithOverflow ?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:17: const int
kDetachDockedPanelThreshold = -80;
Thresholds are usually positive amounts.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:43: total_ommitted_delta_y_ =
0;
Seems cleaner to store original mouse location at start of drag, then compute
the difference between current and starting mouse locations when needed, rather
than accumulate delta y's during the drag process.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:48:
panel->SetAlwaysOnTop(true);
Always-on-top should only be set if panel is in the Dock. Don't want the
dragging panel to seem fully visible, only to become partially hidden when the
drag is finished (scenario when the panel overlaps the dock, but does not end up
in the dock).

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:61: PanelStrip*
strip_to_drag_to = CanDragToDockedStrip(mouse_location);
PanelStrip* target_strip = ComputeDragTarget(mouse_location);
if (target_strip != current strip)
  do stuff
else
  do other stuff

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:101: if (cancelled &&
panel->panel_strip() != dragging_panel_original_strip_) {
I'm having a hard time picturing what happens here if the original strip is the
docked strip. Panel is moved back to the dock at a position that isn't within
the dock area, then it slides horizontally back to its original position? 

Should it be using original panel position here?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:112:
panel->SetAlwaysOnTop(false);
Again, it seems like the responsibility of the strip to decide about
always-on-top status.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:115: PanelStrip*
PanelDragController::CanDragToDockedStrip(
Seems like this function and CanDragToDetachedStrip could be combined into

if (current strip is detached)
  return dock strip if dock strip says panel can be attached
else if docked
  return detached strip if dock strip says panel can be detached

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:123: int new_panel_bottom =
dragging_panel_->GetBounds().bottom() +
Would it be cleaner to do computations using starting and current mouse
positions?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:126:
panel_manager_->docked_strip()->display_area().bottom();
Should dock strip be the one to decide if a panel can be detached/attached
rather than drag controller digging into display area of the dock?

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:157:
old_strip->EndDraggingPanel(panel, false);
Confusing. Dragging isn't really ended. For instance, the dock still needs to
remember panel's original index in the strip until dragging is truly completed
in case the drag is cancelled.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:159: // Move the panel to new
strip.
I'd prefer if moving a panel between strips was always brokered by PanelManager.
See https://chromiumcodereview.appspot.com/9560002/

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:165:
new_strip->StartDraggingPanel(panel);
Very weird logic flow:

EndDragging()
MoveToStrip()
strip->StartDragging()

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_strip.h (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_strip.h:41: virtual void
AddPanelAtPosition(Panel* panel, const gfx::Point& position) = 0;
Is the origin of the panel placed close to |position|? Could you explain more?

[jianli, 2012-03-02 22:42:43]

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
File chrome/browser/ui/panels/docked_panel_strip.cc (right):

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.cc:190: // Set panel properties for
this strip.
On 2012/03/01 00:33:38, jennb wrote:
> Wish this method and AddPanels() could be consolidated more. It's easy to mess
> up panel properties with it being set in both methods.

I agree but it will involve quite a bit of refactoring and add up the complexity
to this patch. I would do it in another patch.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.cc:204: gfx::Rect
new_bounds(panel->GetBounds());
On 2012/03/01 00:33:38, jennb wrote:
> Won't refresh layout update the panel bounds correctly? No need for these 3
> lines.

RefreshLayout will not update the dragging panel. See if check in that function.
When we drag a panel around, it be moved anywhere and we do not want
RefreshLayout to update it with its "finalized" bounds.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.h:142: Panel* GetNextPanel(const
Panels::iterator& iter) const;
On 2012/03/01 00:33:38, jennb wrote:
> Is this just a wrapper for iter++?

Yes. I removed it since it is not needed.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/docked...
chrome/browser/ui/panels/docked_panel_strip.h:171: Panel*
panel_to_restore_after_on_drag_cancelled_;
On 2012/03/01 00:33:38, jennb wrote:
> I don't understand the comment for this var. 

Per discussion, removed.

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

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_browsertest.cc:14: class
PanelDragBrowserTest : public BasePanelBrowserTest {
On 2012/03/01 00:33:38, jennb wrote:
> Is this file supposed to contain all panel dragging tests? We currently have
> dragging tests in 4 different files, plus this one makes 5. Please
consolidate.

Yes. I will move other dragging related tests to this file after this patch is
landed.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_browsertest.cc:48:
EXPECT_EQ(PanelStrip::IN_OVERFLOW, panel->panel_strip()->type());
On 2012/03/01 00:33:38, jennb wrote:
> Redundant as WaitForLayoutModeChanged already checks this.

Just want to be safe because only after you read WaitForLayoutModeChanged code
you will know this check is redundant.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_browsertest.cc:76: gfx::Point
small_drag_delta(-5, -10);
On 2012/03/01 00:33:38, jennb wrote:
> Could you write these tests in such a way that they will not break if the
offset
> amount to trigger a detach is changed?

Done.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_browsertest.cc:357:
IN_PROC_BROWSER_TEST_F(PanelDragBrowserTest, DetachInOverflow) {
On 2012/03/01 00:33:38, jennb wrote:
> s/InOverflow/WithOverflow ?

Done.

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

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:17: const int
kDetachDockedPanelThreshold = -80;
On 2012/03/01 00:33:38, jennb wrote:
> Thresholds are usually positive amounts.

Done.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:43: total_ommitted_delta_y_ =
0;
On 2012/03/01 00:33:38, jennb wrote:
> Seems cleaner to store original mouse location at start of drag, then compute
> the difference between current and starting mouse locations when needed,
rather
> than accumulate delta y's during the drag process.

Removed. We now use original mouse location at start of dragging at docked
strip.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:48:
panel->SetAlwaysOnTop(true);
On 2012/03/01 00:33:38, jennb wrote:
> Always-on-top should only be set if panel is in the Dock. Don't want the
> dragging panel to seem fully visible, only to become partially hidden when the
> drag is finished (scenario when the panel overlaps the dock, but does not end
up
> in the dock).

Done.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:61: PanelStrip*
strip_to_drag_to = CanDragToDockedStrip(mouse_location);
On 2012/03/01 00:33:38, jennb wrote:
> PanelStrip* target_strip = ComputeDragTarget(mouse_location);
> if (target_strip != current strip)
>   do stuff
> else
>   do other stuff

Done.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:101: if (cancelled &&
panel->panel_strip() != dragging_panel_original_strip_) {
On 2012/03/01 00:33:38, jennb wrote:
> I'm having a hard time picturing what happens here if the original strip is
the
> docked strip. Panel is moved back to the dock at a position that isn't within
> the dock area, then it slides horizontally back to its original position? 
> 
> Should it be using original panel position here?

Changed per discussed.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:112:
panel->SetAlwaysOnTop(false);
On 2012/03/01 00:33:38, jennb wrote:
> Again, it seems like the responsibility of the strip to decide about
> always-on-top status.

Done.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:115: PanelStrip*
PanelDragController::CanDragToDockedStrip(
On 2012/03/01 00:33:38, jennb wrote:
> Seems like this function and CanDragToDetachedStrip could be combined into
> 
> if (current strip is detached)
>   return dock strip if dock strip says panel can be attached
> else if docked
>   return detached strip if dock strip says panel can be detached

I think it is cleaner to split the different checking logic and put them in 2
functions.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:123: int new_panel_bottom =
dragging_panel_->GetBounds().bottom() +
On 2012/03/01 00:33:38, jennb wrote:
> Would it be cleaner to do computations using starting and current mouse
> positions?

The triggering condition for attach is to check the bottom of the panel. This
cannot be done purely based on mouse locations.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:126:
panel_manager_->docked_strip()->display_area().bottom();
On 2012/03/01 00:33:38, jennb wrote:
> Should dock strip be the one to decide if a panel can be detached/attached
> rather than drag controller digging into display area of the dock?

Docked strip can only decide if a docked panel can be detached. It cannot decide
if a detached panel can be docked because this decision is made based on both
knowledge of detached strip (information about last mouse location) and docked
strip (information about docked area). So I rather keep both in drag controller.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:157:
old_strip->EndDraggingPanel(panel, false);
On 2012/03/01 00:33:38, jennb wrote:
> Confusing. Dragging isn't really ended. For instance, the dock still needs to
> remember panel's original index in the strip until dragging is truly completed
> in case the drag is cancelled.

Per discussion, this is what we want.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:159: // Move the panel to new
strip.
On 2012/03/01 00:33:38, jennb wrote:
> I'd prefer if moving a panel between strips was always brokered by
PanelManager.
> See https://chromiumcodereview.appspot.com/9560002/
> 

I agree. I would merge with your patch later.

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_drag_controller.cc:165:
new_strip->StartDraggingPanel(panel);
On 2012/03/01 00:33:38, jennb wrote:
> Very weird logic flow:
> 
> EndDragging()
> MoveToStrip()
> strip->StartDragging()

Per discussion, this is what we want. Renamed dragging methods to make the whole
logic clearer.

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

http://codereview.chromium.org/9546001/diff/1/chrome/browser/ui/panels/panel_...
chrome/browser/ui/panels/panel_strip.h:41: virtual void
AddPanelAtPosition(Panel* panel, const gfx::Point& position) = 0;
On 2012/03/01 00:33:38, jennb wrote:
> Is the origin of the panel placed close to |position|? Could you explain more?

Renamed this method to AddDraggingPanel to mean it is only intended to be user
for adding dragging panel from another strip. Also updated the comment to remove
"close to position" since indeed we now place the panel at position.

[jennb, 2012-03-03 02:19:33]

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_browsertest.cc:48:
EXPECT_EQ(PanelStrip::IN_OVERFLOW, panel->panel_strip()->type());
On 2012/03/02 22:42:43, jianli wrote:
> On 2012/03/01 00:33:38, jennb wrote:
> > Redundant as WaitForLayoutModeChanged already checks this.
> 
> Just want to be safe because only after you read WaitForLayoutModeChanged code
> you will know this check is redundant.

Should be removed as it deviates from the pattern in other tests where we do the
WaitForX without a check X after line.

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
File chrome/browser/ui/panels/panel_drag_controller.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/1/chrome/browser/ui/panel...
chrome/browser/ui/panels/panel_drag_controller.cc:127: if (new_panel_bottom -
docked_area_bottom < kDockDetachedPanelThreshold)
if new_panel_bottom is above the bottom of the dock, this will be negative and
therefore, always less than threshold, even if new_panel_bottom is close to the
bottom of the dock.

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_browsertest.cc:54: // Note that we
should not drag the panel close to the bottom area to trigger
This comment contradict the 'anywhere' in the previous line.

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.cc:99:
saved_panel_placement_.panel = NULL;
Should this call DiscardSavedPanelPlacement() instead? Ensures the same cleanup
logic applies in both places. Ditto for docked strip.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.cc:114: if (!HasPanel(panel)) {
Reminder that you meant to revert this change.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/detached_panel_strip.cc:132: void
DetachedPanelStrip::AddDraggingPanel(Panel* panel,
As discussed, separate out moving panels between strips from starting drag.

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:174:
InsertPanelToCollection(panel, panels_.end());
Don't refactor out the insertion logic as no other code path would be inserting
a temporary panel. I think what you really want to refactor out is the logic to
set panel properties.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:275: saved_panel_placement_.panel
= NULL;
leave it and call Discard at end of this method.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:407: // If the drag is cancelled,
we don't want to move the dragging panel to the
Maybe explain a bit about what is expected to happen on cancelled here? The
reader has no clue looking at this code...

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.cc:427: Panel* current_panel =
*iter;
I was confusing current_panel with the panel you're currently trying to insert.
Find a better name or drop the var and do:
x > (*iter)->GetBounds().x()

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.h:116: // When |panel| is restored,
it will be bring back before
The wording here is confusing to me. How about:

Remember the panel to the left, if any, for use when restoring the position of
the panel being dragged. Will be updated if the panel to the left is closed.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/docked_panel_strip.h:128: void
InsertPanelToCollection(Panel* panel, Panels::iterator position);
Doesn't return anything. This sounds like a wrapper for list::insert?

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
File chrome/browser/ui/panels/overflow_panel_strip.cc (right):

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/overflow_panel_strip.cc:188: void
OverflowPanelStrip::SavePanelPlacement(Panel* panel) {
nit: (and probably overkill) but feels like this interface belongs in
DraggableStrip, which DockedStrip and DetachedStrip implement, but OverflowStrip
does not implement.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:45: NSPoint
ConvertPointToCocoaCoordinates(const gfx::Point& point) {
Instead of defining this twice, please put somewhere and share it between these
2 files at least.

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:50: gfx::Point
target_panel_position;
Add comment  explaining why target panel position cannot be set here? If I
asked, someone else will too.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:78:
current_strip->RemovePanel(dragging_panel_);
Try to go through PanelManager to move panel between strips. Easier to keep
track of logic if it's all in one class.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:80:
dragging_panel_original_strip_->AddPanel(dragging_panel_);
Add followed by Load - will this appear to the user as a panel being added to
the end of the docked strip, then it zooms over back to it's saved placement?

What if AddPanel ends up with the panel in temporary layout? Seems like bad
things would happen.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:138: // The panel should be
dragged up higher enough to pass certain threshold.
high enough

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:139: int delta =
mouse_location.y() - mouse_location_on_docked_.y();
if (mouse_location.y() >= mouse_location_on_docked_.y() || 
  mouse_location_on_docked_.y() - mouse_location.y() < threshold)  
return false;

That seems easier to read than negating the delta.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.cc:144:
new_panel_position->Offset(
I don't understand the commenting in these lines.

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.h:61: // The original panel strip
that the drag is initiated.
Fix grammar.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.h:65: // received last time.
'last time' ?

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_drag_controller.h:72: // The threshold to detach
a docked panel to make it free-floating.
These comments simply repeat the var name. Explain what is being measured, i.e.
change in y direction from original drag position or distance of bottom edge of
panel from bottom of docked strip.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
File chrome/browser/ui/panels/panel_strip.h (right):

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_strip.h:70: // int bringing back the dragging
panel to its original positioning when the
typo: int

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_strip.h:73: virtual void
LoadSavedPanelPlacement() = 0;
RestorePanelToSavedPlacement()?

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_strip.h:74: virtual void
DiscardSavedPanelPlacement() = 0;
Please comment that drag controller will only call one of Load or Discard and
when.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_strip.h:81: virtual void
StartDraggingPanelLocally(Panel* panel) = 0;
s/Locally/WithinStrip or InStrip

Locally might be too vague.

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_strip.h:90: // position. Otherwise, |panel| will
stay as it is.
Add explanation for how the drag is truly cancelled. E.g. Drag controller will
do it...

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

https://chromiumcodereview.appspot.com/9546001/diff/10001/chrome/browser/ui/p...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:66: gfx::Point
ConvertPointFromCocoaCoordinates(NSPoint point) {
ConvertCoordinatesFromCocoa ? matches better with what we already have in
panel_browser_window_cocoa.mm

Surprised there isn't already a utility somewhere for this in the code base.

[jianli, 2012-03-07 19:14:31]

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_browsertest.cc:54: // Note that we
should not drag the panel close to the bottom area to trigger
On 2012/03/03 02:19:33, jennb wrote:
> This comment contradict the 'anywhere' in the previous line.

Done.

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:99:
saved_panel_placement_.panel = NULL;
On 2012/03/03 02:19:33, jennb wrote:
> Should this call DiscardSavedPanelPlacement() instead? Ensures the same
cleanup
> logic applies in both places. Ditto for docked strip.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:114: if (!HasPanel(panel)) {
On 2012/03/03 02:19:33, jennb wrote:
> Reminder that you meant to revert this change.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:132: void
DetachedPanelStrip::AddDraggingPanel(Panel* panel,
On 2012/03/03 02:19:33, jennb wrote:
> As discussed, separate out moving panels between strips from starting drag.

Done.

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:174:
InsertPanelToCollection(panel, panels_.end());
On 2012/03/03 02:19:33, jennb wrote:
> Don't refactor out the insertion logic as no other code path would be
inserting
> a temporary panel. I think what you really want to refactor out is the logic
to
> set panel properties.

Done. Added OnPanelAdded.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:275: saved_panel_placement_.panel
= NULL;
On 2012/03/03 02:19:33, jennb wrote:
> leave it and call Discard at end of this method.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:407: // If the drag is cancelled,
we don't want to move the dragging panel to the
On 2012/03/03 02:19:33, jennb wrote:
> Maybe explain a bit about what is expected to happen on cancelled here? The
> reader has no clue looking at this code...

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:427: Panel* current_panel =
*iter;
On 2012/03/03 02:19:33, jennb wrote:
> I was confusing current_panel with the panel you're currently trying to
insert.
> Find a better name or drop the var and do:
> x > (*iter)->GetBounds().x()

Dropped the temporary vars.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:116: // When |panel| is restored,
it will be bring back before
On 2012/03/03 02:19:33, jennb wrote:
> The wording here is confusing to me. How about:
> 
> Remember the panel to the left, if any, for use when restoring the position of
> the panel being dragged. Will be updated if the panel to the left is closed.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:128: void
InsertPanelToCollection(Panel* panel, Panels::iterator position);
On 2012/03/03 02:19:33, jennb wrote:
> Doesn't return anything. This sounds like a wrapper for list::insert?

Removed. We now don't need a wrapper. Instead, I introduced OnPanelAdded in
order to set common panel properties.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/ov...
File chrome/browser/ui/panels/overflow_panel_strip.cc (right):

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/ov...
chrome/browser/ui/panels/overflow_panel_strip.cc:188: void
OverflowPanelStrip::SavePanelPlacement(Panel* panel) {
On 2012/03/03 02:19:33, jennb wrote:
> nit: (and probably overkill) but feels like this interface belongs in
> DraggableStrip, which DockedStrip and DetachedStrip implement, but
OverflowStrip
> does not implement.

Will add DraggableStrip if we have more strips later.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_window_cocoa.mm (right):

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_window_cocoa.mm:45: NSPoint
ConvertPointToCocoaCoordinates(const gfx::Point& point) {
On 2012/03/03 02:19:33, jennb wrote:
> Instead of defining this twice, please put somewhere and share it between
these
> 2 files at least.

Done.

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:50: gfx::Point
target_panel_position;
On 2012/03/03 02:19:33, jennb wrote:
> Add comment  explaining why target panel position cannot be set here? If I
> asked, someone else will too.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:78:
current_strip->RemovePanel(dragging_panel_);
On 2012/03/03 02:19:33, jennb wrote:
> Try to go through PanelManager to move panel between strips. Easier to keep
> track of logic if it's all in one class.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:80:
dragging_panel_original_strip_->AddPanel(dragging_panel_);
On 2012/03/03 02:19:33, jennb wrote:
> Add followed by Load - will this appear to the user as a panel being added to
> the end of the docked strip, then it zooms over back to it's saved placement?
> 
> What if AddPanel ends up with the panel in temporary layout? Seems like bad
> things would happen.

Fixed per discussion.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:138: // The panel should be
dragged up higher enough to pass certain threshold.
On 2012/03/03 02:19:33, jennb wrote:
> high enough

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:139: int delta =
mouse_location.y() - mouse_location_on_docked_.y();
On 2012/03/03 02:19:33, jennb wrote:
> if (mouse_location.y() >= mouse_location_on_docked_.y() || 
>   mouse_location_on_docked_.y() - mouse_location.y() < threshold)  
> return false;
> 
> That seems easier to read than negating the delta.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:144:
new_panel_position->Offset(
On 2012/03/03 02:19:33, jennb wrote:
> I don't understand the commenting in these lines.

Comment updated.

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:61: // The original panel strip
that the drag is initiated.
On 2012/03/03 02:19:33, jennb wrote:
> Fix grammar. 

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:65: // received last time.
On 2012/03/03 02:19:33, jennb wrote:
> 'last time' ? 

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:72: // The threshold to detach
a docked panel to make it free-floating.
On 2012/03/03 02:19:33, jennb wrote:
> These comments simply repeat the var name. Explain what is being measured,
i.e.
> change in y direction from original drag position or distance of bottom edge
of
> panel from bottom of docked strip.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:70: // int bringing back the dragging
panel to its original positioning when the
On 2012/03/03 02:19:33, jennb wrote:
> typo: int

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:73: virtual void
LoadSavedPanelPlacement() = 0;
On 2012/03/03 02:19:33, jennb wrote:
> RestorePanelToSavedPlacement()?

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:74: virtual void
DiscardSavedPanelPlacement() = 0;
On 2012/03/03 02:19:33, jennb wrote:
> Please comment that drag controller will only call one of Load or Discard and
> when.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:81: virtual void
StartDraggingPanelLocally(Panel* panel) = 0;
On 2012/03/03 02:19:33, jennb wrote:
> s/Locally/WithinStrip or InStrip
> 
> Locally might be too vague.

Done.

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:90: // position. Otherwise, |panel| will
stay as it is.
On 2012/03/03 02:19:33, jennb wrote:
> Add explanation for how the drag is truly cancelled. E.g. Drag controller will
> do it...

Done.

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

http://codereview.chromium.org/9546001/diff/10001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:66: gfx::Point
ConvertPointFromCocoaCoordinates(NSPoint point) {
On 2012/03/03 02:19:33, jennb wrote:
> ConvertCoordinatesFromCocoa ? matches better with what we already have in
> panel_browser_window_cocoa.mm

Renamed the other one to ConvertRectToCocoaCoordinates since we now need to
convert either rect or point.
> 
> Surprised there isn't already a utility somewhere for this in the code base.

Moved to a utility file.

[Andrei, 2012-03-07 22:55:12]

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:112: void
DockedPanelStrip::InsertPreviewedPanel(Panel* panel) {
DCHECK panel is in preview mode

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:160: int max_panel_width =
GetMaxPanelWidth();
DCHECK panel is not initialized

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:256:
saved_panel_placement_.left_panel_ = NULL;
saved_panel_placement_.left_panel_ may be simply going to overflow, and will
come back. Do we want to handle that scenario?

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:756:
panel_manager_->MovePanelsToOverflow(last_panel_to_send_to_overflow);
This may move the panel in preview mode to overflow!

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:791:
panel_manager_->MovePanelsToOverflow(*panel_iter);
This may move the panel in preview mode to overflow. :(

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:162:
native_panel_->SetPanelAlwaysOnTop(on_top);
Actually the native panel needs to be on top iff always_on_top_ ||
in_preview_mode_

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:73: // make it free-floating.
Could we formulate the condition for detaching in terms of how  high the bottom
of the panel is as well? 

We could keep track of the panel's "imaginary" position (where it would be if it
followed the mouse exactly). To do that you only need to remember the offset
from the mouse position to the panel's origin when you start dragging.

The panel's "imaginary" bottom is current mouse position + offset from mouse to
origin at the start of the drag in the docked strip + panel height. 

Now detach == panel's "imaginary" bottom is  50 pixels from the bottom of the
docked area, and attach == it's less than 20 pixels away.

The advantage is that we don't need to keep track of "mouse last position when
docked", which for me was hard to follow.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:78: static const int
kDockDetachedPanelThreshold;
Why do they start with a "k"? I thought "k" was reserved for enum constants.

[jianli, 2012-03-08 01:18:26]

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:112: void
DockedPanelStrip::InsertPreviewedPanel(Panel* panel) {
On 2012/03/07 22:55:13, Andrei wrote:
> DCHECK panel is in preview mode

Done.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:160: int max_panel_width =
GetMaxPanelWidth();
On 2012/03/07 22:55:13, Andrei wrote:
> DCHECK panel is not initialized

Done.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:256:
saved_panel_placement_.left_panel_ = NULL;
On 2012/03/07 22:55:13, Andrei wrote:
> saved_panel_placement_.left_panel_ may be simply going to overflow, and will
> come back. Do we want to handle that scenario?

Good thought. I think we don't want to deal with it specially since if left
panel moves to overflow regardless if it comes back or not, we simply restore
the panel to most-left of docked panels. Added a note here.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:756:
panel_manager_->MovePanelsToOverflow(last_panel_to_send_to_overflow);
On 2012/03/07 22:55:13, Andrei wrote:
> This may move the panel in preview mode to overflow!

Yes, we have potential problem here and it has already been existing. I would
rather fix it in a separate patch. Added TODO for it.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:791:
panel_manager_->MovePanelsToOverflow(*panel_iter);
On 2012/03/07 22:55:13, Andrei wrote:
> This may move the panel in preview mode to overflow. :(

ditto.

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:162:
native_panel_->SetPanelAlwaysOnTop(on_top);
On 2012/03/07 22:55:13, Andrei wrote:
> Actually the native panel needs to be on top iff always_on_top_ ||
> in_preview_mode_

Per suggestion from Jenn, we're not going to make dragging panel always visible
for now.

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

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:73: // make it free-floating.
On 2012/03/07 22:55:13, Andrei wrote:
> Could we formulate the condition for detaching in terms of how  high the
bottom
> of the panel is as well? 
> 
> We could keep track of the panel's "imaginary" position (where it would be if
it
> followed the mouse exactly). To do that you only need to remember the offset
> from the mouse position to the panel's origin when you start dragging.
> 
> The panel's "imaginary" bottom is current mouse position + offset from mouse
to
> origin at the start of the drag in the docked strip + panel height. 
> 
> Now detach == panel's "imaginary" bottom is  50 pixels from the bottom of the
> docked area, and attach == it's less than 20 pixels away.
> 
> The advantage is that we don't need to keep track of "mouse last position when
> docked", which for me was hard to follow.  

Done.

http://codereview.chromium.org/9546001/diff/24001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:78: static const int
kDockDetachedPanelThreshold;
On 2012/03/07 22:55:13, Andrei wrote:
> Why do they start with a "k"? I thought "k" was reserved for enum constants.

Per coding style guide, we tend to prefix any constants with k.

[jennb, 2012-03-08 23:41:09]

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:122: void
DetachedPanelStrip::StartDraggingPanelWithinStrip(Panel* panel) {
dcheck panel is in strip?

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:91: if (panel->initialized()) {
if (panel !initialized)
  insert newly created
else if (preview mode)
  insert preview
else
  insert existing

Easier to read than nested ifs.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:111: 
Extra blank line.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:316: // If this panel is already
in the right position, nothing to do.
nit: rephrase the if-comment to match the if-condition

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:339: saved_panel_placement_.panel
= NULL;
Should clear out saved pointer to left_panel too.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:429:
dragging_panel_current_iterator_ = panels_.end();
Shame that you lose this iterator right before you might need it again in
RestorePanelToSavedPlacement. Maybe you can copy it to saved panel  placement
for use by the restored code upon cancel?

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:603: if (state ==
Panel::TITLE_ONLY && panel->in_preview_mode())
Keep old code here? It's obvious you want to leave title-bar up when dragging a
panel, but not so obvious why preview-mode needs to show the title...

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:810: // TODO(jianli): Need to
handle the case that panel in preview mode could be
crbug this and reference bug # here pls.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:122: // closed.
or moved out of the dock (e.g. to overflow).

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:139: void EnsureAvailableSpace(int
width);
Should not need this now that you've synced to latest code.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:324: // True if this panel should stay always
on top of other windows.
s/stay always/always stay

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:327: // True if this panel is in preview mode.
That is, panel bounds should not be
s/That is,/When in preview mode

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:329: // add a panel to the strip without
causing its bounds changed.
s/changed/to change

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:525: // The panel should not show
app icon in the desktop bar if it is in overflow.
Delete. If you need such a comment, it belongs in the place that calls this
method, presumably, the overflow strip.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:14: class
PanelDragBrowserTest : public BasePanelBrowserTest {
Is the intent to only have inter-strip drag tests in this file? If so, please
comment/rename as such. PanelInterStripDragTest? Can drop Browser from test name
to keep length manageable.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:39:
panel->SetPanelBounds(bounds);
Move this after the EXPECT_EQ as that expect is checking the MovePanelToStrip
call. Perhaps add a comment why bounds need to be set as CreatePanelWithBounds
already used the bounds param to create the panel? I had to think about that one
a bit.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:45: CreatePanelParams
params(name, bounds, SHOW_AS_INACTIVE);
Will the tests work if panel is created as active? If not, then please comment
why this must start as INACTIVE so nobody comes along and changes it.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:51: void
DragPanelAndFinish(Panel* panel, const gfx::Point& delta) {
Comment explaining what is delta. Ditto for DragPanelAndCancel. Maybe the 2
methods should share dragging code as it's the same except for the last line?

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:392: //   docked:    P3  P2 
P1
This is different from how you commented the panels in the overflow tests.
Should stick to the overflow style for consistency.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:506: docked_position2.x() -
detached_position3.x() + 10, 210);
This formula for X is non-obvious. Perhaps this test should decide where to go
using mouse position rather than deltas? It was not as big of a problem in the
tests above as those aren't positioning relative to other panels, just the
bottom of the dock.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:530: docked_position1.x() -
detached_position2.x() + 10, 210);
ditto

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:15: const int
PanelDragController::kDetachDockedPanelThreshold = 80;
Was there a spec change? I remember these as 60 and 30.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:55: // when drag leaves docked
strip and enters detached strip (see comment in
There's no comment in CanDragToDetachedStrip about special computation for
target panel position. It's computed identically whether you're detaching or
attaching...

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:57: gfx::Point
target_panel_position;
Should have ComputeDragTargetStrip return target_panel_bounds. That also makes
GetOffsetToDockedAreaBottom clearer (or even unnecessary as you're already
dup-ing so much code between CanDragToDocked/DetachedStrip anyways that one more
dup'ed line won't make much difference).

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:94:
panel_manager_->MovePanelToStrip(
bounds have not been restored yet. docked_strip::AddPanel won't have the bounds
info in the panel to correctly position the panel in the list.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:105: // We need to clear the
preview mode first because in this mode, the panel
discard
end preview mode
end dragging

More readable to do all cleanup, then end dragging.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:159: if
(dragging_panel_->expansion_state() != Panel::EXPANDED)
if dragging_panel_->IsMinimized()

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:176: const gfx::Point&
target_panel_position) const {
Should make this take a rect. It's confusing to say offset from a point to the
bottom, especially when the distance is NOT between the point and  the bottom of
the dock.

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:48: // Used to figure out if
the panel can be dragged to other strip.
Comment what the params and return values mean.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:49: PanelStrip*
ComputeDragTagetStrip(
typo: Taget

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:56: int
GetOffsetToDockedAreaBottom(
Needs comment. Unclear what this is.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:73: gfx::Point
offset_from_mouse_location_on_drag_start_;
This is a delta of some sort? I don't understand the comment to know what it's
measuring.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:73: // the caller should only call one of
RestorePanelToSavedPlacement and
s/and/or

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:89: virtual void
DragPanelWithinStrip(Panel* panel, int delta_x, int delta_y) = 0;
why deltas instead of mouse position?

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:95: // If |aborted| is true, |panel| will
stay as it is; otherwise, it will be
Perhaps the flag should be finalize_position instead?

[jennb, 2012-03-08 23:44:19]

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

http://codereview.chromium.org/9546001/diff/36001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:637: [self
updateWindowLevel];
You've ignored the isFullScreen param...

[jianli, 2012-03-09 21:48:58]

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:122: void
DetachedPanelStrip::StartDraggingPanelWithinStrip(Panel* panel) {
On 2012/03/08 23:41:09, jennb wrote:
> dcheck panel is in strip?

Done.

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:91: if (panel->initialized()) {
On 2012/03/08 23:41:09, jennb wrote:
> if (panel !initialized)
>   insert newly created
> else if (preview mode)
>   insert preview
> else
>   insert existing
> 
> Easier to read than nested ifs.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:111: 
On 2012/03/08 23:41:09, jennb wrote:
> Extra blank line.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:316: // If this panel is already
in the right position, nothing to do.
On 2012/03/08 23:41:09, jennb wrote:
> nit: rephrase the if-comment to match the if-condition

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:339: saved_panel_placement_.panel
= NULL;
On 2012/03/08 23:41:09, jennb wrote:
> Should clear out saved pointer to left_panel too.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:429:
dragging_panel_current_iterator_ = panels_.end();
On 2012/03/08 23:41:09, jennb wrote:
> Shame that you lose this iterator right before you might need it again in
> RestorePanelToSavedPlacement. Maybe you can copy it to saved panel  placement
> for use by the restored code upon cancel?

Not sure I understand this comment. I want to separate the logic between
dragging and placement loading/saving. No matter how the dragging is done, the
placement loading/saving will not be affected and might be served for other
purpose.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:603: if (state ==
Panel::TITLE_ONLY && panel->in_preview_mode())
On 2012/03/08 23:41:09, jennb wrote:
> Keep old code here? It's obvious you want to leave title-bar up when dragging
a
> panel, but not so obvious why preview-mode needs to show the title...

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.cc:810: // TODO(jianli): Need to
handle the case that panel in preview mode could be
On 2012/03/08 23:41:09, jennb wrote:
> crbug this and reference bug # here pls.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
File chrome/browser/ui/panels/docked_panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:122: // closed.
On 2012/03/08 23:41:09, jennb wrote:
> or moved out of the dock (e.g. to overflow).

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/do...
chrome/browser/ui/panels/docked_panel_strip.h:139: void EnsureAvailableSpace(int
width);
On 2012/03/08 23:41:09, jennb wrote:
> Should not need this now that you've synced to latest code.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:324: // True if this panel should stay always
on top of other windows.
On 2012/03/08 23:41:09, jennb wrote:
> s/stay always/always stay

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:327: // True if this panel is in preview mode.
That is, panel bounds should not be
On 2012/03/08 23:41:09, jennb wrote:
> s/That is,/When in preview mode

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:329: // add a panel to the strip without
causing its bounds changed.
On 2012/03/08 23:41:09, jennb wrote:
> s/changed/to change

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_browser_view.cc (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_browser_view.cc:525: // The panel should not show
app icon in the desktop bar if it is in overflow.
On 2012/03/08 23:41:09, jennb wrote:
> Delete. If you need such a comment, it belongs in the place that calls this
> method, presumably, the overflow strip.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:14: class
PanelDragBrowserTest : public BasePanelBrowserTest {
On 2012/03/08 23:41:09, jennb wrote:
> Is the intent to only have inter-strip drag tests in this file? If so, please
> comment/rename as such. PanelInterStripDragTest? Can drop Browser from test
name
> to keep length manageable.

No. This test is for both inter-strip and intra-strip drag testing. After this
patch is landed, I am going to move all intra-strip drag tests to this file.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:39:
panel->SetPanelBounds(bounds);
On 2012/03/08 23:41:09, jennb wrote:
> Move this after the EXPECT_EQ as that expect is checking the MovePanelToStrip
> call. Perhaps add a comment why bounds need to be set as CreatePanelWithBounds
> already used the bounds param to create the panel? I had to think about that
one
> a bit.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:45: CreatePanelParams
params(name, bounds, SHOW_AS_INACTIVE);
On 2012/03/08 23:41:09, jennb wrote:
> Will the tests work if panel is created as active? If not, then please comment
> why this must start as INACTIVE so nobody comes along and changes it.

Changed to use expected_active_state so that we can still pass SHOW_AS_ACTIVE.
Added comment.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:51: void
DragPanelAndFinish(Panel* panel, const gfx::Point& delta) {
On 2012/03/08 23:41:09, jennb wrote:
> Comment explaining what is delta. Ditto for DragPanelAndCancel. Maybe the 2
> methods should share dragging code as it's the same except for the last line?

DragPanelAndCancel is indeed not used and thus I removed it. Added comment for
delta.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:392: //   docked:    P3  P2 
P1
On 2012/03/08 23:41:09, jennb wrote:
> This is different from how you commented the panels in the overflow tests.
> Should stick to the overflow style for consistency.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:506: docked_position2.x() -
detached_position3.x() + 10, 210);
On 2012/03/08 23:41:09, jennb wrote:
> This formula for X is non-obvious. Perhaps this test should decide where to go
> using mouse position rather than deltas? It was not as big of a problem in the
> tests above as those aren't positioning relative to other panels, just the
> bottom of the dock.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:530: docked_position1.x() -
detached_position2.x() + 10, 210);
On 2012/03/08 23:41:09, jennb wrote:
> ditto

Done.

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:15: const int
PanelDragController::kDetachDockedPanelThreshold = 80;
On 2012/03/08 23:41:09, jennb wrote:
> Was there a spec change? I remember these as 60 and 30.

Changed to use the values in spec.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:55: // when drag leaves docked
strip and enters detached strip (see comment in
On 2012/03/08 23:41:09, jennb wrote:
> There's no comment in CanDragToDetachedStrip about special computation for
> target panel position. It's computed identically whether you're detaching or
> attaching...

Outdated comment removed.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:57: gfx::Point
target_panel_position;
On 2012/03/08 23:41:09, jennb wrote:
> Should have ComputeDragTargetStrip return target_panel_bounds. That also makes
> GetOffsetToDockedAreaBottom clearer (or even unnecessary as you're already
> dup-ing so much code between CanDragToDocked/DetachedStrip anyways that one
more
> dup'ed line won't make much difference).

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:94:
panel_manager_->MovePanelToStrip(
On 2012/03/08 23:41:09, jennb wrote:
> bounds have not been restored yet. docked_strip::AddPanel won't have the
bounds
> info in the panel to correctly position the panel in the list.

Per discussion, we added a new parameter to MovePanelToStrip.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:105: // We need to clear the
preview mode first because in this mode, the panel
On 2012/03/08 23:41:09, jennb wrote:
> discard
> end preview mode
> end dragging
> 
> More readable to do all cleanup, then end dragging.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:159: if
(dragging_panel_->expansion_state() != Panel::EXPANDED)
On 2012/03/08 23:41:09, jennb wrote:
> if dragging_panel_->IsMinimized()

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:176: const gfx::Point&
target_panel_position) const {
On 2012/03/08 23:41:09, jennb wrote:
> Should make this take a rect. It's confusing to say offset from a point to the
> bottom, especially when the distance is NOT between the point and  the bottom
of
> the dock.

Done.

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

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:48: // Used to figure out if
the panel can be dragged to other strip.
On 2012/03/08 23:41:09, jennb wrote:
> Comment what the params and return values mean.

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:49: PanelStrip*
ComputeDragTagetStrip(
On 2012/03/08 23:41:09, jennb wrote:
> typo: Taget

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:56: int
GetOffsetToDockedAreaBottom(
On 2012/03/08 23:41:09, jennb wrote:
> Needs comment. Unclear what this is.

Removed since it is not needed now.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.h:73: gfx::Point
offset_from_mouse_location_on_drag_start_;
On 2012/03/08 23:41:09, jennb wrote:
> This is a delta of some sort? I don't understand the comment to know what it's
> measuring.

Comment updated.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_strip.h (right):

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:73: // the caller should only call one of
RestorePanelToSavedPlacement and
On 2012/03/08 23:41:09, jennb wrote:
> s/and/or

Done.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:89: virtual void
DragPanelWithinStrip(Panel* panel, int delta_x, int delta_y) = 0;
On 2012/03/08 23:41:09, jennb wrote:
> why deltas instead of mouse position?

If mouse location, the strip has to keep track of last mouse location to compute
the delta.

http://codereview.chromium.org/9546001/diff/33001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_strip.h:95: // If |aborted| is true, |panel| will
stay as it is; otherwise, it will be
On 2012/03/08 23:41:09, jennb wrote:
> Perhaps the flag should be finalize_position instead?

I would prefer to use |abort| because for now the panel is moved to its
finalized position when |abort| is false. But in the future, we might add some
more things to end the drag.

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

http://codereview.chromium.org/9546001/diff/36001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:637: [self
updateWindowLevel];
On 2012/03/08 23:44:19, jennb wrote:
> You've ignored the isFullScreen param...

Yes since updateWindowLevel checks
windowShim_->panel()->manager()->is_full_screen().

[jennb, 2012-03-12 22:43:00]

LGTM

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:413: // The panel is first
created as docked panel that causes the specified
bad grammar. Suggest "docked panel, which ignores the specified origin in
|bounds|."

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.h (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.h:85: Panel*
CreateOverflowPanel(const std::string& name, const gfx::Rect& bounds);
Should comment that the caller is responsible for setting up the scenario that
causes this panel to be an overflow panel.

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_browsertest.cc:38: // Create one
detached panel.
nit: Comment not needed anymore.

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:133:
panel->SetPanelBoundsInstantly(new_bounds);
Why no animation?

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:485: gfx::Point
drag_to_location(panel5->GetBounds().x() + 10,
Much more readable, thanks!

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:511: DragPanelByDelta(panel2,
drag_to_location2);
Did you mean DragPanelToMouseLocation here?

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:533: DragPanelByDelta(panel1,
drag_to_location3);
ditto

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:96:
static_cast<PanelStrip::PositioningMask>(
nit: A var might make this more readable than static casting here.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:106: // The saved placement is
not needed.
nit: s/not needed/no longer needed

[jianli, 2012-03-13 00:21:39]

Will land with final patch.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.cc (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.cc:413: // The panel is first
created as docked panel that causes the specified
On 2012/03/12 22:43:00, jennb wrote:
> bad grammar. Suggest "docked panel, which ignores the specified origin in
> |bounds|."

Done.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
File chrome/browser/ui/panels/base_panel_browser_test.h (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/ba...
chrome/browser/ui/panels/base_panel_browser_test.h:85: Panel*
CreateOverflowPanel(const std::string& name, const gfx::Rect& bounds);
On 2012/03/12 22:43:00, jennb wrote:
> Should comment that the caller is responsible for setting up the scenario that
> causes this panel to be an overflow panel.

Done.

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_browsertest.cc:38: // Create one
detached panel.
On 2012/03/12 22:43:00, jennb wrote:
> nit: Comment not needed anymore.

Done.

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/de...
chrome/browser/ui/panels/detached_panel_strip.cc:133:
panel->SetPanelBoundsInstantly(new_bounds);
On 2012/03/12 22:43:00, jennb wrote:
> Why no animation?

When we're dragging a panel via mouse moving, the panel should jump immediately
to the right position without any animation because otherwise the user will see
some kind of artifact images.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel_drag_browsertest.cc (right):

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:511: DragPanelByDelta(panel2,
drag_to_location2);
On 2012/03/12 22:43:00, jennb wrote:
> Did you mean DragPanelToMouseLocation here?

Done.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_browsertest.cc:533: DragPanelByDelta(panel1,
drag_to_location3);
On 2012/03/12 22:43:00, jennb wrote:
> ditto

Done.

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

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:96:
static_cast<PanelStrip::PositioningMask>(
On 2012/03/12 22:43:00, jennb wrote:
> nit: A var might make this more readable than static casting here.

Done.

http://codereview.chromium.org/9546001/diff/43001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel_drag_controller.cc:106: // The saved placement is
not needed.
On 2012/03/12 22:43:00, jennb wrote:
> nit: s/not needed/no longer needed

Done.

[commit-bot: I haz the power, 2012-03-13 03:48:17]

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

[commit-bot: I haz the power, 2012-03-13 05:12:12]

Change committed as 126339