Remove unittests based on wrong assumption of compositor

ui/compositor/layer_unittest.cc

 // Copyright (c) 2012 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 "base/basictypes.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
@@ skipping 506 matching lines @@
   TestLayerDelegate delegate;
   l1->set_delegate(&delegate);
   delegate.AddColor(SK_ColorWHITE);
   delegate.AddColor(SK_ColorYELLOW);
   delegate.AddColor(SK_ColorGREEN);
 
   l1->SchedulePaint(gfx::Rect(0, 0, 400, 400));
   WaitForDraw();
 
   EXPECT_EQ(delegate.color_index(), 1);
-  EXPECT_EQ(delegate.paint_size(), l1->bounds().size());
 
   l1->SchedulePaint(gfx::Rect(10, 10, 200, 200));
   WaitForDraw();
   EXPECT_EQ(delegate.color_index(), 2);
-  EXPECT_EQ(delegate.paint_size(), gfx::Size(200, 200));

[danakj, 2015/03/02 23:03:09]

can you make the delegate bigger so that you can verify the rect isn't the full
layer? i assume that is the point of the test that we're passing along what we
paint not just the layer bounds always?

[weiliangc, 2015/03/03 16:35:44]

Done.

 
   l1->SchedulePaint(gfx::Rect(5, 5, 50, 50));
   WaitForDraw();
   EXPECT_EQ(delegate.color_index(), 0);
-  EXPECT_EQ(delegate.paint_size(), gfx::Size(50, 50));
 }
 
 TEST_F(LayerWithRealCompositorTest, DrawTree) {
   scoped_ptr<Layer> l1(CreateColorLayer(SK_ColorRED,
                                         gfx::Rect(20, 20, 400, 400)));
   scoped_ptr<Layer> l2(CreateColorLayer(SK_ColorBLUE,
                                         gfx::Rect(10, 10, 350, 350)));
   scoped_ptr<Layer> l3(CreateColorLayer(SK_ColorYELLOW,
                                         gfx::Rect(10, 10, 100, 100)));
   l1->Add(l2.get());
@@ skipping 753 matching lines @@
   EXPECT_EQ("10,20 200x220", root->bounds().ToString());
   EXPECT_EQ("10,20 140x180", l1->bounds().ToString());
   gfx::Size cc_bounds_size = root->cc_layer()->bounds();
   EXPECT_EQ("200x220", cc_bounds_size.ToString());
   cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
   // No scale change, so no scale notification.
   EXPECT_EQ(0.0f, root_delegate.device_scale_factor());
   EXPECT_EQ(0.0f, l1_delegate.device_scale_factor());
 
-  EXPECT_EQ("200x220", root_delegate.paint_size().ToString());
-  EXPECT_EQ("140x180", l1_delegate.paint_size().ToString());
-
   // Scale up to 2.0. Changing scale doesn't change the bounds in DIP.
   GetCompositor()->SetScaleAndSize(2.0f, gfx::Size(500, 500));
   EXPECT_EQ("10,20 200x220", root->bounds().ToString());
   EXPECT_EQ("10,20 140x180", l1->bounds().ToString());
   // CC layer should still match the UI layer bounds.
   cc_bounds_size = root->cc_layer()->bounds();
   EXPECT_EQ("200x220", cc_bounds_size.ToString());
   cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
   // New scale factor must have been notified.
   EXPECT_EQ(2.0f, root_delegate.device_scale_factor());
   EXPECT_EQ(2.0f, l1_delegate.device_scale_factor());
 
   // Canvas size must have been scaled down up.

[danakj, 2015/03/03 17:57:05]

You can remove this WaitForDraw() block entirely if it's not doing anything (I
don't think it is?)

   WaitForDraw();
-  EXPECT_EQ("400x440", root_delegate.paint_size().ToString());
-  EXPECT_EQ("2.0 2.0", root_delegate.ToScaleString());

[danakj, 2015/03/02 23:03:09]

Does the DSF no longer go to the delegate? This test is no longer testing
anything either. I have similar comment to the other test.

[weiliangc, 2015/03/03 16:35:44]

The ScaleString is from the canvas passed to LayerDelegate. I don't think this
directly reflects DSF.

-  EXPECT_EQ("280x360", l1_delegate.paint_size().ToString());
-  EXPECT_EQ("2.0 2.0", l1_delegate.ToScaleString());
 
   // Scale down back to 1.0f.
   GetCompositor()->SetScaleAndSize(1.0f, gfx::Size(500, 500));
   EXPECT_EQ("10,20 200x220", root->bounds().ToString());
   EXPECT_EQ("10,20 140x180", l1->bounds().ToString());
   // CC layer should still match the UI layer bounds.
   cc_bounds_size = root->cc_layer()->bounds();
   EXPECT_EQ("200x220", cc_bounds_size.ToString());
   cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
   // New scale factor must have been notified.
   EXPECT_EQ(1.0f, root_delegate.device_scale_factor());
   EXPECT_EQ(1.0f, l1_delegate.device_scale_factor());
 
   // Canvas size must have been scaled down too.
   WaitForDraw();

[danakj, 2015/03/03 17:57:05]

same here

-  EXPECT_EQ("200x220", root_delegate.paint_size().ToString());
   EXPECT_EQ("1.0 1.0", root_delegate.ToScaleString());
-  EXPECT_EQ("140x180", l1_delegate.paint_size().ToString());
   EXPECT_EQ("1.0 1.0", l1_delegate.ToScaleString());
 
   root_delegate.reset();
   l1_delegate.reset();
   // Just changing the size shouldn't notify the scale change nor
   // trigger repaint.
   GetCompositor()->SetScaleAndSize(1.0f, gfx::Size(1000, 1000));
   // No scale change, so no scale notification.
   EXPECT_EQ(0.0f, root_delegate.device_scale_factor());
   EXPECT_EQ(0.0f, l1_delegate.device_scale_factor());
   WaitForDraw();
   EXPECT_EQ("0x0", root_delegate.paint_size().ToString());

[danakj, 2015/03/03 17:57:05]

can you test "paint_count()" or something instead, and remove paint_size() and
ToScaleString() entirely?

[weiliangc, 2015/03/04 01:23:57]

paint_size() can still be used for the later tests's "contains layer or not" I
think.

   EXPECT_EQ("0.0 0.0", root_delegate.ToScaleString());
   EXPECT_EQ("0x0", l1_delegate.paint_size().ToString());
   EXPECT_EQ("0.0 0.0", l1_delegate.ToScaleString());
 }
 
 TEST_F(LayerWithRealCompositorTest, ScaleReparent) {
   scoped_ptr<Layer> root(CreateColorLayer(SK_ColorWHITE,
                                           gfx::Rect(10, 20, 200, 220)));
   scoped_ptr<Layer> l1(CreateColorLayer(SK_ColorWHITE,
                                         gfx::Rect(10, 20, 140, 180)));
   TestLayerDelegate l1_delegate;
   l1_delegate.AddColor(SK_ColorWHITE);
   l1->set_delegate(&l1_delegate);
 
   GetCompositor()->SetScaleAndSize(1.0f, gfx::Size(500, 500));
   GetCompositor()->SetRootLayer(root.get());
   WaitForDraw();
 
   root->Add(l1.get());
   EXPECT_EQ("10,20 140x180", l1->bounds().ToString());
   gfx::Size cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
   EXPECT_EQ(0.0f, l1_delegate.device_scale_factor());
 
   WaitForDraw();
-  EXPECT_EQ("140x180", l1_delegate.paint_size().ToString());

[danakj, 2015/03/02 23:03:09]

This test isn't testing anything now. We should look at blame and see what bug
this was testing and how we can change the test to guard regressions of that
bug.

[weiliangc, 2015/03/03 16:35:44]

The test was added when moving DIP translation to ui/compositor
(https://codereview.chromium.org/10221028)

Maybe this can still use to test DSF is passed along?

[danakj, 2015/03/03 17:57:05]

Thanks for digging. I think this test made sense to make sure painting happens
at the right DSF (ie it uses images of the right scale factory in the
recording). But now we just want to make sure the delegate's
device_scale_factor() is right, so that it paints with images at the right
scale.

I Don't think the WaitForDraws() do anything for this test either, can you
remove them, and comment near the EXPECTs on the device scale factor to show
that these are what we care about and why?

-  EXPECT_EQ("1.0 1.0", l1_delegate.ToScaleString());
 
   // Remove l1 from root and change the scale.
   root->Remove(l1.get());
   EXPECT_EQ(NULL, l1->parent());
   EXPECT_EQ(NULL, l1->GetCompositor());
   GetCompositor()->SetScaleAndSize(2.0f, gfx::Size(500, 500));
   // Sanity check on root and l1.
   EXPECT_EQ("10,20 200x220", root->bounds().ToString());
   cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
 
   root->Add(l1.get());
   EXPECT_EQ("10,20 140x180", l1->bounds().ToString());
   cc_bounds_size = l1->cc_layer()->bounds();
   EXPECT_EQ("140x180", cc_bounds_size.ToString());
   EXPECT_EQ(2.0f, l1_delegate.device_scale_factor());
   WaitForDraw();
-  EXPECT_EQ("280x360", l1_delegate.paint_size().ToString());
-  EXPECT_EQ("2.0 2.0", l1_delegate.ToScaleString());
 }
 
 // Verifies that when changing bounds on a layer that is invisible, and then
 // made visible, the right thing happens:
 // - if just a move, then no painting should happen.
 // - if a resize, the layer should be repainted.
 TEST_F(LayerWithDelegateTest, SetBoundsWhenInvisible) {
   scoped_ptr<Layer> root(CreateNoTextureLayer(gfx::Rect(0, 0, 1000, 1000)));
 
   scoped_ptr<Layer> child(CreateLayer(LAYER_TEXTURED));
@@ skipping 346 matching lines @@
                                      MakeFrameData(gfx::Size(10, 10))));
   layer->SetShowDelegatedContent(frame_provider.get(), gfx::Size(10, 10));
 
   EXPECT_FALSE(delegate.delegated_frame_damage_called());
   layer->OnDelegatedFrameDamage(damage_rect);
   EXPECT_TRUE(delegate.delegated_frame_damage_called());
   EXPECT_EQ(damage_rect, delegate.delegated_frame_damage_rect());
 }
 
 }  // namespace ui