[mac] Implement dragging of multiple tabs.

[mac] Implement dragging of multiple tabs to a new window or another window.

BUG=10899
TEST=Cmd-click to select multiple tabs, drag up or down to detach, drop tabs to another window or make a new window.

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

[Andre, 2014-02-12 22:56:37]

https://codereview.chromium.org/157403004/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/157403004/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1324: tabFrame =
[[dragController window] convertRectToScreen:tabFrame];
Changed from [view window] to [dragController window], because [view window] is
the child overlay window and did not result in the right target coordinate for
the dropped tabs.

https://codereview.chromium.org/157403004/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1325: tabFrame = [[self
window] convertRectFromScreen:tabFrame];
Change to new API, -convertBaseToScreen: and -convertScreenToBase: are
deprecated.

[Andre, 2014-02-20 23:05:26]

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1322: tabOrigin =
[[dragController window] convertBaseToScreen:tabOrigin];
Changed from [view window] to [dragController window], because [view window] is
the child overlay window and did not result in the right target coordinate for
the dropped tabs.

[Robert Sesek, 2014-02-24 15:25:20]

Nice! This is a lot less than I expected.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (left):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1293: // Moving between
windows. Figure out the WebContents to drop into our tab
I'd maybe keep the comment about "Moving between windows" since that's what this
if is about. Does not hurt o repeat it on line 1304, though.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1389: DCHECK([tabViews
count] > 0);
DCHECK_GT

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1430: // Create the new
window with a single tab in its model, the one being
No longer just a "single tab" :)

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:182: // by the |contents|,
but the |pinned| state is the caller's responsibility.
Should also discuss what |activate| does.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:195: -
(NSArray*)selectedViews;
Comment all public declarations.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:787: -
(NSArray*)selectedViews {
Header order should match declaration order.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1818: (activate ?
TabStripModel::ADD_ACTIVE : 0) |
Use ADD_NONE instead of 0, like you do in the other file.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:390: [dropController
moveTabViews:@[ [dropController activeTabView] ]
Does this mean you cannot drag multiple tabs within the same window?

[Andre, 2014-02-25 01:16:16]

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (left):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1293: // Moving between
windows. Figure out the WebContents to drop into our tab
On 2014/02/24 15:25:20, rsesek wrote:
> I'd maybe keep the comment about "Moving between windows" since that's what
this
> if is about. Does not hurt o repeat it on line 1304, though.

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1389: DCHECK([tabViews
count] > 0);
On 2014/02/24 15:25:20, rsesek wrote:
> DCHECK_GT

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1430: // Create the new
window with a single tab in its model, the one being
On 2014/02/24 15:25:20, rsesek wrote:
> No longer just a "single tab" :)

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.h (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:182: // by the |contents|,
but the |pinned| state is the caller's responsibility.
On 2014/02/24 15:25:20, rsesek wrote:
> Should also discuss what |activate| does.

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.h:195: -
(NSArray*)selectedViews;
On 2014/02/24 15:25:20, rsesek wrote:
> Comment all public declarations.

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:787: -
(NSArray*)selectedViews {
On 2014/02/24 15:25:20, rsesek wrote:
> Header order should match declaration order.

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:1818: (activate ?
TabStripModel::ADD_ACTIVE : 0) |
On 2014/02/24 15:25:20, rsesek wrote:
> Use ADD_NONE instead of 0, like you do in the other file.

Done.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:390: [dropController
moveTabViews:@[ [dropController activeTabView] ]
On 2014/02/24 15:25:20, rsesek wrote:
> Does this mean you cannot drag multiple tabs within the same window?

Right, I have only implemented it for a different window.
I can work on supporting the same window with a different CL, but it does not
seem nearly as useful.

[Robert Sesek, 2014-02-25 21:06:46]

LGTM

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:787: -
(NSArray*)selectedViews {
On 2014/02/25 01:16:16, Andre wrote:
> On 2014/02/24 15:25:20, rsesek wrote:
> > Header order should match declaration order.
> 
> Done.

Not yet. The order of the methods in the .h should match the .mm, which still
isn't true. Although this file is not really great about doing that, apparently,
so maybe just drop this comment.

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm (right):

https://codereview.chromium.org/157403004/diff/210001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tabs/tab_strip_drag_controller.mm:390: [dropController
moveTabViews:@[ [dropController activeTabView] ]
On 2014/02/25 01:16:16, Andre wrote:
> On 2014/02/24 15:25:20, rsesek wrote:
> > Does this mean you cannot drag multiple tabs within the same window?
> 
> Right, I have only implemented it for a different window.
> I can work on supporting the same window with a different CL, but it does not
> seem nearly as useful.

That's fine. I think it'd mostly be used to organize tabs within a window. Maybe
check what Aura/ChromeOS does?

[Andre, 2014-02-25 23:07:07]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2014-02-26 00:23:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andresantoso@chromium.org/157403004/30...

[Paweł Hajdan Jr., 2014-02-26 05:32:12]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-26 05:41:34]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-26 06:01:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/andresantoso@chromium.org/157403004/30...

[commit-bot: I haz the power, 2014-02-26 15:42:19]

Change committed as 253476