Moving RenderSurface creation outside of CalcDrawProps

Move RenderSurface creation outside of CalcDrawProps

BUG=386788
Committed: https://crrev.com/e83f28ca816694be553baed069f784eb286d8170
Cr-Commit-Position: refs/heads/master@{#309428}

[awoloszyn, 2014-08-14 17:37:27]

https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
This is used to trigger RenderSurface creation on impl-side, because many of our
unit-tests create only the impl-side version of the tree. I tried with templates
to hide some of this, and only made it much worse. The only other thing I can
think of is creating a virtual that we call into before CDP, with a null default
behavior, and the render-surface creation for testing. (If preferred I can
change to that).

[danakj, 2014-08-14 17:44:21]

https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
On 2014/08/14 17:37:27, awoloszyn wrote:
> This is used to trigger RenderSurface creation on impl-side, because many of
our
> unit-tests create only the impl-side version of the tree. I tried with
templates
> to hide some of this, and only made it much worse. The only other thing I can
> think of is creating a virtual that we call into before CDP, with a null
default
> behavior, and the render-surface creation for testing. (If preferred I can
> change to that).

Oh gosh. Maybe we need to be refactoring our unit tests before we attempt this,
because leaking unit test problems into production code sounds like bad times.

Maybe (some of?) those tests that directly make impl things should be making
main thread things instead.

This is a huge conceptual change in the compositor and a lot of our tests have
assumptions built in which are breaking now, or will break in the future as we
rewrite the world. We need to consider how we can make those tests reflect the
real world not some past one with bandaids.

How can we fix our tests to not need this?

[awoloszyn, 2014-08-14 18:12:53]

On 2014/08/14 17:44:21, danakj wrote:
>
https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.h (right):
> 
>
https://codereview.chromium.org/465853004/diff/20001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
> On 2014/08/14 17:37:27, awoloszyn wrote:
> > This is used to trigger RenderSurface creation on impl-side, because many of
> our
> > unit-tests create only the impl-side version of the tree. I tried with
> templates
> > to hide some of this, and only made it much worse. The only other thing I
can
> > think of is creating a virtual that we call into before CDP, with a null
> default
> > behavior, and the render-surface creation for testing. (If preferred I can
> > change to that).
> 
> Oh gosh. Maybe we need to be refactoring our unit tests before we attempt
this,
> because leaking unit test problems into production code sounds like bad times.
> 
> Maybe (some of?) those tests that directly make impl things should be making
> main thread things instead.
> 
> This is a huge conceptual change in the compositor and a lot of our tests have
> assumptions built in which are breaking now, or will break in the future as we
> rewrite the world. We need to consider how we can make those tests reflect the
> real world not some past one with bandaids.
> 
> How can we fix our tests to not need this?

I agree with you w.r.t. leaking unit test problems into production code. 

From what I remember there are roughly 900 unit tests that fail because of this,
I will start looking at how to make the changes required to get them passing in
the new world.

[danakj, 2014-08-14 18:19:52]

Awesome thanks. If we can do some refactors as separate CLs that'd be
super, once you sort out things.


On Thu, Aug 14, 2014 at 2:12 PM, <awoloszyn@chromium.org> wrote:

> On 2014/08/14 17:44:21, danakj wrote:
>
> https://codereview.chromium.org/465853004/diff/20001/cc/
> trees/layer_tree_host_impl.h
>
>> File cc/trees/layer_tree_host_impl.h (right):
>>
>
>
> https://codereview.chromium.org/465853004/diff/20001/cc/
> trees/layer_tree_host_impl.h#newcode188
>
>> cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
>> On 2014/08/14 17:37:27, awoloszyn wrote:
>> > This is used to trigger RenderSurface creation on impl-side, because
>> many of
>> our
>> > unit-tests create only the impl-side version of the tree. I tried with
>> templates
>> > to hide some of this, and only made it much worse. The only other thing
>> I
>>
> can
>
>> > think of is creating a virtual that we call into before CDP, with a null
>> default
>> > behavior, and the render-surface creation for testing. (If preferred I
>> can
>> > change to that).
>>
>
>  Oh gosh. Maybe we need to be refactoring our unit tests before we attempt
>>
> this,
>
>> because leaking unit test problems into production code sounds like bad
>> times.
>>
>
>  Maybe (some of?) those tests that directly make impl things should be
>> making
>> main thread things instead.
>>
>
>  This is a huge conceptual change in the compositor and a lot of our tests
>> have
>> assumptions built in which are breaking now, or will break in the future
>> as we
>> rewrite the world. We need to consider how we can make those tests
>> reflect the
>> real world not some past one with bandaids.
>>
>
>  How can we fix our tests to not need this?
>>
>
> I agree with you w.r.t. leaking unit test problems into production code.
>
> From what I remember there are roughly 900 unit tests that fail because of
> this,
> I will start looking at how to make the changes required to get them
> passing in
> the new world.
>
> https://codereview.chromium.org/465853004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awoloszyn, 2014-08-19 14:36:02]

On 2014/08/14 18:19:52, danakj wrote:
> Awesome thanks. If we can do some refactors as separate CLs that'd be
> super, once you sort out things.
> 
> 
> On Thu, Aug 14, 2014 at 2:12 PM, <mailto:awoloszyn@chromium.org> wrote:
> 
> > On 2014/08/14 17:44:21, danakj wrote:
> >
> > https://codereview.chromium.org/465853004/diff/20001/cc/
> > trees/layer_tree_host_impl.h
> >
> >> File cc/trees/layer_tree_host_impl.h (right):
> >>
> >
> >
> > https://codereview.chromium.org/465853004/diff/20001/cc/
> > trees/layer_tree_host_impl.h#newcode188
> >
> >> cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
> >> On 2014/08/14 17:37:27, awoloszyn wrote:
> >> > This is used to trigger RenderSurface creation on impl-side, because
> >> many of
> >> our
> >> > unit-tests create only the impl-side version of the tree. I tried with
> >> templates
> >> > to hide some of this, and only made it much worse. The only other thing
> >> I
> >>
> > can
> >
> >> > think of is creating a virtual that we call into before CDP, with a null
> >> default
> >> > behavior, and the render-surface creation for testing. (If preferred I
> >> can
> >> > change to that).
> >>
> >
> >  Oh gosh. Maybe we need to be refactoring our unit tests before we attempt
> >>
> > this,
> >
> >> because leaking unit test problems into production code sounds like bad
> >> times.
> >>
> >
> >  Maybe (some of?) those tests that directly make impl things should be
> >> making
> >> main thread things instead.
> >>
> >
> >  This is a huge conceptual change in the compositor and a lot of our tests
> >> have
> >> assumptions built in which are breaking now, or will break in the future
> >> as we
> >> rewrite the world. We need to consider how we can make those tests
> >> reflect the
> >> real world not some past one with bandaids.
> >>
> >
> >  How can we fix our tests to not need this?
> >>
> >
> > I agree with you w.r.t. leaking unit test problems into production code.
> >
> > From what I remember there are roughly 900 unit tests that fail because of
> > this,
> > I will start looking at how to make the changes required to get them
> > passing in
> > the new world.
> >
> > https://codereview.chromium.org/465853004/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I ended up re-working how we integrated with the unit tests so that it avoids
leaking testing info into our code. The only other way I can think of still
being able to do a lot of these tests would be to always create the main-thread 
trees and always do CDP + Sync the trees during every test needs to run CDP on 
impl-thread.

[danakj, 2014-08-19 14:58:54]

On Tue, Aug 19, 2014 at 10:36 AM, <awoloszyn@chromium.org> wrote:

> On 2014/08/14 18:19:52, danakj wrote:
>
>> Awesome thanks. If we can do some refactors as separate CLs that'd be
>> super, once you sort out things.
>>
>
>
>  On Thu, Aug 14, 2014 at 2:12 PM, <mailto:awoloszyn@chromium.org> wrote:
>>
>
>  > On 2014/08/14 17:44:21, danakj wrote:
>> >
>> > https://codereview.chromium.org/465853004/diff/20001/cc/
>> > trees/layer_tree_host_impl.h
>> >
>> >> File cc/trees/layer_tree_host_impl.h (right):
>> >>
>> >
>> >
>> > https://codereview.chromium.org/465853004/diff/20001/cc/
>> > trees/layer_tree_host_impl.h#newcode188
>> >
>> >> cc/trees/layer_tree_host_impl.h:188: virtual bool IsTesting();
>> >> On 2014/08/14 17:37:27, awoloszyn wrote:
>> >> > This is used to trigger RenderSurface creation on impl-side, because
>> >> many of
>> >> our
>> >> > unit-tests create only the impl-side version of the tree. I tried
>> with
>> >> templates
>> >> > to hide some of this, and only made it much worse. The only other
>> thing
>> >> I
>> >>
>> > can
>> >
>> >> > think of is creating a virtual that we call into before CDP, with a
>> null
>> >> default
>> >> > behavior, and the render-surface creation for testing. (If preferred
>> I
>> >> can
>> >> > change to that).
>> >>
>> >
>> >  Oh gosh. Maybe we need to be refactoring our unit tests before we
>> attempt
>> >>
>> > this,
>> >
>> >> because leaking unit test problems into production code sounds like bad
>> >> times.
>> >>
>> >
>> >  Maybe (some of?) those tests that directly make impl things should be
>> >> making
>> >> main thread things instead.
>> >>
>> >
>> >  This is a huge conceptual change in the compositor and a lot of our
>> tests
>> >> have
>> >> assumptions built in which are breaking now, or will break in the
>> future
>> >> as we
>> >> rewrite the world. We need to consider how we can make those tests
>> >> reflect the
>> >> real world not some past one with bandaids.
>> >>
>> >
>> >  How can we fix our tests to not need this?
>> >>
>> >
>> > I agree with you w.r.t. leaking unit test problems into production code.
>> >
>> > From what I remember there are roughly 900 unit tests that fail because
>> of
>> > this,
>> > I will start looking at how to make the changes required to get them
>> > passing in
>> > the new world.
>> >
>> > https://codereview.chromium.org/465853004/
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> I ended up re-working how we integrated with the unit tests so that it
> avoids
> leaking testing info into our code. The only other way I can think of still
> being able to do a lot of these tests would be to always create the
> main-thread
> trees and always do CDP + Sync the trees during every test needs to run
> CDP on
> impl-thread.
>

FWIW If the computation only exists on the main thread, we'll eventually
have to move all the tests to the main thread, or have it generate the impl
structures that we need for the tests.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[awoloszyn, 2014-08-25 21:03:44]

I have updated the unit-tests to better reflect the current world. I have a
follow-up CL that specifically tests RenderSurface creation, so we can remove
the impl-side checks for render-surfaces, especially since they now exist or not
simply based on whether or not we call CreateRenderSurface on them.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_impl_unittest.cc:298: 
These tests were moved to main-thread where we now create RenderSurfaces.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/damage_tracker_...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/trees/damage_tracker_...
cc/trees/damage_tracker_unittest.cc:1162: 
This test will be subsumed by the main-thread RenderSurface tests in the next
CL.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_impl...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl_unittest.cc:686: 
This will move to the RenderSurface creation tests in the next CL.

[danakj, 2014-08-26 18:06:13]

Some high level questions

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_unittest.cc:82: CalcDrawProps(host_.get());
if there's no expects after this, why do it?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:913:
layer->SetShouldHaveRenderSurface(ShouldHaveRenderSurface());
could this just create or destroy the render surface directly?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1085: draw_properties_.render_target = this;
hm.... can this move to CalcDrawProps?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1251: SetNeedsPushProperties();
ITYM SetNeedsCommit() or else this will just stay on the main thread until
something else makes a commit?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h#newcod...
cc/layers/layer.h:257: RenderSurface* render_surface() const { return
render_surface_.get(); }
nit: can you move this out of the middle of the draw_props accessors?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h#newcod...
cc/layers/layer.h:379: void SetRenderSurfaceActive(bool active);
Should we get rid of {Create,Clear}RenderSurface then? I don't like having 2
ways to do the same thing. Or not add this one?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:244: draw_properties_.render_target = this;
Can this move to CalcDrawProps?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1551: active_render_surface_ = render_surface_.get();
Can you explain what is going on here, why do you have this
active_render_surface_ pointer? Either the LayerImpl has one or it doesn't?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:300: void ClearRenderSurface();
Same Q re {Create,Clear} vs SetActive

https://codereview.chromium.org/465853004/diff/60001/cc/test/layer_tree_host_...
File cc/test/layer_tree_host_common_test.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/test/layer_tree_host_...
cc/test/layer_tree_host_common_test.h:68: bool create_render_surface = false);
no default parameters please. this violates the style guide

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1111: static inline typename
LayerType::RenderSurfaceType* CreateOrReuseRenderSurface(
unused?

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1822: layer->ShouldHaveRenderSurface() &&
IsRootLayer(layer);
the root has to have a surface, and nothing else is allowed in this case. can
you instead do

bool render_to_separate_surface = layer->ShouldHaveRenderSurface();

// Root must own a surface.
DCHECK(!IsRootLayer(layer) || render_to_separate_surface);
// When globals.can_render_to_separate_surface is false, non-root layers can't
own a surface.
DCHECK(globals.can_render_to_separate_surface ||
    IsRootLayer(layer) ||
    !render_to_separate_surface);

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1824:
layer->SetRenderSurfaceActive(render_to_separate_surface);
The RS computation decides if the render surface should exist. ISTM that this
method call should not exist, cuz the surface already exists or not correctly.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1831: layer->SetRenderSurfaceActive(false);
And since the RS exists or not correctly, we shouldn't be deleting it here?

[awoloszyn, 2014-08-28 19:31:43]

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/delegated_rend...
cc/layers/delegated_renderer_layer_unittest.cc:82: CalcDrawProps(host_.get());
On 2014/08/26 18:06:12, danakj wrote:
> if there's no expects after this, why do it?

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:913:
layer->SetShouldHaveRenderSurface(ShouldHaveRenderSurface());
On 2014/08/26 18:06:13, danakj wrote:
> could this just create or destroy the render surface directly?

I changed the impl-side to just create/destroy it's own RenderSurface based on
the call to layer->SetHasRenderSurface.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1085: draw_properties_.render_target = this;
On 2014/08/26 18:06:12, danakj wrote:
> hm.... can this move to CalcDrawProps?

I moved setting draw_properties_.render_target into CalcDrawProps, so that we
can decide what our render_target is, independently of our having a
render_surface.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.cc#newco...
cc/layers/layer.cc:1251: SetNeedsPushProperties();
This functionality has moved, but I added comments as to why we can avoid the
SetNeedsCommit. Since this gets called during a commit already.
On 2014/08/26 18:06:13, danakj wrote:
> ITYM SetNeedsCommit() or else this will just stay on the main thread until
> something else makes a commit?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h#newcod...
cc/layers/layer.h:257: RenderSurface* render_surface() const { return
render_surface_.get(); }
On 2014/08/26 18:06:13, danakj wrote:
> nit: can you move this out of the middle of the draw_props accessors?

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer.h#newcod...
cc/layers/layer.h:379: void SetRenderSurfaceActive(bool active);
On 2014/08/26 18:06:13, danakj wrote:
> Should we get rid of {Create,Clear}RenderSurface then? I don't like having 2
> ways to do the same thing. Or not add this one?

This one is a bit tricky. RenderSurfaces get destroyed as soon as the rssl goes
out of scope in UpdateDrawProperties, so we no longer have a render-surface when
we go to PushProperties, so we have a separate bool to determine whether or not
we should be letting the impl-side know whether or not it needs one. I could
probably re-work this to SetHasRenderSurface(true/false) and a separate
ClearRenderSurface, which only reset's the render_surface_ ptr. Will definitely
remove the SetShouldHaveRenderSurface and SetRenderSurfaceActive

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:244: draw_properties_.render_target = this;
On 2014/08/26 18:06:13, danakj wrote:
> Can this move to CalcDrawProps?

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.cc#...
cc/layers/layer_impl.cc:1551: active_render_surface_ = render_surface_.get();
This has been re-worked so that the existence of our render_surface is
independent of our render_target.
On 2014/08/26 18:06:13, danakj wrote:
> Can you explain what is going on here, why do you have this
> active_render_surface_ pointer? Either the LayerImpl has one or it doesn't?

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:300: void ClearRenderSurface();
On 2014/08/26 18:06:13, danakj wrote:
> Same Q re {Create,Clear} vs SetActive

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/test/layer_tree_host_...
File cc/test/layer_tree_host_common_test.h (right):

https://codereview.chromium.org/465853004/diff/60001/cc/test/layer_tree_host_...
cc/test/layer_tree_host_common_test.h:68: bool create_render_surface = false);
On 2014/08/26 18:06:13, danakj wrote:
> no default parameters please. this violates the style guide

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1111: static inline typename
LayerType::RenderSurfaceType* CreateOrReuseRenderSurface(
On 2014/08/26 18:06:13, danakj wrote:
> unused?

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1822: layer->ShouldHaveRenderSurface() &&
IsRootLayer(layer);
On 2014/08/26 18:06:13, danakj wrote:
> the root has to have a surface, and nothing else is allowed in this case. can
> you instead do
> 
> bool render_to_separate_surface = layer->ShouldHaveRenderSurface();
> 
> // Root must own a surface.
> DCHECK(!IsRootLayer(layer) || render_to_separate_surface);
> // When globals.can_render_to_separate_surface is false, non-root layers can't
> own a surface.
> DCHECK(globals.can_render_to_separate_surface ||
>     IsRootLayer(layer) ||
>     !render_to_separate_surface);

We may still have a render-surface if !globals.can_render_to_separate_surface,
since that can be changed per-frame on the impl-thread. The first check is done
already on 1983
if (render_to_...) { 
} else {
 DCHECK(layer->parent());
}
but I will add the second check.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1824:
layer->SetRenderSurfaceActive(render_to_separate_surface);
On 2014/08/26 18:06:13, danakj wrote:
> The RS computation decides if the render surface should exist. ISTM that this
> method call should not exist, cuz the surface already exists or not correctly.

Done.

https://codereview.chromium.org/465853004/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_common.cc:1831: layer->SetRenderSurfaceActive(false);
On 2014/08/26 18:06:13, danakj wrote:
> And since the RS exists or not correctly, we shouldn't be deleting it here?

Done.

[danakj, 2014-09-03 19:30:36]

Phew! Went through all the changes in detail. High level this looks much better
thanks! I have two larger questions which are below also:

1. The transform * parent_transform check being used is not the same as the
actual computation for combined transform, and I don't believe we'd get the same
answers all the time. What do you think about this?

2. Now our unit test authors have to be very diligant to SetHasRenderSurface()
when they do things like set a mask or a filter. Can we guard against them doing
this wrong and writing tests with behaviours we wouldn't ship?

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1083: if (has_render_surface_ == has_render_surface) {
nit: no {}

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1086: has_render_surface_ = has_render_surface;
Can this function just Create/reset the surface and we don't need this extra
bool, or the Create/Clear methods? It seems like we do both together always from
LTHCommon.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.h#newco...
cc/layers/layer.h:378: // We do not trigger a Commit when this is called since
we calculate
Can this comment instead say "This should only be called during the
BeginMainFrame", to warn off people who would wanna call it.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1533: if (!!render_surface() ==
should_have_render_surface) {
nit: no {}

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:349: RenderSurfaceImpl* render_surface() const { return
render_surface_.get(); }
nit: move this out of the set of draw_props getters

https://codereview.chromium.org/465853004/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:140:
pending_tree->root_layer()->SetHasRenderSurface(true);
nit: do this on pending_layer above the SetRootLayer call

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:882: CalcDrawProps(&list);
Why do you need this?

Doesn't this clobber the visible_content_rect we set above? If not can you
remove that, and EXPECT_EQ it after this instead?

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1757: CalcDrawProps(&list);
Shouldn't this happen after we set the layer bounds etc? What is the point of
this call then?

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1804: CalcDrawProps(&list);
Similar question re clobbering the draw_props set above.

https://codereview.chromium.org/465853004/diff/100001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:66: bool create_render_surface) {
this is always false, so just remove it?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
File cc/trees/damage_tracker_unittest.cc (left):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1115:
EXPECT_FALSE(grand_child1->render_surface());
this seems like it should be there and we shouldn't have put a surface on it
from the comments

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:131: child1->SetOpacity(0.5f);
does the opacity really matter anymore? do we need to bother setting it?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:144:
grand_child1->SetHasRenderSurface(true);
The comment at the top of this method suggests this shouldn't have a surface.
Why did you add one?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:931: child1->SetOpacity(1.f);
do we need to do this anymore?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:957: child1->SetOpacity(0.5f);
and this?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1158: child->SetHasRenderSurface(true);
move this up to the SetMaskLayer() call on L1144? does the opacity call on L1150
still need to be there?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1384:
ExecuteCalculateDrawProperties(root.get(), &render_surface_layer_list);
why is this needed?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:17: #include
"cc/test/fake_layer_tree_host_impl.h"
including cc/test from a non-test file, this looks like a mistake?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:19: #include "cc/trees/layer_tree_host.h"
why?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1809:
DCHECK(globals.can_render_to_separate_surface || IsRootLayer(layer) ||
this looks a bit awkward, like it's just repeating back the above. i think we
can remove this and instead..

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1813: // Check back-face visibility before
continuing with this surface and its
DCHECK(layer->render_surface()) here

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1817: layer->ClearRenderSurfaceLayerList();
I'm noticing all the ways we have to clear the RS layer list. This is the exact
same thing as you added below on 1824, but goes down a different path. Can we
maybe remove one of these two methods?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1821: layer_draw_properties.render_target =
layer;
remove this, did this twice also on L1825

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2131: // then it has it's own surface
nit: period

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2133: child->render_surface() &&
the render_surface() must be true in that case, so no need to check it here?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2385: transform->IsIdentity() ||
transform->Preserves2dAxisAlignment()))) {
nit: Preserves2dAxisAlignment should already return true for Identity?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2404: transformForChildren *=
parent_transform;
This is not really the same thing as checking the combined_transform. That one
includes things like the layer position, and the transform origin, etc. Couldn't
those things change whether a rotation ends up causing the transform to be 2d
aligned or not? What do you think about that?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3717: // Verify which render
surfaces were created.
created and used?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3721:
EXPECT_FALSE(back_facing_surface->render_target() == back_facing_surface);
should we check that the surface was created also, or no?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3721:
EXPECT_FALSE(back_facing_surface->render_target() == back_facing_surface);
EXPECT_NE

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3985: // Verify which render
surfaces were created.
and used?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3987:
EXPECT_FALSE(back_facing_surface->render_target() ==
should we check that the surface was created also, or no?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3987:
EXPECT_FALSE(back_facing_surface->render_target() ==
EXPECT_NE

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6023: false);
true?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6052:
root->SetHasRenderSurface(true);
not needed if true above?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:7622: true,
this is now 3d sorted?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:7639: true);
why does top/bottom content need a surface for this test?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1784: layer1->SetOpacity(0.3f);
needed still?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:5265:
content_layer->SetHasRenderSurface(true);
For new tests that want to use a mask/replica/filters/etc that would cause them
to have a surface, how can we guard ourselves against ending up forgetting these
SetHasRenderSurface() calls? Do you think we should do that?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:637: root_layer_->CreateRenderSurface();
nit: can you do this in SetupTree?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:661: false);
isn't this one supposed to have a surface based on the sanity check removed
below? why is this passing without?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:203:
CreateRenderSurfaceOnImplThread(layer_ptr);
you could just Types::SetForceRenderSurface(layer_ptr, true)?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:399: void
CreateRenderSurfaceOnImplThread(Layer* root) {}
These seem redundant with SetForceRenderSurface, can we just use those instead?

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:867: typename Types::LayerType* child =
this->CreateLayer(
Can you just CreateSurface here instead

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:869: child->SetMasksToBounds(true);
and drop this and the CreateRSOnImplThread

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:1194: blur_layer->SetFilters(filters);
you can use Type::SetForceRenderSurface instead of CreateRSOnImplThread? Can you
do this beside each call to SetFilters?

[awoloszyn, 2014-09-09 15:31:38]

Addressing 2 comments:
 "For new tests that want to use a mask/replica/filters/etc that would cause
them
to have a surface, how can we guard ourselves against ending up forgetting these
SetHasRenderSurface() calls? Do you think we should do that?"

I was trying to think of a good way to do this in tests, and I have not come up
with any sort of reasonable way to do it, rather than a) littering test code
throughout or b) creating a class that deals with creating a rendersurface for
testing. I think then we just run into the issue where people will not remember
to use the FakeMaskLayer for impl-side testing, in the same way they would fail
to create a RenderSurface.

layer.cc:1086
"Can this function just Create/reset the surface and we don't need this extra
bool, or the Create/Clear methods? It seems like we do both together always from
LTHCommon."

We actually need the separate bool because just after we call CDP on main-thread
we throw out all of our rendersurfaces (clearing the RSSL calls
layer->ClearRenderSurface), so when we go to sync them across they are already
gone. The bool is a persistent way to keep track of whether or not this layer
should have a surface, which only changes on the next call to CDP.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1083: if (has_render_surface_ == has_render_surface) {
On 2014/09/03 19:30:33, danakj wrote:
> nit: no {}

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1086: has_render_surface_ = has_render_surface;
On 2014/09/03 19:30:33, danakj wrote:
> Can this function just Create/reset the surface and we don't need this extra
> bool, or the Create/Clear methods? It seems like we do both together always
from
> LTHCommon.
As far as I can tell we cannot. Once we return from CalculateDrawProps, on the
main thread, we clear the RSSL, which destroys all of the RenderSurfaces. So we
cannot check if we have a render_surface after that point, since the answer will
always be no.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.h#newco...
cc/layers/layer.h:378: // We do not trigger a Commit when this is called since
we calculate
On 2014/09/03 19:30:33, danakj wrote:
> Can this comment instead say "This should only be called during the
> BeginMainFrame", to warn off people who would wanna call it.

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1533: if (!!render_surface() ==
should_have_render_surface) {
On 2014/09/03 19:30:33, danakj wrote:
> nit: no {}

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer_impl.h#...
cc/layers/layer_impl.h:349: RenderSurfaceImpl* render_surface() const { return
render_surface_.get(); }
On 2014/09/03 19:30:33, danakj wrote:
> nit: move this out of the set of draw_props getters

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/picture_layer...
File cc/layers/picture_layer_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/picture_layer...
cc/layers/picture_layer_impl_unittest.cc:140:
pending_tree->root_layer()->SetHasRenderSurface(true);
On 2014/09/03 19:30:33, danakj wrote:
> nit: do this on pending_layer above the SetRootLayer call

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:882: CalcDrawProps(&list);
On 2014/09/03 19:30:33, danakj wrote:
> Why do you need this?
> 
> Doesn't this clobber the visible_content_rect we set above? If not can you
> remove that, and EXPECT_EQ it after this instead?

I removed this (and the others) and correctly created the render_surface
elsewhere.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1757: CalcDrawProps(&list);
On 2014/09/03 19:30:33, danakj wrote:
> Shouldn't this happen after we set the layer bounds etc? What is the point of
> this call then?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/layers/tiled_layer_u...
cc/layers/tiled_layer_unittest.cc:1804: CalcDrawProps(&list);
On 2014/09/03 19:30:33, danakj wrote:
> Similar question re clobbering the draw_props set above.

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/test/layer_tree_host...
File cc/test/layer_tree_host_common_test.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/test/layer_tree_host...
cc/test/layer_tree_host_common_test.cc:66: bool create_render_surface) {
On 2014/09/03 19:30:33, danakj wrote:
> this is always false, so just remove it?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
File cc/trees/damage_tracker_unittest.cc (left):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1115:
EXPECT_FALSE(grand_child1->render_surface());
On 2014/09/03 19:30:33, danakj wrote:
> this seems like it should be there and we shouldn't have put a surface on it
> from the comments

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
File cc/trees/damage_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:131: child1->SetOpacity(0.5f);
On 2014/09/03 19:30:33, danakj wrote:
> does the opacity really matter anymore? do we need to bother setting it?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:144:
grand_child1->SetHasRenderSurface(true);
On 2014/09/03 19:30:34, danakj wrote:
> The comment at the top of this method suggests this shouldn't have a surface.
> Why did you add one?

This was an over-eager fix to some other tests. I have moved the creation into
the appropriate tests.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:931: child1->SetOpacity(1.f);
On 2014/09/03 19:30:33, danakj wrote:
> do we need to do this anymore?

Have removed the SetOpacity, but we must keep the SetHasRenderSurface(false)
since it was set to true in CreateAndSetUpTestTreeWithTwoSurfaces.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:957: child1->SetOpacity(0.5f);
On 2014/09/03 19:30:34, danakj wrote:
> and this?

Same as above, SetOpacity removed.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1158: child->SetHasRenderSurface(true);
On 2014/09/03 19:30:33, danakj wrote:
> move this up to the SetMaskLayer() call on L1144? does the opacity call on
L1150
> still need to be there?

Done. And correct, we no longer need the opacity call.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1384:
ExecuteCalculateDrawProperties(root.get(), &render_surface_layer_list);
On 2014/09/03 19:30:33, danakj wrote:
> why is this needed?

Since we now only set render_targets during CDP we have to run CDP in order to
set them up. (Since we can have a render_surface without a render_target). I
have switched this to the simpler root->draw_properties().draw_target =
root.get();

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:17: #include
"cc/test/fake_layer_tree_host_impl.h"
On 2014/09/03 19:30:34, danakj wrote:
> including cc/test from a non-test file, this looks like a mistake?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:19: #include "cc/trees/layer_tree_host.h"
On 2014/09/03 19:30:34, danakj wrote:
> why?

This is no longer needed, was from earlier patch. Have removed.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1809:
DCHECK(globals.can_render_to_separate_surface || IsRootLayer(layer) ||
On 2014/09/03 19:30:34, danakj wrote:
> this looks a bit awkward, like it's just repeating back the above. i think we
> can remove this and instead..

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1813: // Check back-face visibility before
continuing with this surface and its
On 2014/09/03 19:30:34, danakj wrote:
> DCHECK(layer->render_surface()) here

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1817: layer->ClearRenderSurfaceLayerList();
On 2014/09/03 19:30:34, danakj wrote:
> I'm noticing all the ways we have to clear the RS layer list. This is the
exact
> same thing as you added below on 1824, but goes down a different path. Can we
> maybe remove one of these two methods?

I will replace all with layer->ClearRenderSurfaceLayerList()

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1821: layer_draw_properties.render_target =
layer;
On 2014/09/03 19:30:34, danakj wrote:
> remove this, did this twice also on L1825

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2131: // then it has it's own surface
On 2014/09/03 19:30:34, danakj wrote:
> nit: period

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2133: child->render_surface() &&
On 2014/09/03 19:30:34, danakj wrote:
> the render_surface() must be true in that case, so no need to check it here?

I have re-worked this to be a bit more clear. A child has a render_surface if
child->render_target() == child.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2385: transform->IsIdentity() ||
transform->Preserves2dAxisAlignment()))) {
On 2014/09/03 19:30:34, danakj wrote:
> nit: Preserves2dAxisAlignment should already return true for Identity?

It does already return true for Identity. transform->IsIdentity will be a
cheaper check than transform->Preserves... although this is true only in the
case where SkMatrix44::getType() is already cached. I will remove the
IsIdentity, since it does not obviously provide us with an improvement.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2404: transformForChildren *=
parent_transform;
On 2014/09/03 19:30:34, danakj wrote:
> This is not really the same thing as checking the combined_transform. That one
> includes things like the layer position, and the transform origin, etc.
Couldn't
> those things change whether a rotation ends up causing the transform to be 2d
> aligned or not? What do you think about that?

2d offsets are not able to change the 2d axis aligment of an element. Rotations
will always rotate the rect by Xdeg regardless of where the origin or position
is. We will be unable to use this to determine the ACTUAL position, but we can
safely determine the axis-align-ness of the element.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3717: // Verify which render
surfaces were created.
On 2014/09/03 19:30:34, danakj wrote:
> created and used?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3721:
EXPECT_FALSE(back_facing_surface->render_target() == back_facing_surface);
On 2014/09/03 19:30:35, danakj wrote:
> EXPECT_NE

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3721:
EXPECT_FALSE(back_facing_surface->render_target() == back_facing_surface);
On 2014/09/03 19:30:34, danakj wrote:
> should we check that the surface was created also, or no?

Correct, we should probably make sure that it was created so that we know it
will get propagated to the impl-side.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3985: // Verify which render
surfaces were created.
On 2014/09/03 19:30:35, danakj wrote:
> and used?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3987:
EXPECT_FALSE(back_facing_surface->render_target() ==
On 2014/09/03 19:30:34, danakj wrote:
> should we check that the surface was created also, or no?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:3987:
EXPECT_FALSE(back_facing_surface->render_target() ==
On 2014/09/03 19:30:35, danakj wrote:
> EXPECT_NE

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6023: false);
On 2014/09/03 19:30:35, danakj wrote:
> true?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:6052:
root->SetHasRenderSurface(true);
On 2014/09/03 19:30:34, danakj wrote:
> not needed if true above?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:7622: true,
On 2014/09/03 19:30:35, danakj wrote:
> this is now 3d sorted?

That was incorrect, has been fixed.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:7639: true);
On 2014/09/03 19:30:35, danakj wrote:
> why does top/bottom content need a surface for this test?

This test checks the sorted order of the created render_surfaces and expects
top_ and bottom_ to have render_surfaces.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:1784: layer1->SetOpacity(0.3f);
On 2014/09/03 19:30:35, danakj wrote:
> needed still?
No longer needed.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest.cc:637: root_layer_->CreateRenderSurface();
On 2014/09/03 19:30:35, danakj wrote:
> nit: can you do this in SetupTree?

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl_unittest.cc:661: false);
On 2014/09/03 19:30:35, danakj wrote:
> isn't this one supposed to have a surface based on the sanity check removed
> below? why is this passing without?

We could create a surface, but the sanity check is just checking whether or not
a surface was created. I removed it because in the new world we would just
basically be checking did a RS get created when I said CreateRenderSurface. I
have a follow-up CL with a new set of unit tests specifically for RS creation.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:203:
CreateRenderSurfaceOnImplThread(layer_ptr);
On 2014/09/03 19:30:35, danakj wrote:
> you could just Types::SetForceRenderSurface(layer_ptr, true)?

That would have the same effect, although on MainThread it would also
ForceRenderSurface, which we don't really need in this case, since the root
layer will get one.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:399: void
CreateRenderSurfaceOnImplThread(Layer* root) {}
On 2014/09/03 19:30:35, danakj wrote:
> These seem redundant with SetForceRenderSurface, can we just use those
instead?

This would force a RenderSurface on the main thread as well.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:867: typename Types::LayerType* child =
this->CreateLayer(
On 2014/09/03 19:30:35, danakj wrote:
> Can you just CreateSurface here instead

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:869: child->SetMasksToBounds(true);
On 2014/09/03 19:30:35, danakj wrote:
> and drop this and the CreateRSOnImplThread

Done.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:1194: blur_layer->SetFilters(filters);
On 2014/09/03 19:30:35, danakj wrote:
> you can use Type::SetForceRenderSurface instead of CreateRSOnImplThread? Can
you
> do this beside each call to SetFilters?

Done.

[danakj, 2014-09-25 16:02:12]

> > "For new tests that want to use a mask/replica/filters/etc that would cause
> > them
> > to have a surface, how can we guard ourselves against ending up forgetting
these
> > SetHasRenderSurface() calls? Do you think we should do that?"

> I was trying to think of a good way to do this in tests, and I have not come
up
> with any sort of reasonable way to do it, rather than a) littering test code
> throughout or b) creating a class that deals with creating a rendersurface for
> testing. I think then we just run into the issue where people will not
remember
> to use the FakeMaskLayer for impl-side testing, in the same way they would
fail
> to create a RenderSurface.

How about putting DCHECKs in LayerImpl::SetFoo(a) methods when |a| would cause
it to have a surface, like: DCHECK(!a || surface_)?

Also, we talked about validating that everything in
SubtreeShouldRenderToSeparateSurface is being used to create surfaces in a
minimal way - animations won't create a surface unless at least one frame on the
impl side would create a surface, etc. Did you look into that yet?

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1086: has_render_surface_ = has_render_surface;
On 2014/09/09 15:31:35, awoloszyn wrote:
> On 2014/09/03 19:30:33, danakj wrote:
> > Can this function just Create/reset the surface and we don't need this extra
> > bool, or the Create/Clear methods? It seems like we do both together always
> from
> > LTHCommon.
> As far as I can tell we cannot. Once we return from CalculateDrawProps, on the
> main thread, we clear the RSSL, which destroys all of the RenderSurfaces. So
we
> cannot check if we have a render_surface after that point, since the answer
will
> always be no.

I see, at some point we can probably remove the RenderSurface pointer/type
entirely and just use the bool here then.

The alternative would be to hold onto the RSLL longer on the LTH, but that seems
undesirable. Commits can get cancelled, so the cleanup would need to take such
things into account.

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common_unittest.cc:7639: true);
On 2014/09/09 15:31:37, awoloszyn wrote:
> On 2014/09/03 19:30:35, danakj wrote:
> > why does top/bottom content need a surface for this test?
> 
> This test checks the sorted order of the created render_surfaces and expects
> top_ and bottom_ to have render_surfaces.

From what I see it's testing the order of layers in the root render surface's
layer_list, and the test didn't make surfaces before this change, did it?

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:204: if (requests->empty())
can this DCHECK the layer has a render surface (reorder as needed in push props
here and in layer.cc)

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:218: DCHECK(!copy_requests_.empty());
can this dcheck that the render target is itself?

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:892: if (filters_ == filters)
can this DCHECK that the layer has a surface?

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:911: if (background_filters_ == filters)
and this?

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2379: transform->MakeIdentity();
Please add a thorough comment explaining this MakeIdentity.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2380: if (!layer->render_surface()) {
since we always clear them on the main thread, can this DCHECK instead of if?

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2387: if (layer->render_surface())
can this also DCHECK instead and drop the Clear?

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2394: if (!layer)
why is this needed, you can't call CDP() with a null root layer, right?

https://codereview.chromium.org/465853004/diff/120001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:2449:
Types::SetForceRenderSurface(filtered_surface, true);
nit: move right beside SetBGFilters, whitespace below it.

[awoloszyn, 2014-11-25 15:48:47]

>Also, we talked about validating that everything in
>SubtreeShouldRenderToSeparateSurface is being used to create surfaces in a
>minimal way - animations won't create a surface unless at least one frame on
the
>impl side would create a surface, etc. Did you look into that yet?

I added a check so that we consider something off-axis if it's transform is
off-axis or it has an animation that would put it off-axis so that we correctly
get a surface for impl-side if we ever end up needing one.

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:204: if (requests->empty())
On 2014/09/25 16:02:12, danakj wrote:
> can this DCHECK the layer has a render surface (reorder as needed in push
props
> here and in layer.cc)

Done.

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:218: DCHECK(!copy_requests_.empty());
On 2014/09/25 16:02:12, danakj wrote:
> can this dcheck that the render target is itself?

Done.

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:892: if (filters_ == filters)
On 2014/09/25 16:02:12, danakj wrote:
> can this DCHECK that the layer has a surface?

Done.

https://codereview.chromium.org/465853004/diff/120001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:911: if (background_filters_ == filters)
On 2014/09/25 16:02:12, danakj wrote:
> and this?

Done.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2379: transform->MakeIdentity();
On 2014/09/25 16:02:12, danakj wrote:
> Please add a thorough comment explaining this MakeIdentity.

Done.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2380: if (!layer->render_surface()) {
On 2014/09/25 16:02:12, danakj wrote:
> since we always clear them on the main thread, can this DCHECK instead of if?

If we had a render surface and then it was cleaned up by
RemoveSurfaceForEarlyExit, we still have a render surface but it does not show
up in the RSSL, so when the RSSL cleans up the render surfaces this one does not
go away.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2387: if (layer->render_surface())
On 2014/09/25 16:02:12, danakj wrote:
> can this also DCHECK instead and drop the Clear?
Same as above.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2394: if (!layer)
On 2014/09/25 16:02:12, danakj wrote:
> why is this needed, you can't call CDP() with a null root layer, right?

Was done for a unit-test originally, but no longer needed.

https://codereview.chromium.org/465853004/diff/120001/cc/trees/occlusion_trac...
File cc/trees/occlusion_tracker_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/120001/cc/trees/occlusion_trac...
cc/trees/occlusion_tracker_unittest.cc:2449:
Types::SetForceRenderSurface(filtered_surface, true);
On 2014/09/25 16:02:12, danakj wrote:
> nit: move right beside SetBGFilters, whitespace below it.

Done.

[enne (OOO), 2014-12-12 21:26:30]

enne@chromium.org changed reviewers:
+ enne@chromium.org

[enne (OOO), 2014-12-12 21:26:33]

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:594: bool Layer::HasNotAxisAlignedTransformAnimation() const
{
nit: don't not put negatives in function names; maybe just
AnimationsPreserveAxisAlignment?

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1092: // We do not need SetNeedsCommit here, since this is
only ever called
Could this be less special somehow?

It seems like you're trying to avoid a SetNeedsCommit here because
SetNeedsCommit during LayerTreeHost::UpdateLayers will still cause another
commit.  Could we pull out the CalcDrawProps / RenderSurface calculation prior
to UpdateLayers so that we could cancel commits that get issued as a part of it?
 You could also tack it on to be called at the end of LayerTreeHost::Layout()
instead of at the beginning of LayerTreeHost::UpdateLayers?  Not sure how I feel
about either of those, but they would work.

If none of that sounds palatable, could you make this function private/friended
by the one function that calls it in the right place?

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:921: DCHECK(filters.IsEmpty() || render_surface());
I'm not sure this is the right place for this.  It seems like you're creating an
unnecessary ordering dependency here that SetHasRenderSurface is called before
SetFilters.

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1593: if (should_have_render_surface) {
This function should SetNeedsPushProperties and
set_needs_update_draw_properties, right? Can you add that to the needs update
draw properties test?

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2390: // will now be relative to this Render
Surface.
nit: RenderSurface or "render surface", but not Render Surface

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2392: *non_aligned_animated_transform =
false;
nit: What about draw_transform_is_axis_aligned? I like transforms to say what
space they're in.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2404: static void CreateRenderSurfaces(Layer*
layer,
nit: Can you name these functions consistently? CreateRenderSurfaces calls
CreateOrDestroyRenderSurface.  What about consistently calling them
(Setup|Update)RenderSurface(s)?

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2408: gfx::Transform transformForChildren =
layer->transform();
transform_for_children

[awoloszyn, 2014-12-15 20:24:53]

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:594: bool Layer::HasNotAxisAlignedTransformAnimation() const
{
On 2014/12/12 21:26:32, enne wrote:
> nit: don't not put negatives in function names; maybe just
> AnimationsPreserveAxisAlignment?

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1092: // We do not need SetNeedsCommit here, since this is
only ever called
On 2014/12/12 21:26:32, enne wrote:
> Could this be less special somehow?
> 
> It seems like you're trying to avoid a SetNeedsCommit here because
> SetNeedsCommit during LayerTreeHost::UpdateLayers will still cause another
> commit.  Could we pull out the CalcDrawProps / RenderSurface calculation prior
> to UpdateLayers so that we could cancel commits that get issued as a part of
it?
>  You could also tack it on to be called at the end of LayerTreeHost::Layout()
> instead of at the beginning of LayerTreeHost::UpdateLayers?  Not sure how I
feel
> about either of those, but they would work.
> 
> If none of that sounds palatable, could you make this function
private/friended
> by the one function that calls it in the right place?

I opted to friend the caller (LayerTreeHostCommon). The main reason for the
decision is because we would have to make sure every call of
CalculateDrawProperties is preceded by a call to CreateRenderSurfaces. This
happens a ton in tests. It would also remove what makes sense as a draw
property, from the calculation of the rest of the draw properties.

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:921: DCHECK(filters.IsEmpty() || render_surface());
On 2014/12/12 21:26:32, enne wrote:
> I'm not sure this is the right place for this.  It seems like you're creating
an
> unnecessary ordering dependency here that SetHasRenderSurface is called before
> SetFilters.

This was done primarily for the sake of tests. We make assumptions in CDP that
you have a render surface if you have filters, which hold when running normally,
but have to be set up manually when running tests. (Since we no longer get our
render surfaces from main-thread). Removing may make tests a bit harder to
debug, when this assumption is violated.

I have removed for now.

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:1593: if (should_have_render_surface) {
On 2014/12/12 21:26:32, enne wrote:
> This function should SetNeedsPushProperties and
> set_needs_update_draw_properties, right? Can you add that to the needs update
> draw properties test?

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2390: // will now be relative to this Render
Surface.
On 2014/12/12 21:26:32, enne wrote:
> nit: RenderSurface or "render surface", but not Render Surface

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2392: *non_aligned_animated_transform =
false;
On 2014/12/12 21:26:32, enne wrote:
> nit: What about draw_transform_is_axis_aligned? I like transforms to say what
> space they're in.

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2404: static void CreateRenderSurfaces(Layer*
layer,
On 2014/12/12 21:26:32, enne wrote:
> nit: Can you name these functions consistently? CreateRenderSurfaces calls
> CreateOrDestroyRenderSurface.  What about consistently calling them
> (Setup|Update)RenderSurface(s)?

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2404: static void CreateRenderSurfaces(Layer*
layer,
On 2014/12/12 21:26:32, enne wrote:
> nit: Can you name these functions consistently? CreateRenderSurfaces calls
> CreateOrDestroyRenderSurface.  What about consistently calling them
> (Setup|Update)RenderSurface(s)?

Done.

https://codereview.chromium.org/465853004/diff/160001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:2408: gfx::Transform transformForChildren =
layer->transform();
On 2014/12/12 21:26:32, enne wrote:
> transform_for_children

Done.

[enne (OOO), 2014-12-15 22:46:51]

Looking good.  A few small comments:

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1092: // We do not need SetNeedsCommit here, since this is
only ever called
On 2014/12/15 20:24:53, awoloszyn wrote:
> On 2014/12/12 21:26:32, enne wrote:
> > Could this be less special somehow?
> > 
> > It seems like you're trying to avoid a SetNeedsCommit here because
> > SetNeedsCommit during LayerTreeHost::UpdateLayers will still cause another
> > commit.  Could we pull out the CalcDrawProps / RenderSurface calculation
prior
> > to UpdateLayers so that we could cancel commits that get issued as a part of
> it?
> >  You could also tack it on to be called at the end of
LayerTreeHost::Layout()
> > instead of at the beginning of LayerTreeHost::UpdateLayers?  Not sure how I
> feel
> > about either of those, but they would work.
> > 
> > If none of that sounds palatable, could you make this function
> private/friended
> > by the one function that calls it in the right place?
> 
> I opted to friend the caller (LayerTreeHostCommon). The main reason for the
> decision is because we would have to make sure every call of
> CalculateDrawProperties is preceded by a call to CreateRenderSurfaces. This
> happens a ton in tests. It would also remove what makes sense as a draw
> property, from the calculation of the rest of the draw properties.

I kind of feel like we should separate out those steps, but maybe we can talk
about that as a follow up.

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:921: DCHECK(filters.IsEmpty() || render_surface());
On 2014/12/15 20:24:53, awoloszyn wrote:
> On 2014/12/12 21:26:32, enne wrote:
> > I'm not sure this is the right place for this.  It seems like you're
creating
> an
> > unnecessary ordering dependency here that SetHasRenderSurface is called
before
> > SetFilters.
> 
> This was done primarily for the sake of tests. We make assumptions in CDP that
> you have a render surface if you have filters, which hold when running
normally,
> but have to be set up manually when running tests. (Since we no longer get our
> render surfaces from main-thread). Removing may make tests a bit harder to
> debug, when this assumption is violated.
> 
> I have removed for now.

I think the check makes a lot of sense and will help catch such errors in the
future, but I think this is just in the wrong place.  If you want to have such a
debug check, maybe add a post-calcdraw iteration over layers? Or, a
pre-appendquads step?

https://codereview.chromium.org/465853004/diff/180001/cc/animation/transform_...
File cc/animation/transform_operations.h (right):

https://codereview.chromium.org/465853004/diff/180001/cc/animation/transform_...
cc/animation/transform_operations.h:66: // Returns true if the operations affect
2d axis alignment.
Returns false?

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer.h#newco...
cc/layers/layer.h:649: scoped_ptr<RenderSurface> render_surface_;
Can you leave a TODO here that this is redundant with has_render_surface_ and
should get removed?

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:242: if (render_surface_)
render_surface_->layer_list().clear();
Did git cl format do this?  It doesn't seem to do that locally for me.

https://codereview.chromium.org/465853004/diff/180001/ui/compositor/transform...
File ui/compositor/transform_animation_curve_adapter.cc (right):

https://codereview.chromium.org/465853004/diff/180001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:75:
(target_value_.IsScaleOrTranslation());
What if the target value is identity?

[awoloszyn, 2014-12-16 15:50:44]

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/465853004/diff/160001/cc/layers/layer.cc#newc...
cc/layers/layer.cc:1092: // We do not need SetNeedsCommit here, since this is
only ever called
On 2014/12/15 22:46:46, enne wrote:
> On 2014/12/15 20:24:53, awoloszyn wrote:
> > On 2014/12/12 21:26:32, enne wrote:
> > > Could this be less special somehow?
> > > 
> > > It seems like you're trying to avoid a SetNeedsCommit here because
> > > SetNeedsCommit during LayerTreeHost::UpdateLayers will still cause another
> > > commit.  Could we pull out the CalcDrawProps / RenderSurface calculation
> prior
> > > to UpdateLayers so that we could cancel commits that get issued as a part
of
> > it?
> > >  You could also tack it on to be called at the end of
> LayerTreeHost::Layout()
> > > instead of at the beginning of LayerTreeHost::UpdateLayers?  Not sure how
I
> > feel
> > > about either of those, but they would work.
> > > 
> > > If none of that sounds palatable, could you make this function
> > private/friended
> > > by the one function that calls it in the right place?
> > 
> > I opted to friend the caller (LayerTreeHostCommon). The main reason for the
> > decision is because we would have to make sure every call of
> > CalculateDrawProperties is preceded by a call to CreateRenderSurfaces. This
> > happens a ton in tests. It would also remove what makes sense as a draw
> > property, from the calculation of the rest of the draw properties.
> 
> I kind of feel like we should separate out those steps, but maybe we can talk
> about that as a follow up.

Done.

https://codereview.chromium.org/465853004/diff/180001/cc/animation/transform_...
File cc/animation/transform_operations.h (right):

https://codereview.chromium.org/465853004/diff/180001/cc/animation/transform_...
cc/animation/transform_operations.h:66: // Returns true if the operations affect
2d axis alignment.
On 2014/12/15 22:46:51, enne wrote:
> Returns false?

Done.

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer.h
File cc/layers/layer.h (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer.h#newco...
cc/layers/layer.h:649: scoped_ptr<RenderSurface> render_surface_;
On 2014/12/15 22:46:51, enne wrote:
> Can you leave a TODO here that this is redundant with has_render_surface_ and
> should get removed?

Done.

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:242: if (render_surface_)
render_surface_->layer_list().clear();
On 2014/12/15 22:46:51, enne wrote:
> Did git cl format do this?  It doesn't seem to do that locally for me.

Yes, it made a couple changes like this, that I find were a bit suspect, I can
switch it back if that is clearer (which I feel like it is).

https://codereview.chromium.org/465853004/diff/180001/ui/compositor/transform...
File ui/compositor/transform_animation_curve_adapter.cc (right):

https://codereview.chromium.org/465853004/diff/180001/ui/compositor/transform...
ui/compositor/transform_animation_curve_adapter.cc:75:
(target_value_.IsScaleOrTranslation());
On 2014/12/15 22:46:51, enne wrote:
> What if the target value is identity?

Done.

[danakj, 2014-12-16 16:06:43]

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:242: if (render_surface_)
render_surface_->layer_list().clear();
On 2014/12/16 15:50:44, awoloszyn wrote:
> On 2014/12/15 22:46:51, enne wrote:
> > Did git cl format do this?  It doesn't seem to do that locally for me.
> 
> Yes, it made a couple changes like this, that I find were a bit suspect, I can
> switch it back if that is clearer (which I feel like it is).

If you do reject any formatting, please file a bug against clang-format. And if
they WontFix it then you should just use it.

[enne (OOO), 2014-12-16 17:21:01]

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:242: if (render_surface_)
render_surface_->layer_list().clear();
As danakj says, change them back, file a bug.  I did a pull yesterday to test
that, so am curious why it didn't format like that for me.

[awoloszyn, 2014-12-16 18:01:52]

On 2014/12/16 17:21:01, enne wrote:
> https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc
> File cc/layers/layer_impl.cc (right):
> 
>
https://codereview.chromium.org/465853004/diff/180001/cc/layers/layer_impl.cc...
> cc/layers/layer_impl.cc:242: if (render_surface_)
> render_surface_->layer_list().clear();
> As danakj says, change them back, file a bug.  I did a pull yesterday to test
> that, so am curious why it didn't format like that for me.

It turns out that I had another version of clang-format installed that was
clobbering
the other formatting. I have fixed my path, and now things look more sane.

[enne (OOO), 2014-12-16 19:08:48]

https://codereview.chromium.org/465853004/diff/220001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:11: #include
"cc/resources/resource_update_queue.h"
Is this needed?

https://codereview.chromium.org/465853004/diff/220001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:11: #include
"cc/resources/resource_update_queue.h"
Is this needed?

https://codereview.chromium.org/465853004/diff/220001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:220: DCHECK(render_target() == this);
DCHECK_EQ, to differentiate NULL vs different

https://codereview.chromium.org/465853004/diff/220001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:220: DCHECK(render_target() == this);
DCHECK_EQ, to differentiate NULL vs different

https://codereview.chromium.org/465853004/diff/220001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1193: static bool ValidateLayer(LayerImpl*
layer) {
What about calling this ValidateRenderSurface? Also, if you're going to check
this, should you also check other properties from
SubtreeShouldRenderToSeparateSurface?

[awoloszyn, 2014-12-16 21:54:29]

https://codereview.chromium.org/465853004/diff/220001/cc/layers/delegated_ren...
File cc/layers/delegated_renderer_layer_unittest.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/layers/delegated_ren...
cc/layers/delegated_renderer_layer_unittest.cc:11: #include
"cc/resources/resource_update_queue.h"
On 2014/12/16 19:08:48, enne wrote:
> Is this needed?

Nope, has been removed.

https://codereview.chromium.org/465853004/diff/220001/cc/layers/layer_impl.cc
File cc/layers/layer_impl.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/layers/layer_impl.cc...
cc/layers/layer_impl.cc:220: DCHECK(render_target() == this);
On 2014/12/16 19:08:48, enne wrote:
> DCHECK_EQ, to differentiate NULL vs different

Done.

https://codereview.chromium.org/465853004/diff/220001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/220001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1193: static bool ValidateLayer(LayerImpl*
layer) {
On 2014/12/16 19:08:48, enne wrote:
> What about calling this ValidateRenderSurface? Also, if you're going to check
> this, should you also check other properties from
> SubtreeShouldRenderToSeparateSurface?

Done.

[enne (OOO), 2014-12-16 22:00:16]

lgtm

https://codereview.chromium.org/465853004/diff/240001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/465853004/diff/240001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_common.cc:1196: return (layer->render_surface() ||
I think this would be easier to read with an early out for
layer->render_surface() rather than the really long || (&& && &&) conditional.

[awoloszyn, 2014-12-16 22:09:17]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-16 22:10:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/260001

[commit-bot: I haz the power, 2014-12-16 22:15:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-16 22:15:15]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/99685)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[awoloszyn, 2014-12-17 15:24:42]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-17 15:25:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/280001

[commit-bot: I haz the power, 2014-12-17 15:31:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 15:31:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[awoloszyn, 2014-12-17 21:56:05]

awoloszyn@chromium.org changed reviewers:
+ ajuma@chromium.org

[awoloszyn, 2014-12-17 21:56:06]

+ajuma for ui/compositor/transform_animation*

[ajuma, 2014-12-17 21:59:00]

On 2014/12/17 21:56:06, awoloszyn wrote:
> +ajuma for ui/compositor/transform_animation*

ui/compositor/transform_animation* lgtm.

[awoloszyn, 2014-12-17 22:07:09]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-17 22:08:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/280001

[commit-bot: I haz the power, 2014-12-17 23:44:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-17 23:44:49]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2014-12-18 18:31:16]

lgtm still

[awoloszyn, 2014-12-18 18:45:07]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-18 18:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/300001

[commit-bot: I haz the power, 2014-12-18 21:18:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-18 21:18:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[awoloszyn, 2014-12-19 20:58:21]

It turns out "layer->ClearRenderSurfaceLayerList()" and
layer->render_surface()->ClearLayerLists() is not the same, although it is
"mostly" the same. I have made them the same, which fixes the try failures, as
well as clears up some confusion.

[enne (OOO), 2014-12-19 21:01:11]

lgtm

[awoloszyn, 2014-12-19 21:32:16]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-19 21:33:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/320001

[commit-bot: I haz the power, 2014-12-19 21:39:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-19 21:39:31]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[awoloszyn, 2014-12-22 14:41:29]

The CQ bit was checked by awoloszyn@chromium.org

[commit-bot: I haz the power, 2014-12-22 14:41:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/465853004/340001

[commit-bot: I haz the power, 2014-12-22 15:40:30]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2014-12-22 15:41:20]

Patchset 18 (id:??) landed as
https://crrev.com/e83f28ca816694be553baed069f784eb286d8170
Cr-Commit-Position: refs/heads/master@{#309428}