Add ui::GrabDesktopSnapshot for Windows.

Add ui::GrabDesktopSnapshot for Windows.

GrabWindowSnapshot(NULL) had been used previously to get the desktop,
but this doesn't work for Aura builds.

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

[sky, 2013-04-16 22:06:49]

https://codereview.chromium.org/13926006/diff/1/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/13926006/diff/1/ui/snapshot/snapshot.h#newcode34
ui/snapshot/snapshot.h:34: // Grabs a snapshot of the desktop. No security
checks are done.  This is
If this is for tests, why is it here and not in the test?

[grt (UTC plus 2), 2013-04-17 16:58:17]

PTAL, thanks.

https://codereview.chromium.org/13926006/diff/1/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/13926006/diff/1/ui/snapshot/snapshot.h#newcode34
ui/snapshot/snapshot.h:34: // Grabs a snapshot of the desktop. No security
checks are done.  This is
On 2013/04/16 22:06:49, sky wrote:
> If this is for tests, why is it here and not in the test?

Good point. I've moved GrabDesktopShortcut into a new test support lib
(snapshot_test_support). To avoid code duplication, it calls into its own
linked-in copy of the guts of the non-Aura GrabWindowSnapshot.

https://codereview.chromium.org/13926006/diff/7007/chrome/chrome_tests_unit.gypi
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/13926006/diff/7007/chrome/chrome_tests_unit.g...
chrome/chrome_tests_unit.gypi:292: 'cloud_policy_proto',
This dependency is required to build this target due to
browser/policy/cloud/policy_builder.cc.

[grt (UTC plus 2), 2013-04-18 22:58:18]

sky: friendly ping

[sky, 2013-04-22 16:25:18]

Sorry, I'm a bit confused now. What exactly are you trying to accomplish?

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot.gyp
File ui/snapshot/snapshot.gyp (right):

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot.gyp#n...
ui/snapshot/snapshot.gyp:27: 'snapshot_hwnd_win.cc',
Why is this file both here and in test support?

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot_deskt...
File ui/snapshot/snapshot_desktop.h (right):

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot_deskt...
ui/snapshot/snapshot_desktop.h:20: std::vector<unsigned char>*
png_representation,
style guide says out params are last.

[grt (UTC plus 2), 2013-04-22 19:14:03]

The goal is to get desktop snapshots working in tests in Aura builds. They
previously worked in non-Aura builds by passing a NULL gfx::NativeWindow to
GrabWindowSnapshot. This happened to work in non-Aura builds since NULL HWND is
the desktop. In Aura builds, a NativeWindow is an aura::Window*, so this NULL
trick no longer works. This here change exposes a GrabDesktopSnapshot method
(based on the non-Aura GrabWindowSnapshot guts) in snapshot_test_support.

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot.gyp
File ui/snapshot/snapshot.gyp (right):

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot.gyp#n...
ui/snapshot/snapshot.gyp:27: 'snapshot_hwnd_win.cc',
On 2013/04/22 16:25:19, sky wrote:
> Why is this file both here and in test support?

The desktop snapshotting code (in 'snapshot_test_support' target) is identical
to the non-Aura window snapshotting code (in 'snapshot' target). To avoid source
code duplication, I pulled this implementation out of snapshot_win and into
snapshot_hwnd_win, and include it in both targets. It is omitted from 'snapshot'
for Aura builds, but is unconditionally present in 'snapshot_test_support' so
that desktop snapshotting can be done from tests. I experimented with having it
appear in only one or the other (and never both), but found that the gyp hijinks
to make that work was too confusing. I've moved desktop_snapshot.{cc,h} into a
test/ subdir to make it more explicit that these are test-only files.

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot_deskt...
File ui/snapshot/snapshot_desktop.h (right):

https://codereview.chromium.org/13926006/diff/7007/ui/snapshot/snapshot_deskt...
ui/snapshot/snapshot_desktop.h:20: std::vector<unsigned char>*
png_representation,
On 2013/04/22 16:25:19, sky wrote:
> style guide says out params are last.

Done.

[sky, 2013-04-22 23:16:11]

Ah, ok. I think a better approach here is:
. create snapshot_win.h, it'll have you're GrabHwndSnapshot.
. snapshot_win.cc will only define GrabViewSnapshot if not-aura.
. snapshot_win.cc will now always be compiled.

Sorry for the confusion.

[grt (UTC plus 2), 2013-04-23 01:49:42]

On 2013/04/22 23:16:11, sky wrote:
> Ah, ok. I think a better approach here is:
> . create snapshot_win.h, it'll have you're GrabHwndSnapshot.
> . snapshot_win.cc will only define GrabViewSnapshot if not-aura.
> . snapshot_win.cc will now always be compiled.
> 
> Sorry for the confusion.

One downside to this is that GrabHwndSnapshot will be unconditionally linked
into chrome.dll even though in Aura builds it will only be used by tests. Is
this acceptable? Thanks.

[sky, 2013-04-23 14:55:40]

That's fine.

On Mon, Apr 22, 2013 at 6:49 PM,  <grt@chromium.org> wrote:
> On 2013/04/22 23:16:11, sky wrote:
>>
>> Ah, ok. I think a better approach here is:
>> . create snapshot_win.h, it'll have you're GrabHwndSnapshot.
>> . snapshot_win.cc will only define GrabViewSnapshot if not-aura.
>> . snapshot_win.cc will now always be compiled.
>
>
>> Sorry for the confusion.
>
>
> One downside to this is that GrabHwndSnapshot will be unconditionally linked
> into chrome.dll even though in Aura builds it will only be used by tests. Is
> this acceptable? Thanks.
>
> https://codereview.chromium.org/13926006/

[grt (UTC plus 2), 2013-04-24 12:21:12]

PTAL.

[sky, 2013-04-24 14:49:11]

https://codereview.chromium.org/13926006/diff/29001/ui/snapshot/snapshot.gyp
File ui/snapshot/snapshot.gyp (right):

https://codereview.chromium.org/13926006/diff/29001/ui/snapshot/snapshot.gyp#...
ui/snapshot/snapshot.gyp:80: 'targets': [
Shouldn't we only include this target on windows so that you don't need to
create an empty target here?

[grt (UTC plus 2), 2013-04-24 17:44:47]

PTAL. Apologies for the extra noise in chrome_tests_unit.gypi from the rebase.
The only pertinent change is the motion of the snapshot_test_support into a
win-only block.

https://codereview.chromium.org/13926006/diff/29001/ui/snapshot/snapshot.gyp
File ui/snapshot/snapshot.gyp (right):

https://codereview.chromium.org/13926006/diff/29001/ui/snapshot/snapshot.gyp#...
ui/snapshot/snapshot.gyp:80: 'targets': [
On 2013/04/24 14:49:11, sky wrote:
> Shouldn't we only include this target on windows so that you don't need to
> create an empty target here?

There's only the Windows implementation for now, but that could change if
someone became motivated to take care of other platforms. I figured it was
easier to have one win-only condition here (with the empty target for non-win
platforms) rather than one per use of the target in other .gyp* files. There's
only one use of it at the moment, so it's not a big deal either way. I suppose
the empty target could add a little overhead to non-Win platforms, so I've
removed it.

[sky, 2013-04-24 20:52:21]

LGTM

[commit-bot: I haz the power, 2013-04-24 21:02:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grt@chromium.org/13926006/36001

[commit-bot: I haz the power, 2013-04-24 23:11:34]

Change committed as 196256