Add GrabWindowSnapshotAsync tests

Add GrabWindowSnapshotAsync aura tests

This adds a version of GrabWindowSnapshotAsync that behaves identically
to GrabWindowSnapshot except with a callback.  The snapshot aura unit
tests are updated to use this new async function instead of the sync
one.

In order to make the snapshot tests work properly, they also now have to
use a real gl context so that real drawing and readback can occur
through the compositor.

BUG=331410, 332167
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243700

[enne (OOO), 2014-01-03 23:22:39]

https://codereview.chromium.org/119753007/diff/140001/ui/aura/window_tree_hos...
File ui/aura/window_tree_host.cc (left):

https://codereview.chromium.org/119753007/diff/140001/ui/aura/window_tree_hos...
ui/aura/window_tree_host.cc:78: //             verify that this has no adverse
effects.
Unfortunately, this does have adverse effects if I run the snapshot unit tests
with a real context (as this patch adds).

~RootWindowHostX11() calls XDestroyWindow.  Later, when the compositor gets
destroyed, it flushes the command buffer, which could call
NativeViewGLSurfaceOSMesa::SwapBuffers which calls XGetWindowAttributes on the
destroyed window, getting a BadWindow error.

At least naively it seems like the compositor needs to be destroyed before the
window hierarchy.

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:166: gfx::Display::ROTATE_0,
sergeyu: This preserves previous behavior, but is it intentional that this
GrabWindowSnapshotAsync function doesn't respect rotation?

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:172: SNAPSHOT_EXPORT void GrabWindowSnapshotAsync(
My intention is for the sync GrabWindowSnapshot(...) to just become "return
false" once there are no more callers in Aura, but ScreenshotTaker and
ThroughputTest both need to be updated first.  Adding a tested
GrabWindowSnapshotAsync function is step 1.

[Sergey Ulanov, 2014-01-05 07:34:39]

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot.h#...
ui/snapshot/snapshot.h:46: SNAPSHOT_EXPORT void GrabWindowSnapshotAsync(
Maybe rename this to GrabWindowSnapshotAndScaleAsync() to make it clear how it's
different from the new method below.

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:166: gfx::Display::ROTATE_0,
On 2014/01/03 23:22:40, enne wrote:
> sergeyu: This preserves previous behavior, but is it intentional that this
> GrabWindowSnapshotAsync function doesn't respect rotation?

No, it's not intentional. I think it's a bug.

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:172: SNAPSHOT_EXPORT void GrabWindowSnapshotAsync(
On 2014/01/03 23:22:40, enne wrote:
> My intention is for the sync GrabWindowSnapshot(...) to just become "return
> false" once there are no more callers in Aura, but ScreenshotTaker and
> ThroughputTest both need to be updated first.  Adding a tested
> GrabWindowSnapshotAsync function is step 1.

That sounds awesome. Thanks!

[Ben Goodger (Google), 2014-01-06 17:46:31]

chrome, content, aura, keyboard lgtm.

[enne (OOO), 2014-01-07 00:25:24]

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot.h
File ui/snapshot/snapshot.h (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot.h#...
ui/snapshot/snapshot.h:46: SNAPSHOT_EXPORT void GrabWindowSnapshotAsync(
On 2014/01/05 07:34:40, Sergey Ulanov wrote:
> Maybe rename this to GrabWindowSnapshotAndScaleAsync() to make it clear how
it's
> different from the new method below.

Done.

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:166: gfx::Display::ROTATE_0,
On 2014/01/05 07:34:40, Sergey Ulanov wrote:
> On 2014/01/03 23:22:40, enne wrote:
> > sergeyu: This preserves previous behavior, but is it intentional that this
> > GrabWindowSnapshotAsync function doesn't respect rotation?
> 
> No, it's not intentional. I think it's a bug. 

What's the right thing to do, then?

(1) Rotate first, then scale to desired size.
(2) Scale first to desired size, then rotate.

The general GrabWindowSnapshot code does #2, but the desired size is implicit. 
I'm assuming this new GrabWindowSnapshotAndScaleAsync code should do #1?  Is
target size pre or post rotation?

I've left this unchanged for now.

[Sergey Ulanov, 2014-01-07 00:44:48]

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:166: gfx::Display::ROTATE_0,
On 2014/01/07 00:25:25, enne wrote:
> On 2014/01/05 07:34:40, Sergey Ulanov wrote:
> > On 2014/01/03 23:22:40, enne wrote:
> > > sergeyu: This preserves previous behavior, but is it intentional that this
> > > GrabWindowSnapshotAsync function doesn't respect rotation?
> > 
> > No, it's not intentional. I think it's a bug. 
> 
> What's the right thing to do, then?
> 
> (1) Rotate first, then scale to desired size.
> (2) Scale first to desired size, then rotate.
> 
> The general GrabWindowSnapshot code does #2, but the desired size is implicit.

> I'm assuming this new GrabWindowSnapshotAndScaleAsync code should do #1?  Is
> target size pre or post rotation?
> 
> I've left this unchanged for now.


target_size is the size of the result the caller gets, so it should be post
rotation. So logically it should be (1), but (2) would be more efficient (as
images are downscaled more often). You can just swap width and height when
rotation is 90 or 270, then rotation can happen after scaling.

[enne (OOO), 2014-01-07 19:49:28]

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/140001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:166: gfx::Display::ROTATE_0,
On 2014/01/07 00:44:48, Sergey Ulanov wrote:
> On 2014/01/07 00:25:25, enne wrote:
> > On 2014/01/05 07:34:40, Sergey Ulanov wrote:
> > > On 2014/01/03 23:22:40, enne wrote:
> > > > sergeyu: This preserves previous behavior, but is it intentional that
this
> > > > GrabWindowSnapshotAsync function doesn't respect rotation?
> > > 
> > > No, it's not intentional. I think it's a bug. 
> > 
> > What's the right thing to do, then?
> > 
> > (1) Rotate first, then scale to desired size.
> > (2) Scale first to desired size, then rotate.
> > 
> > The general GrabWindowSnapshot code does #2, but the desired size is
implicit.
> 
> > I'm assuming this new GrabWindowSnapshotAndScaleAsync code should do #1?  Is
> > target size pre or post rotation?
> > 
> > I've left this unchanged for now.
> 
> 
> target_size is the size of the result the caller gets, so it should be post
> rotation. So logically it should be (1), but (2) would be more efficient (as
> images are downscaled more often). You can just swap width and height when
> rotation is 90 or 270, then rotation can happen after scaling.

Done.

https://codereview.chromium.org/119753007/diff/350001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura_unittest.cc (right):

https://codereview.chromium.org/119753007/diff/350001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura_unittest.cc:127:
helper_->RunAllPendingInMessageLoop();
Also, fixed the multithreaded case (chromeos=1 vs use_aura=1) by running the
message loop here.  This wasn't needed in singlethreaded mode because of the
WaitForDraw loop, but in the multithreaded case there's only one draw and this
causes it to block forever.  This is a lot more explicit and less hacky than a
loop.

[danakj, 2014-01-07 23:30:17]

This change also fixes test crashes I'm seeing due to Xerrors in single-process
mode when I switch browser tests over to using the real composited path. Yay.

[Sergey Ulanov, 2014-01-08 02:24:16]

lgtm

https://codereview.chromium.org/119753007/diff/490001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/490001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:202: scoped_ptr<cc::CopyOutputRequest> request =
It looks like a lot of code in this function is duplicated in
GrabWindowSnapshotAndScaleAsync(). Is it possible to avoid it? E.g. maybe call
GrabWindowSnapshotAndScaleAsync() here?

[enne (OOO), 2014-01-08 18:50:47]

https://codereview.chromium.org/119753007/diff/490001/ui/snapshot/snapshot_au...
File ui/snapshot/snapshot_aura.cc (right):

https://codereview.chromium.org/119753007/diff/490001/ui/snapshot/snapshot_au...
ui/snapshot/snapshot_aura.cc:202: scoped_ptr<cc::CopyOutputRequest> request =
On 2014/01/08 02:24:16, Sergey Ulanov wrote:
> It looks like a lot of code in this function is duplicated in
> GrabWindowSnapshotAndScaleAsync(). Is it possible to avoid it? E.g. maybe call
> GrabWindowSnapshotAndScaleAsync() here?

Done.  Can't call GrabWindowSnapshotAndScaleAsync because it pre-rotates the
target, but I could refactor setting up the output request into a common helper
function.

[commit-bot: I haz the power, 2014-01-08 20:45:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/119753007/900001

[commit-bot: I haz the power, 2014-01-08 23:55:01]

Change committed as 243700