Change panel drag related methods to use mouse location in screen coordinates.

Change panel drag related methods to use mouse location in screen coordinates.

This is in preparation for support inter-strip dragging.

BUG=none
TEST=existing tests due to no functionality change


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

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

[jennb, 2012-03-07 00:17:46]

There seems to be some mix up in the coordinate system for mac.

http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:443: if (dragState_ ==
PANEL_DRAG_IN_PROGRESS)
Do you need this check? The start of this method already returns if state is
PANEL_DRAG_SUPPRESSED. The remaining states are either CAN_START, which will
change the state to IN_PROGRESS, or it's already IN_PROGRESS.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:412: gfx::Point
test_dragging_mouse_location_;
Why a member var instead of a local var where this is used?

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:830:
panel1_testing->PressLeftMouseButtonTitlebar(panel1->GetBounds().origin());
use mouse_location here

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_drag_controller.h:47: // The mouse location, in
screen coordinates, when StartDragging or Drag is
s/is called previously/was previously called

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.h (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.h:125: -
(void)pressLeftMouseButtonTitlebar:(NSPoint)mouseLocation;
Are these using screen coordinates? NativePanel say these are in screen
coordinates. PanelBrowserWindowCocoa converts these to Cocoa Coordinates before
calling these methods. The .mm file for this class convertsFromScreenToBase on
these coordinates in pressLeftMouseButtonTitlebar but uses them directly as
screen coordinates in dragTitlebar.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:416:
cocoa_utils::ConvertPointFromCocoaCoordinates(mouseLocation));
The .h said mouseLocation was already in screen coordinates. Which is it? Ditto
for drag().

[jianli, 2012-03-07 00:46:49]

http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm (right):

http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm:443: if (dragState_ ==
PANEL_DRAG_IN_PROGRESS)
On 2012/03/07 00:17:46, jennb wrote:
> Do you need this check? The start of this method already returns if state is
> PANEL_DRAG_SUPPRESSED. The remaining states are either CAN_START, which will
> change the state to IN_PROGRESS, or it's already IN_PROGRESS.

This is needed because we might have bug if startDrag could fail to proceed with
the drag.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_browsertest.cc (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:412: gfx::Point
test_dragging_mouse_location_;
On 2012/03/07 00:17:46, jennb wrote:
> Why a member var instead of a local var where this is used?

This is because the helper function TestDragging is called to trigger maybe just
one stage of the dragging process, like only StartDragging. To handle this, we
need to remember the mouse location where TestDragging is called last time. I am
going to fix this once we move all drag related tests to
panel_drag_browsertests. Added a TODO here.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_browsertest.cc:830:
panel1_testing->PressLeftMouseButtonTitlebar(panel1->GetBounds().origin());
On 2012/03/07 00:17:46, jennb wrote:
> use mouse_location here

Done.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_drag_controller.h (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_drag_controller.h:47: // The mouse location, in
screen coordinates, when StartDragging or Drag is
On 2012/03/07 00:17:46, jennb wrote:
> s/is called previously/was previously called

Done.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_titlebar_view_cocoa.h (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_titlebar_view_cocoa.h:125: -
(void)pressLeftMouseButtonTitlebar:(NSPoint)mouseLocation;
On 2012/03/07 00:17:46, jennb wrote:
> Are these using screen coordinates? NativePanel say these are in screen
> coordinates. PanelBrowserWindowCocoa converts these to Cocoa Coordinates
before
> calling these methods. The .mm file for this class convertsFromScreenToBase on
> these coordinates in pressLeftMouseButtonTitlebar but uses them directly as
> screen coordinates in dragTitlebar.

Per discussion, added comment.

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
File chrome/browser/ui/panels/panel_window_controller_cocoa.mm (right):

http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/pan...
chrome/browser/ui/panels/panel_window_controller_cocoa.mm:416:
cocoa_utils::ConvertPointFromCocoaCoordinates(mouseLocation));
On 2012/03/07 00:17:46, jennb wrote:
> The .h said mouseLocation was already in screen coordinates. Which is it?
Ditto
> for drag().

Added comment.

[jennb, 2012-03-07 00:49:41]

LGTM

On Tue, Mar 6, 2012 at 4:46 PM, <jianli@chromium.org> wrote:

>
> http://codereview.chromium.**org/9616037/diff/2001/chrome/**
>
browser/ui/panels/panel_**titlebar_view_cocoa.mm<http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm>
> File
chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm<http://panel_titlebar_view_cocoa.mm>(right):
>
> http://codereview.chromium.**org/9616037/diff/2001/chrome/**
>
browser/ui/panels/panel_**titlebar_view_cocoa.mm#**newcode443<http://codereview.chromium.org/9616037/diff/2001/chrome/browser/ui/panels/panel_titlebar_view_cocoa.mm#newcode443>
>
chrome/browser/ui/panels/panel**_titlebar_view_cocoa.mm:443<http://panel_titlebar_view_cocoa.mm:443>:
> if
> (dragState_ == PANEL_DRAG_IN_PROGRESS)
> On 2012/03/07 00:17:46, jennb wrote:
>
>> Do you need this check? The start of this method already returns if
>>
> state is
>
>> PANEL_DRAG_SUPPRESSED. The remaining states are either CAN_START,
>>
> which will
>
>> change the state to IN_PROGRESS, or it's already IN_PROGRESS.
>>
>
> This is needed because we might have bug if startDrag could fail to
> proceed with the drag.
>
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**browsertest.cc<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_browsertest.cc>
> File chrome/browser/ui/panels/**panel_browsertest.cc (right):
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**browsertest.cc#newcode412<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_browsertest.cc#newcode412>
> chrome/browser/ui/panels/**panel_browsertest.cc:412: gfx::Point
> test_dragging_mouse_location_;
> On 2012/03/07 00:17:46, jennb wrote:
>
>> Why a member var instead of a local var where this is used?
>>
>
> This is because the helper function TestDragging is called to trigger
> maybe just one stage of the dragging process, like only StartDragging.
> To handle this, we need to remember the mouse location where
> TestDragging is called last time. I am going to fix this once we move
> all drag related tests to panel_drag_browsertests. Added a TODO here.
>
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**browsertest.cc#newcode830<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_browsertest.cc#newcode830>
> chrome/browser/ui/panels/**panel_browsertest.cc:830:
> panel1_testing->**PressLeftMouseButtonTitlebar(**
> panel1->GetBounds().origin());
> On 2012/03/07 00:17:46, jennb wrote:
>
>> use mouse_location here
>>
>
> Done.
>
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_drag_**controller.h<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_drag_controller.h>
> File chrome/browser/ui/panels/**panel_drag_controller.h (right):
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_drag_**controller.h#newcode47<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_drag_controller.h#newcode47>
> chrome/browser/ui/panels/**panel_drag_controller.h:47: // The mouse
> location, in screen coordinates, when StartDragging or Drag is
> On 2012/03/07 00:17:46, jennb wrote:
>
>> s/is called previously/was previously called
>>
>
> Done.
>
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**titlebar_view_cocoa.h<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_titlebar_view_cocoa.h>
> File chrome/browser/ui/panels/**panel_titlebar_view_cocoa.h (right):
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**titlebar_view_cocoa.h#**newcode125<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_titlebar_view_cocoa.h#newcode125>
> chrome/browser/ui/panels/**panel_titlebar_view_cocoa.h:**125: -
> (void)**pressLeftMouseButtonTitlebar:(**NSPoint)mouseLocation;
> On 2012/03/07 00:17:46, jennb wrote:
>
>> Are these using screen coordinates? NativePanel say these are in
>>
> screen
>
>> coordinates. PanelBrowserWindowCocoa converts these to Cocoa
>>
> Coordinates before
>
>> calling these methods. The .mm file for this class
>>
> convertsFromScreenToBase on
>
>> these coordinates in pressLeftMouseButtonTitlebar but uses them
>>
> directly as
>
>> screen coordinates in dragTitlebar.
>>
>
> Per discussion, added comment.
>
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**window_controller_cocoa.mm<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_window_controller_cocoa.mm>
> File
chrome/browser/ui/panels/panel**_window_controller_cocoa.mm<http://panel_window_controller_cocoa.mm>(right):
>
> http://codereview.chromium.**org/9616037/diff/4002/chrome/**
>
browser/ui/panels/panel_**window_controller_cocoa.mm#**newcode416<http://codereview.chromium.org/9616037/diff/4002/chrome/browser/ui/panels/panel_window_controller_cocoa.mm#newcode416>
>
chrome/browser/ui/panels/panel**_window_controller_cocoa.mm:**416<http://panel_window_controller_cocoa.mm:416>
> :
> cocoa_utils::**ConvertPointFromCocoaCoordinat**es(mouseLocation));
> On 2012/03/07 00:17:46, jennb wrote:
>
>> The .h said mouseLocation was already in screen coordinates. Which is
>>
> it? Ditto
>
>> for drag().
>>
>
> Added comment.
>
>
http://codereview.chromium.**org/9616037/<http://codereview.chromium.org/9616...
>

[commit-bot: I haz the power, 2012-03-07 00:58:26]

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

[commit-bot: I haz the power, 2012-03-07 03:55:52]

Change committed as 125319

[commit-bot: I haz the power, 2012-03-12 18:40:22]

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

[commit-bot: I haz the power, 2012-03-12 21:31:26]

Change committed as 126235