cc : Stop pushing properties not used by LayerImpl to LayerImpl. (2)

cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6b5a32bf6cabef2f242db43bb483b2e9a94b9c8d
Cr-Commit-Position: refs/heads/master@{#389256}

[jaydasika, 2016-04-21 22:29:14]

Description was changed from

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProeprties. Since force_render_surface tracking is required
to compute damage rect, I have added layer_property_chnaged bool to
Layer.

BUG=602438
==========

to

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProeprties. Since force_render_surface tracking is required
to compute damage rect, I have added layer_property_chnaged bool to
Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-04-21 22:29:34]

Description was changed from

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProeprties. Since force_render_surface tracking is required
to compute damage rect, I have added layer_property_chnaged bool to
Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties. Since force_render_surface tracking is required
to compute damage rect, I have added layer_property_chnaged bool to
Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[jaydasika, 2016-04-21 22:36:31]

PTAL

[jaydasika, 2016-04-21 22:37:04]

jaydasika@chromium.org changed reviewers:
+ ajuma@chromium.org, vollick@chromium.org, weiliangc@chromium.org

[jaydasika, 2016-04-21 22:37:05]

PTAL

[ajuma, 2016-04-21 23:59:15]

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

https://codereview.chromium.org/1912893002/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:491: bool layer_property_changed() const { return
layer_property_changed_; }
There aren't any real non-test callers of SetForceRenderSurface. It's exposed on
WebLayer and ui::Layer but not used in a non-trivial way (ui::Layer uses it in a
test, but that can be deleted). What if we remove
WebLayer::setForceRenderSurface and ui::Layer::SetForceRenderSurface, and rename
the cc::Layer method to SetForRenderSurfaceForTesting? Then we don't need to add
or push this layer_property_changed_ bool (since any tests that depend on
SetForceRenderSurface to cause damage should be changed to not depend on this).
Does that seem reasonable?

https://codereview.chromium.org/1912893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp
(right):

https://codereview.chromium.org/1912893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp:51:
layer->test_properties()->force_render_surface = true;
I think this line can be deleted altogether.

[jaydasika, 2016-04-22 17:00:39]

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

https://codereview.chromium.org/1912893002/diff/20001/cc/layers/layer.h#newco...
cc/layers/layer.h:491: bool layer_property_changed() const { return
layer_property_changed_; }
On 2016/04/21 23:59:14, ajuma wrote:
> There aren't any real non-test callers of SetForceRenderSurface. It's exposed
on
> WebLayer and ui::Layer but not used in a non-trivial way (ui::Layer uses it in
a
> test, but that can be deleted). What if we remove
> WebLayer::setForceRenderSurface and ui::Layer::SetForceRenderSurface, and
rename
> the cc::Layer method to SetForRenderSurfaceForTesting? Then we don't need to
add
> or push this layer_property_changed_ bool (since any tests that depend on
> SetForceRenderSurface to cause damage should be changed to not depend on
this).
> Does that seem reasonable?

Done.

https://codereview.chromium.org/1912893002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp
(right):

https://codereview.chromium.org/1912893002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/CompositorMutableStateTest.cpp:51:
layer->test_properties()->force_render_surface = true;
On 2016/04/21 23:59:14, ajuma wrote:
> I think this line can be deleted altogether.

Done.

[ajuma, 2016-04-22 18:04:45]

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

https://codereview.chromium.org/1912893002/diff/80001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:576: child->NoteLayerPropertyChanged();
It looks like this test has been relying on SetForceRenderSurface to trigger
damage rather than testing that SetFilters triggers damage. What if we change
this so that we rebuild property trees after forcing the render surface, and
then we call OnFilterAnimated instead of SetFilters? Then we shouldn't need to
call NoteLayerPropertyChanged here.

https://codereview.chromium.org/1912893002/diff/80001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1133:
grand_child1->NoteLayerPropertyChanged();
This one's a bit trickier, since replicas don't ordinarily change on the
compositor. So the call to NoteLayerPropertyChanged might be ok, as long as we
have a test elsewhere that when a replica is added on the main thread, we note
the damage and push it.

[jaydasika, 2016-04-22 19:19:08]

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

https://codereview.chromium.org/1912893002/diff/80001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:576: child->NoteLayerPropertyChanged();
On 2016/04/22 18:04:45, ajuma wrote:
> It looks like this test has been relying on SetForceRenderSurface to trigger
> damage rather than testing that SetFilters triggers damage. What if we change
> this so that we rebuild property trees after forcing the render surface, and
> then we call OnFilterAnimated instead of SetFilters? Then we shouldn't need to
> call NoteLayerPropertyChanged here.

Done.

https://codereview.chromium.org/1912893002/diff/80001/cc/trees/damage_tracker...
cc/trees/damage_tracker_unittest.cc:1133:
grand_child1->NoteLayerPropertyChanged();
On 2016/04/22 18:04:45, ajuma wrote:
> This one's a bit trickier, since replicas don't ordinarily change on the
> compositor. So the call to NoteLayerPropertyChanged might be ok, as long as we
> have a test elsewhere that when a replica is added on the main thread, we note
> the damage and push it.

Added a test.

[ajuma, 2016-04-22 20:10:48]

Thanks, lgtm. Please update the description (since the handling of
SetForceRenderSuface has changed).

[jaydasika, 2016-04-22 20:26:13]

On 2016/04/22 20:10:48, ajuma wrote:
> Thanks, lgtm. Please update the description (since the handling of
> SetForceRenderSuface has changed).

vollick : Can you review changes to ui/compositor and platform/graphics ?

[jaydasika, 2016-04-22 20:27:27]

Description was changed from

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties. Since force_render_surface tracking is required
to compute damage rect, I have added layer_property_chnaged bool to
Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Ian Vollick, 2016-04-22 20:32:20]

On 2016/04/22 20:26:13, jaydasika wrote:
> On 2016/04/22 20:10:48, ajuma wrote:
> > Thanks, lgtm. Please update the description (since the handling of
> > SetForceRenderSuface has changed).
> 
> vollick : Can you review changes to ui/compositor and platform/graphics ?

lgtm if the bots are happy.

[jaydasika, 2016-04-22 20:37:55]

jaydasika@chromium.org changed reviewers:
+ pdr@chromium.org

[jaydasika, 2016-04-22 20:37:56]

+pdr
Can you review changes to public/platform ? I deleted some code that not used.

[pdr., 2016-04-22 21:21:51]

On 2016/04/22 at 20:37:56, jaydasika wrote:
> +pdr
> Can you review changes to public/platform ? I deleted some code that not used.

Deleting code always gets an LGTM from me!
LGTM

[jaydasika, 2016-04-22 21:25:14]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-04-22 21:25:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1912893002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1912893002/100001

[commit-bot: I haz the power, 2016-04-22 21:56:47]

Description was changed from

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-04-22 21:56:48]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-22 21:59:04]

Description was changed from

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc : Stop pushing properties not used on compositor to compositor. (2)

This CL moves double_sided and force_render_surface from LayerImpl to
LayerImplTestProperties and also deletes non-test calls to
SetForceRenderSurface on Layer.

BUG=602438
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/6b5a32bf6cabef2f242db43bb483b2e9a94b9c8d
Cr-Commit-Position: refs/heads/master@{#389256}
==========

[commit-bot: I haz the power, 2016-04-22 21:59:05]

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