Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu.
The option only appears if the current window is not tabbed.

BUG=43953
Committed: https://crrev.com/9d60fcef1fbaf9cb53b1c4a08be17183c0670d9b
Cr-Commit-Position: refs/heads/master@{#374690}

[spqchan, 2016-02-10 00:55:17]

Description was changed from

==========
Added the "Show as Tab" option for popups on OSX

BUG=43953
==========

to

==========
Added the "Show as Tab" option for popups on OSX

BUG=43953
==========

[spqchan, 2016-02-10 00:55:17]

spqchan@chromium.org changed reviewers:
+ erikchen@chromium.org

[spqchan, 2016-02-10 00:56:17]

PTAL

[spqchan, 2016-02-10 00:59:29]

Description was changed from

==========
Added the "Show as Tab" option for popups on OSX

BUG=43953
==========

to

==========
Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu. 
The option only appears if the current window is not tabbed.

BUG=43953
==========

[Avi (use Gerrit), 2016-02-10 01:00:15]

I reviewed a patch to do the same thing
(https://codereview.chromium.org/1248183004), and it wasn't super good at
keeping the item up to date. In particular:

- Open a page that has a popup link
- Click it to make a popup
- Close the original page
- Close the popup (without converting it)

Left the "convert to tab" visible. Your version gets this right?

[spqchan, 2016-02-10 01:06:37]

On 2016/02/10 01:00:15, Avi wrote:
> I reviewed a patch to do the same thing
> (https://codereview.chromium.org/1248183004), and it wasn't super good at
> keeping the item up to date. In particular:
> 
> - Open a page that has a popup link
> - Click it to make a popup
> - Close the original page
> - Close the popup (without converting it)
> 
> Left the "convert to tab" visible. Your version gets this right?

Just tested using the steps, and the item is disabled but is still visible.
I'll work on a fix for that. Thanks for the heads up

[spqchan, 2016-02-10 02:34:28]

On 2016/02/10 01:06:37, spqchan wrote:
> On 2016/02/10 01:00:15, Avi wrote:
> > I reviewed a patch to do the same thing
> > (https://codereview.chromium.org/1248183004), and it wasn't super good at
> > keeping the item up to date. In particular:
> > 
> > - Open a page that has a popup link
> > - Click it to make a popup
> > - Close the original page
> > - Close the popup (without converting it)
> > 
> > Left the "convert to tab" visible. Your version gets this right?
> 
> Just tested using the steps, and the item is disabled but is still visible.
> I'll work on a fix for that. Thanks for the heads up

Fixed the issue.
It turns out |validateUserInterfaceItem| doesn’t get called in
BrowserWindowCommandHandler when there are no key windows. 
Instead, appController's |validateUserInterfaceItem| gets called.

[spqchan, 2016-02-10 02:34:48]

PTAL 

Fixed the issue.
It turns out |validateUserInterfaceItem| doesn’t get called in
BrowserWindowCommandHandler when there are no key windows. 
Instead, appController's |validateUserInterfaceItem| gets called.

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:989: if (tag == IDC_SHOW_AS_TAB) {
Note: I'm handling this outside of the switch statement because we want this to
hidden, whether the command is enabled or not.

[erikchen, 2016-02-10 03:13:13]

lgtm

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:226: [menuItem
setHidden:browser->is_type_tabbed()];
Missing break for this case, and the one above it?

[Avi (use Gerrit), 2016-02-10 06:05:04]

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:989: if (tag == IDC_SHOW_AS_TAB) {
On 2016/02/10 02:34:48, spqchan wrote:
> Note: I'm handling this outside of the switch statement because we want this
to
> hidden, whether the command is enabled or not.

If you have to note it in a comment to me, is it subtle enough to be called out
in a comment in the file? :)

[spqchan, 2016-02-10 17:27:06]

PTAL

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
File chrome/browser/app_controller_mac.mm (right):

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/app_cont...
chrome/browser/app_controller_mac.mm:989: if (tag == IDC_SHOW_AS_TAB) {
On 2016/02/10 06:05:04, Avi wrote:
> On 2016/02/10 02:34:48, spqchan wrote:
> > Note: I'm handling this outside of the switch statement because we want this
> to
> > hidden, whether the command is enabled or not.
> 
> If you have to note it in a comment to me, is it subtle enough to be called
out
> in a comment in the file? :)

Fair enough

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_command_handler.mm (right):

https://codereview.chromium.org/1681863005/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_command_handler.mm:226: [menuItem
setHidden:browser->is_type_tabbed()];
On 2016/02/10 03:13:13, erikchen wrote:
> Missing break for this case, and the one above it?

Done.

[Avi (use Gerrit), 2016-02-10 17:51:01]

I think this is cool. LGTM

[spqchan, 2016-02-10 17:52:08]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-02-10 17:52:08]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/1681863005/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-02-10 17:56:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1681863005/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1681863005/60001

[commit-bot: I haz the power, 2016-02-10 18:30:57]

Description was changed from

==========
Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu. 
The option only appears if the current window is not tabbed.

BUG=43953
==========

to

==========
Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu. 
The option only appears if the current window is not tabbed.

BUG=43953
==========

[commit-bot: I haz the power, 2016-02-10 18:30:58]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-02-10 18:32:20]

Description was changed from

==========
Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu. 
The option only appears if the current window is not tabbed.

BUG=43953
==========

to

==========
Added the "Show as Tab" option for popups on OSX

Added the "Show as Tab" option under the Window menu.
The option only appears if the current window is not tabbed.

BUG=43953

Committed: https://crrev.com/9d60fcef1fbaf9cb53b1c4a08be17183c0670d9b
Cr-Commit-Position: refs/heads/master@{#374690}
==========

[commit-bot: I haz the power, 2016-02-10 18:32:21]

Patchset 4 (id:??) landed as
https://crrev.com/9d60fcef1fbaf9cb53b1c4a08be17183c0670d9b
Cr-Commit-Position: refs/heads/master@{#374690}