Make ChromeOS screenshots use async readback path

Make ChromeOS screenshots use async readback path

The synchronous readback path adds complexity through ui and cc
compositors and will ideally be removed.  The screenshots already use a
post task mechanism to save to disk, so making them use the asynchronous
readback path instead is straightforward.

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

[enne (OOO), 2014-01-07 20:20:20]

Depends on https://codereview.chromium.org/119753007/

[enne (OOO), 2014-01-09 00:59:27]

mukai: window_snapshot changes (if you're not a good reviewer for this, please
let me know)
sergeyu: FYI for async snapshot API changes if you want to take a look
beng: OWNERS for chrome/browser/ui and ui/

[Jun Mukai, 2014-01-09 01:19:31]

lgtm, with a nit comment.

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/ash/sc...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/ash/sc...
chrome/browser/ui/ash/screenshot_taker.cc:558:
ScreenshotTakerObserver::SCREENSHOT_GRABWINDOW_PARTIAL_FAILED,
Right now we don't distinguish PARTIAL_FAILED and FULL_FAILED, and I don't think
we need to keep this.
I'm okay just to remove them and declare GRABWINDOW_FAILED.

Then you can remove |is_partial| and |window_idx| (but probably better to put
screenshot_path to the LOG(ERROR) instead of window_idx).

You don't need to do this in this CL. I can do that later.

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
chrome/browser/ui/window_snapshot/window_snapshot.cc:25: void
GrabWindowSnapshotForUserAsync(
I am not sure we need to declare this if the only user is ash/screenshot_taker.
Screenshot_taker already checks kDisableScreenshots, so you don't have to check
that again.

[enne (OOO), 2014-01-09 02:47:50]

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/ash/sc...
File chrome/browser/ui/ash/screenshot_taker.cc (right):

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/ash/sc...
chrome/browser/ui/ash/screenshot_taker.cc:558:
ScreenshotTakerObserver::SCREENSHOT_GRABWINDOW_PARTIAL_FAILED,
On 2014/01/09 01:19:32, Jun Mukai wrote:
> You don't need to do this in this CL. I can do that later.

Okay, thanks.  I was just trying to preserve behavior here.

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
chrome/browser/ui/window_snapshot/window_snapshot.cc:25: void
GrabWindowSnapshotForUserAsync(
On 2014/01/09 01:19:32, Jun Mukai wrote:
> I am not sure we need to declare this if the only user is
ash/screenshot_taker.
> Screenshot_taker already checks kDisableScreenshots, so you don't have to
check
> that again.

That's a good point.  I'll remove this now and then come back and remove the
other sync version too once there's only caller in the future.

[enne (OOO), 2014-01-09 19:38:42]

ben: ui/ and chrome/browser/ui/ OWNERS

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
File chrome/browser/ui/window_snapshot/window_snapshot.cc (right):

https://codereview.chromium.org/126373002/diff/60001/chrome/browser/ui/window...
chrome/browser/ui/window_snapshot/window_snapshot.cc:25: void
GrabWindowSnapshotForUserAsync(
On 2014/01/09 02:47:51, enne wrote:
> On 2014/01/09 01:19:32, Jun Mukai wrote:
> > I am not sure we need to declare this if the only user is
> ash/screenshot_taker.
> > Screenshot_taker already checks kDisableScreenshots, so you don't have to
> check
> > that again.
> 
> That's a good point.  I'll remove this now and then come back and remove the
> other sync version too once there's only caller in the future.

Here's the followup to clean up window_snapshot once there's only one caller of
the sync version: https://codereview.chromium.org/132203004/

[Jun Mukai, 2014-01-09 19:41:24]

https://codereview.chromium.org/126373002/diff/130001/chrome/browser/ui/windo...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

https://codereview.chromium.org/126373002/diff/130001/chrome/browser/ui/windo...
chrome/browser/ui/window_snapshot/window_snapshot.h:11: #include
"ui/snapshot/snapshot.h"
nit: you don't have to include this now

[enne (OOO), 2014-01-09 19:50:02]

https://codereview.chromium.org/126373002/diff/130001/chrome/browser/ui/windo...
File chrome/browser/ui/window_snapshot/window_snapshot.h (right):

https://codereview.chromium.org/126373002/diff/130001/chrome/browser/ui/windo...
chrome/browser/ui/window_snapshot/window_snapshot.h:11: #include
"ui/snapshot/snapshot.h"
On 2014/01/09 19:41:24, Jun Mukai wrote:
> nit: you don't have to include this now

Done.

[Ben Goodger (Google), 2014-01-10 16:40:08]

owner lgtm

[commit-bot: I haz the power, 2014-01-10 17:28:22]

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

[commit-bot: I haz the power, 2014-01-10 18:06:18]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps,
check_deps2git, chromedriver_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests,
jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-10 18:08:42]

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

[commit-bot: I haz the power, 2014-01-10 18:41:06]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2014-01-10 18:43:44]

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

[commit-bot: I haz the power, 2014-01-10 19:48:47]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-10 20:54:01]

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

[commit-bot: I haz the power, 2014-01-10 22:44:09]

Retried try job too often on linux_chromeos for step(s) browser_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[enne (OOO), 2014-01-10 23:11:48]

mukai: FYI, the TakeSnapshot unit test failed because it was checking the last
screenshot timestamp.  I could have moved the Wait() in the test before the
check (which fixes it), but it seemed more sane to set the screenshot timestamp
before taking the screenshot (even if it ends up failing).

[Jun Mukai, 2014-01-10 23:21:48]

On 2014/01/10 23:11:48, enne wrote:
> mukai: FYI, the TakeSnapshot unit test failed because it was checking the last
> screenshot timestamp.  I could have moved the Wait() in the test before the
> check (which fixes it), but it seemed more sane to set the screenshot
timestamp
> before taking the screenshot (even if it ends up failing).

+1. the timestamp is used for throttling, no problem to move before taking the
screenshot actually.

[commit-bot: I haz the power, 2014-01-11 00:21:10]

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

[commit-bot: I haz the power, 2014-01-11 04:32:34]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[enne (OOO), 2014-01-13 23:50:50]

mukai: Ack, sorry, but I also missed this failure in the policy browser test. 
This test issues a screenshot key to the accelerator, waits for the message loop
to not be idle, and then checks the disk to see how many screenshots were
written.  This doesn't work at all for the new asynchronous case, because the
message loop may be idle for a long time before the asynchronous screenshot
callback gets called and posts a reply to it.

I can't just sleep, because that would be flaky.  I also can't just wait for n
tasks to go by in the message loop with a TaskObserver, because there are
animations and timers and all sorts of other things happening so it's not just
the next task.

I think there needs to be an explicit callback if this test is going to be
non-flaky, although it's unfortunate to have to add such a callback just for
test verification.  It seems like the best path is to add a callback to
ScreenshotDelegate::HandleTakeScreenshotForAllRootWindows (or a second function)
and then have the test call that directly instead of going through the
AcceleratorController.

[Jun Mukai, 2014-01-14 00:56:50]

On 2014/01/13 23:50:50, enne wrote:
> mukai: Ack, sorry, but I also missed this failure in the policy browser test. 
> This test issues a screenshot key to the accelerator, waits for the message
loop
> to not be idle, and then checks the disk to see how many screenshots were
> written.  This doesn't work at all for the new asynchronous case, because the
> message loop may be idle for a long time before the asynchronous screenshot
> callback gets called and posts a reply to it.
> 
> I can't just sleep, because that would be flaky.  I also can't just wait for n
> tasks to go by in the message loop with a TaskObserver, because there are
> animations and timers and all sorts of other things happening so it's not just
> the next task.
> 
> I think there needs to be an explicit callback if this test is going to be
> non-flaky, although it's unfortunate to have to add such a callback just for
> test verification.  It seems like the best path is to add a callback to
> ScreenshotDelegate::HandleTakeScreenshotForAllRootWindows (or a second
function)
> and then have the test call that directly instead of going through the
> AcceleratorController.

I don't even know that policy browser test thing... someone else wrote that.
I'm just wondering that, it can inherit ScreenshotTakerObserver, then runs
its own runloop and quits it upon OnScreenshotCompleted, as
screenshot_taker_unittest.cc does.
I think it would be more robust.

[enne (OOO), 2014-01-14 00:58:53]

> I don't even know that policy browser test thing... someone else wrote that.
> I'm just wondering that, it can inherit ScreenshotTakerObserver, then runs
> its own runloop and quits it upon OnScreenshotCompleted, as
> screenshot_taker_unittest.cc does.
> I think it would be more robust.

Oh, ScreenshotTakerObserver seems much better, thanks! I'll see if I can modify
the test to use that.

[enne (OOO), 2014-01-14 00:59:12]

> I don't even know that policy browser test thing... someone else wrote that.
> I'm just wondering that, it can inherit ScreenshotTakerObserver, then runs
> its own runloop and quits it upon OnScreenshotCompleted, as
> screenshot_taker_unittest.cc does.
> I think it would be more robust.

Oh, ScreenshotTakerObserver seems much better, thanks! I'll see if I can modify
the test to use that.

[enne (OOO), 2014-01-14 20:38:55]

> Oh, ScreenshotTakerObserver seems much better, thanks! I'll see if I can
modify
> the test to use that.

Done.  Thanks for the suggestion!

[commit-bot: I haz the power, 2014-01-14 20:40:02]

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

[commit-bot: I haz the power, 2014-01-14 23:01:36]

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

[commit-bot: I haz the power, 2014-01-15 01:34:59]

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

[commit-bot: I haz the power, 2014-01-15 09:55:03]

Change committed as 244859