Adding a menu item to take a screenshot under "More Tools" submenu in the Wrench menu.

Adding a menu item to take a screenshot under "More Tools" submenu in the Wrench menu. 

The behavior is equivalent to taking the screenshot via VKEY_PRINT keyboard shortcut.

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

[mfomitchev, 2014-01-09 19:44:38]

Can you guys please have a quick look? I will add owners later. Thanks!

[tdanderson, 2014-01-09 20:28:39]

Hi Mikhail, please see my comments below.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:689: #if defined(OS_CHROMEOS)
If you are surrounding the body of TakeScreenshot() with #if
defined(OS_CHROMEOS) (see my suggestion in browser_commands.cc) then I think you
can remove the ifdef here.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:960:
command_updater_.UpdateCommandEnabled(IDC_TAKE_SCREENSHOT, true);
I think this line should be ok as-is given that you're only adding the "take
screenshot" item to the wrench menu in the first place for CrOS.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:9: #include "ash/shell.h"
These three #includes should be inside an #if defined(OS_CHROMEOS)

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:948: void TakeScreenshot() {
Surround the body of this function with an #if defined(OS_CHROMEOS) since this
is calls into ash-specific code. For non-CrOS this makes this function a noop.

A similar thing has been done for TogglePagePinnedToStartScreen() for Windows,
as an example.

[mfomitchev, 2014-01-09 21:11:45]

Fixing ifdefs

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_command_controller.cc (right):

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:689: #if defined(OS_CHROMEOS)
On 2014/01/09 20:28:40, tdanderson wrote:
> If you are surrounding the body of TakeScreenshot() with #if
> defined(OS_CHROMEOS) (see my suggestion in browser_commands.cc) then I think
you
> can remove the ifdef here.

Done.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_command_controller.cc:960:
command_updater_.UpdateCommandEnabled(IDC_TAKE_SCREENSHOT, true);
On 2014/01/09 20:28:40, tdanderson wrote:
> I think this line should be ok as-is given that you're only adding the "take
> screenshot" item to the wrench menu in the first place for CrOS.

Done.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:9: #include "ash/shell.h"
On 2014/01/09 20:28:40, tdanderson wrote:
> These three #includes should be inside an #if defined(OS_CHROMEOS)

Done.

https://codereview.chromium.org/132233003/diff/1/chrome/browser/ui/browser_co...
chrome/browser/ui/browser_commands.cc:948: void TakeScreenshot() {
On 2014/01/09 20:28:40, tdanderson wrote:
> Surround the body of this function with an #if defined(OS_CHROMEOS) since this
> is calls into ash-specific code. For non-CrOS this makes this function a noop.
> 
> A similar thing has been done for TogglePagePinnedToStartScreen() for Windows,
> as an example. 

Done.

[tdanderson, 2014-01-09 21:25:29]

LGTM with one nit. I suggest adding sky@ as an OWNERS reviewer when you're
ready.

https://codereview.chromium.org/132233003/diff/90001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/90001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:11: #endif
Move this block after the #if defined(OS_WIN) block below.

[mfomitchev, 2014-01-09 21:27:59]

sky@chromium.org: Please review changes in 

Can you please take a look? Thanks!

[mfomitchev, 2014-01-09 21:32:30]

Moving ifdef in browser_commands.cc

https://codereview.chromium.org/132233003/diff/90001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/90001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:11: #endif
On 2014/01/09 21:25:29, tdanderson wrote:
> Move this block after the #if defined(OS_WIN) block below.

Done.

[sadrul, 2014-01-09 21:39:32]

The code LG. A couple of comments:

https://codereview.chromium.org/132233003/diff/160001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1383: +          T&ake screenshot
Should we clarify that this will take screenshot of the entire desktop, and not
just the browser window? Because as a user, I would expect a 'Take screenshot'
item on a window menu to take screenshot of that window, and not the whole
desktop.

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:952: #if defined(OS_CHROMEOS)
I was going to suggest using USE_ASH instead of OS_CHROMEOS. But looks like the
accelerators for taking screenshot are also available on chromeos only (I wonder
why). So this is OK

[mfomitchev, 2014-01-09 22:07:23]

https://codereview.chromium.org/132233003/diff/160001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/app/generated_re...
chrome/app/generated_resources.grd:1383: +          T&ake screenshot
Agreed. I thought about this as well and discussed with Rick and Terry, but
couldn't come up with a label which I liked much. I've pinged Kuscher on this
one - let's see what he says. Let me know if you have any specific suggestions
you like, I will run them by Kuscher as well.

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:952: #if defined(OS_CHROMEOS)
I think that's because on other OSes there are other means to take a screenshot,
like 'Print Scrn' on Windows. I guess this is seen as OS functionality rather
than browser functionality (which makes it weird to put the menu item within the
browser menu, but we don't really have a better place to put it now).

[sky, 2014-01-10 00:02:52]

https://codereview.chromium.org/132233003/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/132233003/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:114: ScreenshotDelegate*
screenshot_delegate() const {
Generally you want const methods to return const objects. Otherwise you can
often circumvent the const'ness. Make this non-const.

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:951: void TakeScreenshot() {
If this is chromeos specific, then move to browser_commands_chromeos.

[mfomitchev, 2014-01-10 18:06:50]

https://codereview.chromium.org/132233003/diff/160001/ash/accelerators/accele...
File ash/accelerators/accelerator_controller.h (right):

https://codereview.chromium.org/132233003/diff/160001/ash/accelerators/accele...
ash/accelerators/accelerator_controller.h:114: ScreenshotDelegate*
screenshot_delegate() const {
On 2014/01/10 00:02:52, sky wrote:
> Generally you want const methods to return const objects. Otherwise you can
> often circumvent the const'ness. Make this non-const.

Done.

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/132233003/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:951: void TakeScreenshot() {
On 2014/01/10 00:02:52, sky wrote:
> If this is chromeos specific, then move to browser_commands_chromeos.

Done.

[sky, 2014-01-10 19:12:31]

LGTM

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_chromeos.cc (right):

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_chromeos.cc:28: 
Nuke all these empty lines.

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_chromeos.h (right):

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_chromeos.h:4: 
Only one empty line here.

[mfomitchev, 2014-01-10 19:45:57]

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_chromeos.cc (right):

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_chromeos.cc:28: 
On 2014/01/10 19:12:32, sky wrote:
> Nuke all these empty lines.

Done.

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands_chromeos.h (right):

https://codereview.chromium.org/132233003/diff/280001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands_chromeos.h:4: 
On 2014/01/10 19:12:32, sky wrote:
> Only one empty line here.

Done.

[commit-bot: I haz the power, 2014-01-10 19:49:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/132233003/410001

[commit-bot: I haz the power, 2014-01-10 21:42:52]

Change committed as 244248