[devtools] Support different encodings for Page.CaptureScreenshot.

ui/snapshot/snapshot_aura_unittest.cc

Index: ui/snapshot/snapshot_aura_unittest.cc
diff --git a/ui/snapshot/snapshot_aura_unittest.cc b/ui/snapshot/snapshot_aura_unittest.cc
index 7be0d703fa1c2006a3b69d30f9b137db124db89f..e9fc843c1f016b62c5e944cf9033ee5393fe6320 100644
--- a/ui/snapshot/snapshot_aura_unittest.cc
+++ b/ui/snapshot/snapshot_aura_unittest.cc
@@ -140,19 +140,17 @@ class SnapshotAuraTest : public testing::Test {
     aura::Window::ConvertRectToTarget(
         test_window(), root_window(), &source_rect);
 
-    scoped_refptr<base::TestSimpleTaskRunner> task_runner(
-        new base::TestSimpleTaskRunner());
     scoped_refptr<SnapshotHolder> holder(new SnapshotHolder);
     ui::GrabWindowSnapshotAsync(
         root_window(),
         source_rect,
-        task_runner,
         base::Bind(&SnapshotHolder::SnapshotCallback, holder));
 
     // Wait for copy response.
+    // TODO(eseckler): This doesn't actually wait until the copy response is

[Eric Seckler, 2017/01/09 14:52:05]

enne@/sadrul@: These tests are super flaky and don't actually pass even before
my change. It seems they may simply not be run on the bots?

The reason for their flakiness seems to be that the WaitForDraw() below doesn't
actually ensure that the copy response was received. It looks like the
cc::Display actually calls the draw callbacks before swapping buffers. Even if
that were the case, I'm not entirely sure that the copy response from the
GLRenderer would be sent immediately during SwapBuffers (it looks async in
GLRenderer::GetFramebufferPixelsAsync).

Do you know of a more convenient event we could wait for here, or is the better
option to wait until our own callback (SnapshotHolder::SnapshotCallback) has
been run and fail through timeout otherwise?

[sadrul, 2017/01/10 05:12:50]

Ugh, it does look like these tests are not actually run on any bots :-/
I cannot tell you of what the best fix would be, but the option you suggest
doesn't sound like a bad one. enne@ can tell you what the right fix should be.

[enne (OOO), 2017/01/10 19:26:04]

It sounds likely that this has been broken ever since there was a cc::Display. 
I think waiting on your own callback is going to be the most robust solution
into the future.

[Eric Seckler, 2017/01/11 15:58:44]

OK, thanks!

Turns out, there's more failing than just this (probably for a while as well) -
apparently the screenshot bounds are affected by test_screen()->SetUIScale(),
even though the tests don't expect them to be:

[ RUN      ] SnapshotAuraTest.UIScale
../../ui/snapshot/snapshot_aura_unittest.cc:234: Failure
Value of: snapshot.Size().ToString()
  Actual: "240x160"
Expected: gfx::ToRoundedSize(snapshot_size).ToString()
Which is: "300x200"

Can you tell whether this is a valid bug - or incorrect tests?

[sadrul, 2017/01/11 17:16:53]

I think the current expectations are correct. Sounds like a valid bug? oshima@
can confirm.

+    // issued, because cc::Display::DrawAndSwap() executes surface draw
+    // callbacks before drawing the frame on the renderer.
     WaitForDraw();
-    // Run internal snapshot callback to scale/rotate response image.
-    task_runner->RunUntilIdle();
     // Run SnapshotHolder callback.
     helper_->RunAllPendingInMessageLoop();
 
@@ -169,10 +167,9 @@ class SnapshotAuraTest : public testing::Test {
    public:
     SnapshotHolder() : completed_(false) {}
 
-    void SnapshotCallback(scoped_refptr<base::RefCountedBytes> png_data) {
+    void SnapshotCallback(const gfx::Image& image) {
       DCHECK(!completed_);
-      image_ = gfx::Image::CreateFrom1xPNGBytes(&(png_data->data()[0]),
-                                                png_data->size());
+      image_ = image;
       completed_ = true;
     }
     bool completed() const {