cocoa: add some tab-manipulation menu items

cocoa: add some tab-manipulation menu items

This change adds these items to the Window menu:
* Duplicate Tab
* Pin Tab
* Mute Tab

The latter two are toggle-able menu items.

BUG=699667
Review-Url: https://codereview.chromium.org/2876223003
Cr-Commit-Position: refs/heads/master@{#473202}
Committed: https://chromium.googlesource.com/chromium/src/+/0101ba06c70108d8165c8e2f53e8f3069ab9478c

[Elly Fong-Jones, 2017-05-12 20:01:09]

Description was changed from

==========
cocoa: add some tab-manipulation menu items

This change adds these items to the Window menu:
* Duplicate Tab
* Pin Tab
* Mute Tab

The latter two are toggle-able menu items.

BUG=699667
==========

to

==========
cocoa: add some tab-manipulation menu items

This change adds these items to the Window menu:
* Duplicate Tab
* Pin Tab
* Mute Tab

The latter two are toggle-able menu items.

BUG=699667
==========

[Elly Fong-Jones, 2017-05-12 20:01:09]

ellyjones@chromium.org changed reviewers:
+ rsesek@chromium.org, sky@chromium.org

[Elly Fong-Jones, 2017-05-12 20:01:43]

rsesek: ptal at chrome/browser/ui/cocoa changes and chrome/app/nibs/MainMenu.xib
delta
sky: ptal at other chrome/browser/ui stuff

thanks :)

[sky, 2017-05-14 15:45:03]

https://codereview.chromium.org/2876223003/diff/1/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_DUPLICATE_TAB 35012
Can you keep the alignment as it was?

What's the difference between IDC_WINDOW_DUPLICATE_TAB and IDC_DUPLICATE_TAB?

https://codereview.chromium.org/2876223003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:416: bool WillContextMenuMute(int
index);
Can this be private?

[Robert Sesek, 2017-05-15 18:44:03]

https://codereview.chromium.org/2876223003/diff/1/chrome/app/nibs/MainMenu.xib
File chrome/app/nibs/MainMenu.xib (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/app/nibs/MainMenu.xi...
chrome/app/nibs/MainMenu.xib:2: <document
type="com.apple.InterfaceBuilder3.Cocoa.XIB" version="3.0" toolsVersion="11542"
systemVersion="16F71b" targetRuntime="MacOSX.Cocoa"
propertyAccessControl="none">
Does presubmit not complain about these lines? (You may be able to just un-stage
them).

[shrike, 2017-05-15 20:23:22]

shrike@chromium.org changed reviewers:
+ shrike@chromium.org

[shrike, 2017-05-15 20:23:23]

ellyjones@ - two comments. First, I think you should get rid of the divider
between Select Previous Tab and Duplicate. Second, I think "Duplicate" needs to
read "Duplicate Tab". I don't know if you left it out to avoid the menu
repeating "Tab" a million times but I think it the command needs to be explicit
about what it duplicates (this is the Window menu - does Duplicate duplicate the
current tab or the entire window?).

[Elly Fong-Jones, 2017-05-16 19:05:15]

rsesek, sky: ptal?
shrike: okay - I'll add some new IDS_ constants for those strings with "Tab"
appended.

https://codereview.chromium.org/2876223003/diff/1/chrome/app/chrome_command_i...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/app/chrome_command_i...
chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_DUPLICATE_TAB 35012
On 2017/05/14 15:45:03, sky wrote:
> Can you keep the alignment as it was?

Done.

> What's the difference between IDC_WINDOW_DUPLICATE_TAB and IDC_DUPLICATE_TAB?

Hm, there isn't one, actually. Same for IDC_WINDOW_RELOAD_TAB. I removed both.

https://codereview.chromium.org/2876223003/diff/1/chrome/app/nibs/MainMenu.xib
File chrome/app/nibs/MainMenu.xib (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/app/nibs/MainMenu.xi...
chrome/app/nibs/MainMenu.xib:2: <document
type="com.apple.InterfaceBuilder3.Cocoa.XIB" version="3.0" toolsVersion="11542"
systemVersion="16F71b" targetRuntime="MacOSX.Cocoa"
propertyAccessControl="none">
On 2017/05/15 18:44:03, Robert Sesek wrote:
> Does presubmit not complain about these lines? (You may be able to just
un-stage
> them).

Done.

https://codereview.chromium.org/2876223003/diff/1/chrome/browser/ui/tabs/tab_...
File chrome/browser/ui/tabs/tab_strip_model.h (right):

https://codereview.chromium.org/2876223003/diff/1/chrome/browser/ui/tabs/tab_...
chrome/browser/ui/tabs/tab_strip_model.h:416: bool WillContextMenuMute(int
index);
On 2017/05/14 15:45:03, sky wrote:
> Can this be private?

No :( it's called from BrowserWindowCommandHandler so the menus can reflect
whether they will mute or unmute.

[Robert Sesek, 2017-05-16 19:47:41]

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_MUTE_TAB 35012
nit: alignment

https://codereview.chromium.org/2876223003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2876223003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:648: 
nit: remove blank line (not present between other cases)

[sky, 2017-05-17 16:30:12]

LGTM

[Elly Fong-Jones, 2017-05-17 17:56:17]

shrike: ptal? :)

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_MUTE_TAB 35012
On 2017/05/16 19:47:41, Robert Sesek wrote:
> nit: alignment

Done.

https://codereview.chromium.org/2876223003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/2876223003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_command_controller.cc:648: 
On 2017/05/16 19:47:41, Robert Sesek wrote:
> nit: remove blank line (not present between other cases)

Done.

[Robert Sesek, 2017-05-17 18:14:11]

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_MUTE_TAB 35012
On 2017/05/17 17:56:16, Elly Fong-Jones wrote:
> On 2017/05/16 19:47:41, Robert Sesek wrote:
> > nit: alignment
> 
> Done.

Not done. Is this clang-format..?

[Elly Fong-Jones, 2017-05-17 18:43:45]

On 2017/05/17 18:14:11, Robert Sesek wrote:
>
https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
> File chrome/app/chrome_command_ids.h (right):
> 
>
https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
> chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_MUTE_TAB 35012
> On 2017/05/17 17:56:16, Elly Fong-Jones wrote:
> > On 2017/05/16 19:47:41, Robert Sesek wrote:
> > > nit: alignment
> > 
> > Done.
> 
> Not done. Is this clang-format..?

Argh! It is. Sorry about that.

[Elly Fong-Jones, 2017-05-18 19:06:03]

On 2017/05/17 18:43:45, Elly Fong-Jones wrote:
> On 2017/05/17 18:14:11, Robert Sesek wrote:
> >
>
https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
> > File chrome/app/chrome_command_ids.h (right):
> > 
> >
>
https://codereview.chromium.org/2876223003/diff/20001/chrome/app/chrome_comma...
> > chrome/app/chrome_command_ids.h:86: #define IDC_WINDOW_MUTE_TAB 35012
> > On 2017/05/17 17:56:16, Elly Fong-Jones wrote:
> > > On 2017/05/16 19:47:41, Robert Sesek wrote:
> > > > nit: alignment
> > > 
> > > Done.
> > 
> > Not done. Is this clang-format..?
> 
> Argh! It is. Sorry about that.

Okay, it is better now. rsesek, ptal? :)

[shrike, 2017-05-18 19:32:05]

On 2017/05/17 17:56:17, Elly Fong-Jones wrote:
> shrike: ptal? :)

The menu items look good. I think you should remove the separator that's between
the "Select Previous Tab" and "Duplicate Tab" items so that all of the
tab-related items are in the same group. Or is there a reason you want it to be
there?

[Elly Fong-Jones, 2017-05-18 19:48:35]

On 2017/05/18 19:32:05, shrike wrote:
> On 2017/05/17 17:56:17, Elly Fong-Jones wrote:
> > shrike: ptal? :)
> 
> The menu items look good. I think you should remove the separator that's
between
> the "Select Previous Tab" and "Duplicate Tab" items so that all of the
> tab-related items are in the same group. Or is there a reason you want it to
be
> there?

Removed :)

I just added separators around the "batch" of new items I was adding, with no
particular rhyme or reason.

[Robert Sesek, 2017-05-18 20:47:26]

LGTM

[shrike, 2017-05-18 22:16:51]

lgtm

[Elly Fong-Jones, 2017-05-19 13:30:37]

The CQ bit was checked by ellyjones@chromium.org

[Elly Fong-Jones, 2017-05-19 13:30:38]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2876223003/#ps80001
(title: "remove top separator")

[commit-bot: I haz the power, 2017-05-19 13:30:57]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 15:03:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 15:03:28]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Elly Fong-Jones, 2017-05-19 15:04:09]

The CQ bit was checked by ellyjones@chromium.org

[commit-bot: I haz the power, 2017-05-19 15:04:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 15:50:33]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495206249749500,
"parent_rev": "c5b0d3fffea7f2b8a54718903d82f221ca204d85", "commit_rev":
"0101ba06c70108d8165c8e2f53e8f3069ab9478c"}

[commit-bot: I haz the power, 2017-05-19 15:50:44]

Description was changed from

==========
cocoa: add some tab-manipulation menu items

This change adds these items to the Window menu:
* Duplicate Tab
* Pin Tab
* Mute Tab

The latter two are toggle-able menu items.

BUG=699667
==========

to

==========
cocoa: add some tab-manipulation menu items

This change adds these items to the Window menu:
* Duplicate Tab
* Pin Tab
* Mute Tab

The latter two are toggle-able menu items.

BUG=699667

Review-Url: https://codereview.chromium.org/2876223003
Cr-Commit-Position: refs/heads/master@{#473202}
Committed:
https://chromium.googlesource.com/chromium/src/+/0101ba06c70108d8165c8e2f53e8...
==========

[commit-bot: I haz the power, 2017-05-19 15:50:45]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0101ba06c70108d8165c8e2f53e8...