Clicking tab close with option key close the other tabs.

Clicking tab close with option key close the other tabs.

BUG=91319
TEST=Open several tabs and click the close button of one of them with option key. Closes other tabs.
TEST=Open several tabs and click the close button of one of them without option key. Closes the tab.


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

[bashi, 2011-08-03 15:09:57]

Hi,

This CL might be not good but I couldn't come up with other way to implement
this feature.  Could you take a look?

Thanks,

[Robert Sesek, 2011-08-03 15:25:56]

On 2011/08/03 15:09:57, bashik wrote:
> Hi,
> 
> This CL might be not good but I couldn't come up with other way to implement
> this feature.  Could you take a look?
> 
> Thanks,

I'm not sure we want to implement this. The bug should go through the triage
process first for a decision.

[bashi, 2011-08-05 01:48:44]

On 2011/08/03 15:25:56, rsesek wrote:
> On 2011/08/03 15:09:57, bashik wrote:
> > Hi,
> > 
> > This CL might be not good but I couldn't come up with other way to implement
> > this feature.  Could you take a look?
> > 
> > Thanks,
> 
> I'm not sure we want to implement this. The bug should go through the triage
> process first for a decision.

alcor replied that it's worth to implement.  Could you take another look?  (I
guess there might be more appropriate way to implement this, though)

[Robert Sesek, 2011-08-05 11:13:12]

I think you can do this without any ivars in -[TabView mouseUp:].

[bashi, 2011-08-08 05:24:25]

Hi rsesek,

On 2011/08/05 11:13:12, rsesek wrote:
> I think you can do this without any ivars in -[TabView mouseUp:].

That was my first attempt to implement this. However, when I set a breakpoint at
[TabView mouseUp:] and click close button on a tab, the breakpoint didn't hit.
According to chrome/app/nibs/TabView.xib, it seems that mouse events on close
button convert to closeTab: message during event loop.

[Robert Sesek, 2011-08-08 15:38:18]

On 2011/08/08 05:24:25, bashik wrote:
> Hi rsesek,
> 
> On 2011/08/05 11:13:12, rsesek wrote:
> > I think you can do this without any ivars in -[TabView mouseUp:].
> 
> That was my first attempt to implement this. However, when I set a breakpoint
at
> [TabView mouseUp:] and click close button on a tab, the breakpoint didn't hit.
> According to chrome/app/nibs/TabView.xib, it seems that mouse events on close
> button convert to closeTab: message during event loop.

Ok. You can still do this without ivars by examining [NSApp currentEvent] in
-[TabController closeTab:].

[Peter Kasting, 2011-08-08 17:30:31]

Drive-by: What about option-middle-clicking on tabs?  Does that work the same
way?  Should it?

[Robert Sesek, 2011-08-08 21:49:54]

On 2011/08/08 17:30:31, Peter Kasting wrote:
> Drive-by: What about option-middle-clicking on tabs?  Does that work the same
> way?  Should it?

I think that would be more accident-prone than this and not all that helpful. We
already have a lot of ways to close tabs :).

[Peter Kasting, 2011-08-08 21:54:43]

On 2011/08/08 21:49:54, rsesek wrote:
> On 2011/08/08 17:30:31, Peter Kasting wrote:
> > Drive-by: What about option-middle-clicking on tabs?  Does that work the
same
> > way?  Should it?
> 
> I think that would be more accident-prone than this and not all that helpful.
We
> already have a lot of ways to close tabs :).

The reason I ask is because middle-click on a tab is supposed to behave exactly
like clicking a tab close box, so not making these two match would be (IMO) a
major mistake.  Personally I think the entire option-click behavior here is
bizarre, but given that it's apparently a system standard, I think we really
ought to be consistent and extend it to middle-clicks; I don't see how
option-middle-click on a tab to "close other tabs" is inherently any more
accident-prone than option-left-click on a tab close box to do the same thing,
unless you think it's likely that people would be option-middle-clicking a lot
of other things (I don't).

[Robert Sesek, 2011-08-08 21:59:56]

I just checked and implementing this properly should mean that middle click or
the close button should both do the right thing.

[Peter Kasting, 2011-08-08 22:10:37]

On 2011/08/08 21:59:56, rsesek wrote:
> I just checked and implementing this properly should mean that middle click or
> the close button should both do the right thing.

Thanks for checking that.

[bashi, 2011-08-09 01:49:36]

On 2011/08/08 15:38:18, rsesek wrote:
> Ok. You can still do this without ivars by examining [NSApp currentEvent] in
> -[TabController closeTab:].

This is what I'm looking for.  I'll revise the CL.  Thanks!

[Robert Sesek, 2011-08-09 15:20:37]

What about the unit tests?

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:147: if ([[NSApp currentEvent] 
modifierFlags] & NSAlternateKeyMask) {
nit: extra spaces

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:148: if ([[self target]
respondsToSelector:@selector(closeOtherTabs:)]) {
Combine these two if's with &&

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:748: -
(void)closeOtherTabs:(id)sender {
Add this to the TabControllerTarget protocol.

[bashi, 2011-08-10 06:52:29]

Thank you for review. Added a unit test.

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller.mm (right):

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:147: if ([[NSApp currentEvent] 
modifierFlags] & NSAlternateKeyMask) {
On 2011/08/09 15:20:37, rsesek wrote:
> nit: extra spaces

Done.

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller.mm:148: if ([[self target]
respondsToSelector:@selector(closeOtherTabs:)]) {
On 2011/08/09 15:20:37, rsesek wrote:
> Combine these two if's with &&

Done.

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/7565007/diff/11003/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:748: -
(void)closeOtherTabs:(id)sender {
On 2011/08/09 15:20:37, rsesek wrote:
> Add this to the TabControllerTarget protocol.

Done.

[Robert Sesek, 2011-08-10 16:00:34]

http://codereview.chromium.org/7565007/diff/14001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm (right):

http://codereview.chromium.org/7565007/diff/14001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm:154: [controller
setAction:@selector(closeOtherTabs:)];
I think this should really be testing that the TabController sends
-closeOtherTabs: to the target when the modifier keys are set right.

[bashi, 2011-08-11 08:50:46]

On 2011/08/10 16:00:34, rsesek wrote:
> I think this should really be testing that the TabController sends
> -closeOtherTabs: to the target when the modifier keys are set right.

How can I set [NSApp currentEvent] in unittest?  I tried to send mouse down/up
event to the [controller view] but it didn't call [controller_ closeTab:]
because |closeButton_| is hidden and [controller_ inRapidClosureMode] returns
false.

[Robert Sesek, 2011-08-12 14:27:48]

On 2011/08/11 08:50:46, bashik wrote:
> On 2011/08/10 16:00:34, rsesek wrote:
> > I think this should really be testing that the TabController sends
> > -closeOtherTabs: to the target when the modifier keys are set right.
> 
> How can I set [NSApp currentEvent] in unittest?  I tried to send mouse down/up
> event to the [controller view] but it didn't call [controller_ closeTab:]
> because |closeButton_| is hidden and [controller_ inRapidClosureMode] returns
> false.

Sorry for the delay; this slipped off my radar yesterday. You should try -[NSApp
sendEvent:]. If you send a mouseDown you'll probably also need to send a mouseUp
to cancel the tracking loop. The mouseTimer: logic is similar to what you'll
need, but you'll also need to set the modifier flags in the mouseUp event.

[bashi, 2011-08-12 15:20:27]

Hi rsesek,

Thank you for your kind reply.  I'm on vacation so I'll try it later.  Thanks!

On 2011/08/12 14:27:48, rsesek wrote:
> Sorry for the delay; this slipped off my radar yesterday. You should try
-[NSApp
> sendEvent:]. If you send a mouseDown you'll probably also need to send a
mouseUp
> to cancel the tracking loop. The mouseTimer: logic is similar to what you'll
> need, but you'll also need to set the modifier flags in the mouseUp event.

[bashi, 2011-08-17 10:11:26]

On 2011/08/12 14:27:48, rsesek wrote:
> On 2011/08/11 08:50:46, bashik wrote:
> > On 2011/08/10 16:00:34, rsesek wrote:
> > > I think this should really be testing that the TabController sends
> > > -closeOtherTabs: to the target when the modifier keys are set right.
> > 
> > How can I set [NSApp currentEvent] in unittest?  I tried to send mouse
down/up
> > event to the [controller view] but it didn't call [controller_ closeTab:]
> > because |closeButton_| is hidden and [controller_ inRapidClosureMode]
returns
> > false.
> 
> Sorry for the delay; this slipped off my radar yesterday. You should try
-[NSApp
> sendEvent:]. If you send a mouseDown you'll probably also need to send a
mouseUp
> to cancel the tracking loop. The mouseTimer: logic is similar to what you'll
> need, but you'll also need to set the modifier flags in the mouseUp event.

This didn't work, too. I set a breakpoint at [CrApplication :sendEvent] and
continued execution by "step" command, but nothing happened; just returned to
TabControllerTest::CloseOtherTabs().

[Robert Sesek, 2011-08-18 00:31:49]

On 2011/08/17 10:11:26, bashik wrote:
> This didn't work, too. I set a breakpoint at [CrApplication :sendEvent] and
> continued execution by "step" command, but nothing happened; just returned to
> TabControllerTest::CloseOtherTabs().

Bummer. Try [NSWindow sendEvent:]. If that doesn't work, the first version of
your test would be OK.

[bashi, 2011-08-18 02:22:33]

On 2011/08/18 00:31:49, rsesek wrote:
> Bummer. Try [NSWindow sendEvent:]. If that doesn't work, the first version of
> your test would be OK.

I already tried to use [NSWindow sendEvent:] and it didn't work, too...

[Robert Sesek, 2011-08-18 04:53:42]

On 2011/08/18 02:22:33, bashik wrote:
> On 2011/08/18 00:31:49, rsesek wrote:
> > Bummer. Try [NSWindow sendEvent:]. If that doesn't work, the first version
of
> > your test would be OK.
> 
> I already tried to use [NSWindow sendEvent:] and it didn't work, too...

I thought I had sample code that had this working, but I don't know where it
went. How about this approach?

http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/coc...

Though I'd use base/auto_reset.h instead of manually managing NSApp.

[bashi, 2011-08-19 05:59:59]

On 2011/08/18 04:53:42, rsesek wrote:
> I thought I had sample code that had this working, but I don't know where it
> went. How about this approach?
> 
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/coc...

This approach works fine.  Thank you so much for your help!  Revised the CL.

[Robert Sesek, 2011-08-19 15:36:25]

LGTM. Nice work!

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm (right):

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm:168: id fakeApp =
[OCMockObject partialMockForObject:NSApp];
I had to figure out how this worked. Add a comment like this:

// OCMock will swap NSApp internally and will restore it when the mock gets
// deallocated. This happens when the autorelease pool drains at the end of the
// test.

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:750: if
(tabStripModel_->ContainsIndex(index))
nit: braces {} around multi-line bodies

[bashi, 2011-08-22 06:43:48]

Thank you for review!

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm (right):

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_controller_unittest.mm:168: id fakeApp =
[OCMockObject partialMockForObject:NSApp];
On 2011/08/19 15:36:25, rsesek wrote:
> I had to figure out how this worked. Add a comment like this:
> 
> // OCMock will swap NSApp internally and will restore it when the mock gets
> // deallocated. This happens when the autorelease pool drains at the end of
the
> // test.

Done.

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
File chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm (right):

http://codereview.chromium.org/7565007/diff/26001/chrome/browser/ui/cocoa/tab...
chrome/browser/ui/cocoa/tabs/tab_strip_controller.mm:750: if
(tabStripModel_->ContainsIndex(index))
On 2011/08/19 15:36:25, rsesek wrote:
> nit: braces {} around multi-line bodies

Done.

[commit-bot: I haz the power, 2011-08-23 07:09:36]

Change committed as 97828