Adding menu item to take screenshot

Adding menu item to take screenshot

BUG=196658
Committed: https://crrev.com/4e4e08a606978ebd83ea28176d82af9d806effe0
Cr-Commit-Position: refs/heads/master@{#314639}

[caelyn, 2015-02-03 18:50:31]

caelyn@chromium.org changed reviewers:
+ mfomitchev@chromium.org

[caelyn, 2015-02-03 18:50:32]

Hi Mikhail,

I have the screenshot menu item working with no obscuring open menu. One
question, should this only be showing up in menus for chromeos?

[mfomitchev, 2015-02-03 21:33:32]

LGTM!
Perhaps sky can do the owner review.

> I have the screenshot menu item working with no obscuring open menu. One
> question, should this only be showing up in menus for chromeos?

Yes, we are using ash facilities, so this is CrOS specific.

https://codereview.chromium.org/885673004/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/885673004/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1061: (screenshot_menu_item_ &&
screenshot_menu_item_->IsSelected()))) {
Awesome! Nice find.

[caelyn, 2015-02-03 21:41:06]

New patchsets have been uploaded after l-g-t-m from mfomitchev@chromium.org

[caelyn, 2015-02-03 21:41:43]

caelyn@chromium.org changed reviewers:
+ sky@chromium.org

[caelyn, 2015-02-03 21:41:43]

sky@chromium.org: Please review changes to add a menu item for taking a
screenshot.

[sky, 2015-02-04 17:06:01]

LGTM with the following fixed.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands_chromeos.cc (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
no copyright.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands_chromeos.h (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
nit: no copyright.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:8: namespace chrome {
no need for namespace here.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:10: // Takes a screenshot of the
entire desktop (not just the browser window)
nit: end with period.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:796: screenshot_menu_item_(NULL),
nullptr

[mfomitchev, 2015-02-04 18:15:48]

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands_chromeos.h (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:8: namespace chrome {
On 2015/02/04 17:06:01, sky wrote:
> no need for namespace here.

@sky: Sorry - browser_commands.h uses the chrome namespace. Why wouldn't we use
the same namespace for browser_commands_chromeos?

[sky, 2015-02-04 19:03:43]

On 2015/02/04 18:15:48, mfomitchev wrote:
>
https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
> File chrome/browser/ui/browser_commands_chromeos.h (right):
> 
>
https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
> chrome/browser/ui/browser_commands_chromeos.h:8: namespace chrome {
> On 2015/02/04 17:06:01, sky wrote:
> > no need for namespace here.
> 
> @sky: Sorry - browser_commands.h uses the chrome namespace. Why wouldn't we
use
> the same namespace for browser_commands_chromeos?

It's historical. See: 
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/chrome...

[mfomitchev, 2015-02-04 19:50:49]

On 2015/02/04 19:03:43, sky wrote:
> On 2015/02/04 18:15:48, mfomitchev wrote:
> >
>
https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
> > File chrome/browser/ui/browser_commands_chromeos.h (right):
> > 
> >
>
https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
> > chrome/browser/ui/browser_commands_chromeos.h:8: namespace chrome {
> > On 2015/02/04 17:06:01, sky wrote:
> > > no need for namespace here.
> > 
> > @sky: Sorry - browser_commands.h uses the chrome namespace. Why wouldn't we
> use
> > the same namespace for browser_commands_chromeos?
> 
> It's historical. See: 
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/chrome...

Good to know, thanks for the link!

[caelyn, 2015-02-04 20:29:06]

New patchsets have been uploaded after l-g-t-m from sky@chromium.org

[caelyn, 2015-02-04 20:30:12]

All nits addressed. Will now be committing.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands_chromeos.cc (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/02/04 17:06:01, sky wrote:
> no copyright.

Done.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands_chromeos.h (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/02/04 17:06:01, sky wrote:
> nit: no copyright.

Done.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:8: namespace chrome {
On 2015/02/04 17:06:01, sky wrote:
> no need for namespace here.

Done.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands_chromeos.h:10: // Takes a screenshot of the
entire desktop (not just the browser window)
On 2015/02/04 17:06:01, sky wrote:
> nit: end with period.

Done.

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/885673004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/wrench_menu.cc:796: screenshot_menu_item_(NULL),
On 2015/02/04 17:06:01, sky wrote:
> nullptr

changing all of these to nullptr while I am here. (I followed the pattern of the
other items in case there was some reason we were using NULL instead of
nullptr).

[caelyn, 2015-02-04 20:30:32]

The CQ bit was checked by caelyn@chromium.org

[commit-bot: I haz the power, 2015-02-04 20:31:11]

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

[commit-bot: I haz the power, 2015-02-04 21:28:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-04 21:29:36]

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