color: Fix color not being reset between layout tests

cc/trees/layer_tree_host_impl.cc

 // Copyright 2011 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 "cc/trees/layer_tree_host_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
@@ skipping 1334 matching lines @@
 void LayerTreeHostImpl::SetIsLikelyToRequireADraw(
     bool is_likely_to_require_a_draw) {
   // Proactively tell the scheduler that we expect to draw within each vsync
   // until we get all the tiles ready to draw. If we happen to miss a required
   // for draw tile here, then we will miss telling the scheduler each frame that
   // we intend to draw so it may make worse scheduling decisions.
   is_likely_to_require_a_draw_ = is_likely_to_require_a_draw;
 }
 
 gfx::ColorSpace LayerTreeHostImpl::GetRasterColorSpace() const {
+  gfx::ColorSpace result;
   if (!settings_.enable_color_correct_rasterization)
-    return gfx::ColorSpace();
-  if (!sync_tree())
-    return gfx::ColorSpace::CreateSRGB();
-  return sync_tree()->raster_color_space();
+    return result;
+  if (sync_tree())
+    result = sync_tree()->raster_color_space();

[enne (OOO), 2017/05/20 03:32:21]

I realize you're not changing this here, but this is a little weird to me that
this only asks the pending tree and not the active tree? For threaded
compositors, sync_tree only exists during commit.  Would that be why you're
getting invalid color spaces in these cases?

[ccameron, 2017/05/22 23:27:08]

Oh, LTHI::sync_tree() returns
  return pending_tree_ ? pending_tree_.get() : active_tree_.get();
That's not defined outside of commit?

In this particular case, we were getting an invalid color space because
sync_tree() existed, but had had its color space explicitly set to the
default-ctor color space (which is invalid).

[enne (OOO), 2017/05/23 00:13:43]

Ok, you're correct that as written, sync tree will return you a tree outside of
commit.  However, it really just means "the tree you're committing to" and
probably it should DCHECK that you're in the middle of commit when you call it.

The reason this is problematic is you can get a weird state here where both of
the trees exist and the pending tree has a different color space, and suddenly
the active tree starts using that color space to do raster in.

This is outside of this patch at this point but I think there should be at the
very least a comment here explaining what the process is when the color space is
different on both trees and why this code is ok.  It'd also be better to not
call sync tree and just include the logic to say "use the pending tree if it
exists, otherwise use the active tree, and this is ok because X". 
 
Ok, in that case this patch lgtm.

[ccameron, 2017/05/23 06:28:39]

Yeah, this doesn't have guarantees about when it will be called (it doesn't need
to be during commit).
Added a comment (prefer pending tree because that color space is more recently
sent from the browser).

+  if (!result.IsValid())
+    result = gfx::ColorSpace::CreateSRGB();
+  return result;
 }
 
 void LayerTreeHostImpl::RequestImplSideInvalidation() {
   client_->NeedsImplSideInvalidation();
 }
 
 void LayerTreeHostImpl::NotifyReadyToActivate() {
   client_->NotifyReadyToActivate();
 }
 
@@ skipping 2978 matching lines @@
 
 void LayerTreeHostImpl::ShowScrollbarsForImplScroll(ElementId element_id) {
   if (!element_id)
     return;
   if (ScrollbarAnimationController* animation_controller =
           ScrollbarAnimationControllerForElementId(element_id))
     animation_controller->DidScrollUpdate();
 }
 
 }  // namespace cc