Fix render state restore bug in DirectRenderer

cc/trees/layer_tree_host_pixeltest_readback.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "build/build_config.h"
 #include "cc/layers/solid_color_layer.h"
 #include "cc/layers/texture_layer.h"
 #include "cc/output/copy_output_request.h"
 #include "cc/output/copy_output_result.h"
 #include "cc/test/fake_picture_layer.h"
@@ skipping 1126 matching lines @@
   green->AddChild(blue);
 
   RunReadbackTestWithReadbackTarget(
       PIXEL_TEST_GL,
       READBACK_DEFAULT,
       background,
       green.get(),
       base::FilePath(FILE_PATH_LITERAL("green_with_blue_corner.png")));
 }
 
 class LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest

[danakj, 2015/02/09 18:01:17]

Maybe we can rename this a bit more generally since we're using it for multiple
tests now. Like LayerTreeHostReadbackGLAndSoftwareTest?

(Offhand, it might be nice to apply this pattern to other tests in this file too
when they currently duplicate the test code)

[ericrk, 2015/02/09 19:46:14]

Updated the tests to use parameters. Now all of them run three configs -
Software, GL+Bitmap, GL+Default. Software+Default and Software+Bitmap appear to
be the same, so I always do Software+Default.

     : public LayerTreeHostReadbackPixelTest,
       public testing::WithParamInterface<bool> {};
 
 TEST_P(LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest,
        ReadbackNonRootOrFirstLayer) {
   // This test has 3 render passes with the copy request on the render pass in
-  // the middle. Doing a copy request can be destructive of state, so for render
-  // passes after the first drawn the code path is different. This verifies the
-  // non-first and non-root path. See http://crbug.com/99393 for more info.
+  // the middle. This test caught an issue where copy requests on non-root
+  // non-first render passes were being treated differently from the first
+  // render pass.
   scoped_refptr<SolidColorLayer> background =
       CreateSolidColorLayer(gfx::Rect(200, 200), SK_ColorGREEN);
 
   scoped_refptr<SolidColorLayer> blue =
       CreateSolidColorLayer(gfx::Rect(150, 150, 50, 50), SK_ColorBLUE);
   blue->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
       base::Bind(&IgnoreReadbackResult)));
   background->AddChild(blue);
 
   RunReadbackTestWithReadbackTarget(
       GetParam() ? PIXEL_TEST_GL : PIXEL_TEST_SOFTWARE,
       READBACK_DEFAULT,
       background,
       background.get(),
       base::FilePath(FILE_PATH_LITERAL("green_with_blue_corner.png")));
 }
 
+TEST_P(LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest,
+       MultipleReadbackNonRootOrFirstLayer) {

[danakj, 2015/02/09 18:01:17]

The test has multiple requests on blue which becomes the first renderpass drawn
in the renderer, so this isnt quite right for the name.
MultipleReadbacksOnLayer?

[ericrk, 2015/02/09 19:46:14]

Done.

+  // This test has 3 render passes with multiple copy requests on the second
+  // render pass. For every copy request after the first, state must be
+  // restored via a call to UseRenderPass (see http://crbug.com/99393). This
+  // test ensures that the renderer correctly handles cases where
+  // UseRenderPass is called multiple times for a single layer.
+  scoped_refptr<SolidColorLayer> background =
+      CreateSolidColorLayer(gfx::Rect(200, 200), SK_ColorGREEN);
+
+  scoped_refptr<SolidColorLayer> blue =
+      CreateSolidColorLayer(gfx::Rect(150, 150, 50, 50), SK_ColorBLUE);
+  blue->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(
+      base::Bind(&IgnoreReadbackResult)));
+  blue->RequestCopyOfOutput(CopyOutputRequest::CreateBitmapRequest(

[danakj, 2015/02/09 18:01:17]

I think we can simplify this test a bit, by just putting one extra request like
this on background and removing blue? That should still test that codepath
right?

[ericrk, 2015/02/09 19:46:14]

Done.

+      base::Bind(&IgnoreReadbackResult)));
+  background->AddChild(blue);
+
+  RunReadbackTestWithReadbackTarget(
+      GetParam() ? PIXEL_TEST_GL : PIXEL_TEST_SOFTWARE, READBACK_DEFAULT,

[danakj, 2015/02/09 18:01:17]

Hm, it occured to me if only one version of this test failed, i'd have no idea
if /0 and /1 (the test suffixes) meant GL or Software.. Do you think we can
change the param type from bool to this enum type directly? Then we can see the
order clearly in the definition of the TEST_P?

[ericrk, 2015/02/09 19:46:14]

Good call - done.

+      background, background.get(),
+      base::FilePath(FILE_PATH_LITERAL("green_with_blue_corner.png")));
+}
+
 INSTANTIATE_TEST_CASE_P(
     LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTests,
     LayerTreeHostReadbackNonFirstNonRootRenderPassPixelTest,
     testing::Bool());
 
 }  // namespace
 }  // namespace cc
 
 #endif  // OS_ANDROID