Added "Exit full screen" to context menu.

Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".

Committed: https://crrev.com/8490591edaffefb62a0dbbace192115092dab908
Cr-Commit-Position: refs/heads/master@{#404589}

[Matt Giuca, 2016-07-07 06:09:35]

Description was changed from

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
==========

to

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background, should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background, should see "Exit full screen
     ... F11".
==========

[Matt Giuca, 2016-07-07 06:27:55]

Description was changed from

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background, should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background, should see "Exit full screen
     ... F11".
==========

to

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".
==========

[Matt Giuca, 2016-07-07 06:35:14]

mgiuca@chromium.org changed reviewers:
+ mpearson@chromium.org, sky@chromium.org

[Matt Giuca, 2016-07-07 06:35:15]

mpearson: Please review changes in histograms.xml.

sky: Please review changes in rest of the files.

Note: I could avoid much of the complexity (getting the browser
AcceleratorProvider) by just hard-coding F11 here, like all the other shortcuts
do, but I think it's wrong to hard-code it. I might try and reuse that for the
other shortcuts later on.

See bug for screenshots, UI review (ainslie).

[Mark P, 2016-07-07 16:55:31]

histograms.xml lgtm

[sky, 2016-07-07 17:00:41]

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:309: {88, -1,
IDC_EXIT_FULLSCREEN},
Based upon most of these constants starting with IDC_CONTENT_CONTEXT I would
have expected you to go with IDC_CONTENT_CONTEXT_EXIT_FULLSCREEN. Is there a
reason not to?

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1214: return;
I had question about conditionally adding the item, email sent.

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:138:
} else {
nit: no else after return.

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:144:
return accelerator_provider->GetAcceleratorForCommandId(IDC_FULLSCREEN,
It isn't obvious why you're using the accelerator for FULLSCREEN here. Please
add a comment.

[Matt Giuca, 2016-07-08 00:55:46]

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:309: {88, -1,
IDC_EXIT_FULLSCREEN},
On 2016/07/07 17:00:41, sky wrote:
> Based upon most of these constants starting with IDC_CONTENT_CONTEXT I would
> have expected you to go with IDC_CONTENT_CONTEXT_EXIT_FULLSCREEN. Is there a
> reason not to?

Done. That's a good point. Also moved the actual value of this constant (in
chrome_command_ids.h so it lives among the other CONTENT_CONTEXT constants).

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:1214: return;
On 2016/07/07 17:00:41, sky wrote:
> I had question about conditionally adding the item, email sent.

Acknowledged.

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:138:
} else {
On 2016/07/07 17:00:41, sky wrote:
> nit: no else after return.

Done.

https://codereview.chromium.org/2116973002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:144:
return accelerator_provider->GetAcceleratorForCommandId(IDC_FULLSCREEN,
On 2016/07/07 17:00:41, sky wrote:
> It isn't obvious why you're using the accelerator for FULLSCREEN here. Please
> add a comment.

Done.

[sky, 2016-07-08 15:35:49]

LGTM

[Matt Giuca, 2016-07-11 00:21:12]

The CQ bit was checked by mgiuca@chromium.org

[Matt Giuca, 2016-07-11 00:21:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
mpearson@chromium.org
Link to the patchset: https://codereview.chromium.org/2116973002/#ps180001
(title: "Do not show an accelerator for user-triggered fullscreen on Chrome
OS.")

[commit-bot: I haz the power, 2016-07-11 00:21:24]

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, 2016-07-11 01:17:26]

Description was changed from

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".
==========

to

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".
==========

[commit-bot: I haz the power, 2016-07-11 01:17:27]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-07-11 01:19:32]

Description was changed from

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".
==========

to

==========
Added "Exit full screen" to context menu.

This item is only shown when in fullscreen, and allows users to exit
fullscreen using the mouse (for users who don't have a keyboard
attached).

This is still not foolproof (as it won't work on a site that captures
the right-click menu) but should work in the majority of cases.

BUG=594868
TEST=https://permission.site. Click Fullscreen. Right-click background; should
     see "Exit full screen ... Esc".
TEST=F11 to go fullscreen. Right-click background; should see
     "Exit full screen ... F11".

Committed: https://crrev.com/8490591edaffefb62a0dbbace192115092dab908
Cr-Commit-Position: refs/heads/master@{#404589}
==========

[commit-bot: I haz the power, 2016-07-11 01:19:33]

Patchset 10 (id:??) landed as
https://crrev.com/8490591edaffefb62a0dbbace192115092dab908
Cr-Commit-Position: refs/heads/master@{#404589}