cc: Push valid property values when CalcDrawProps skips layer.

cc: Push valid property values when CalcDrawProps skips layer.

If CalcDropProps does not run on a layer for any reason, then we
do not SavePaintProperties() on the layer. When we push the layer's
properties, we can not push the paint properties since they are not
valid. We should instead push the raw value on the layer.

This is safe because if we didn't SavePaintProperties() we will also
not call Update() on the layer, so there is no chance for painting
to change any property values. This is enforced by a new DCHECK()
in Layer::Update() and we have all the subclasses of Layer call up
to the Update() method.

R=boliu, enne
BUG=267574
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215512

[enne (OOO), 2013-08-02 17:00:47]

Yay, thanks for this cleanup.  :)

https://codereview.chromium.org/21839004/diff/15001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:851: DCHECK(saved_paint_properties_);
This seems like something that could be easily skipped if codereview doesn't
catch it.  What about saved_paint_properties =>
last_save_paint_properties_frame? Then you wouldn't have to reset this var.

Maybe that var could also just go in the saved paint properties struct too?

https://codereview.chromium.org/21839004/diff/15001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/layers/picture_layer.c...
cc/layers/picture_layer.cc:83: // TODO(danakj): Maybe this should not inherit
from ContentsScalingLayer then.
Agreed.

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
This is wrong for WebView.  You can't just add it back.

[boliu, 2013-08-02 17:44:50]

Tested PS2 (with the viewport empty early out added back). Works fine for
webview.

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
On 2013/08/02 17:00:48, enne wrote:
> This is wrong for WebView.  You can't just add it back.

Not commenting on correctness, just a question:

Is not possible for content area in desktop chrome be resized to nothing, open a
new tab, and it would fall into this case?

[enne (OOO), 2013-08-02 17:47:41]

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
On 2013/08/02 17:44:50, boliu wrote:
> On 2013/08/02 17:00:48, enne wrote:
> > This is wrong for WebView.  You can't just add it back.
> 
> Not commenting on correctness, just a question:
> 
> Is not possible for content area in desktop chrome be resized to nothing, open
a
> new tab, and it would fall into this case?

It is possible, but in that case the compositor thread wouldn't draw anything
either.  This viewport would get passed along and used on the compositor thread.

In the WebView case, you can pass an arbitrary viewport to draw with, so if you
skip on the main thread you may not update layers that need updating and then
you'd attempt to draw them on the compositor with a non-empty viewport.

This patch may give you the correct bounds on the compositor thread, but you
wouldn't get the PictureLayer::Update call so you wouldn't get the right
content.

[danakj, 2013-08-02 18:35:11]

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
On 2013/08/02 17:47:41, enne wrote:
> On 2013/08/02 17:44:50, boliu wrote:
> > On 2013/08/02 17:00:48, enne wrote:
> > > This is wrong for WebView.  You can't just add it back.
> > 
> > Not commenting on correctness, just a question:
> > 
> > Is not possible for content area in desktop chrome be resized to nothing,
open
> a
> > new tab, and it would fall into this case?
> 
> It is possible, but in that case the compositor thread wouldn't draw anything
> either.  This viewport would get passed along and used on the compositor
thread.
> 
> In the WebView case, you can pass an arbitrary viewport to draw with, so if
you
> skip on the main thread you may not update layers that need updating and then
> you'd attempt to draw them on the compositor with a non-empty viewport.
> 
> This patch may give you the correct bounds on the compositor thread, but you
> wouldn't get the PictureLayer::Update call so you wouldn't get the right
> content.

I think checkerboards should be fine in this case. In general, any layer could
be outside the viewport and not get updated, and then we draw with a different
viewport. I'm not sure we need to make this work specially when the first
viewport was empty. It will still clip everything, and we'll activate without
any tiles rastered.

[enne (OOO), 2013-08-02 18:41:49]

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
On 2013/08/02 18:35:11, danakj wrote:

> I think checkerboards should be fine in this case. In general, any layer could
> be outside the viewport and not get updated, and then we draw with a different
> viewport. I'm not sure we need to make this work specially when the first
> viewport was empty. It will still clip everything, and we'll activate without
> any tiles rastered.

Checkerboarding would be wrong for software raster in WebView, as the
PictureLayer needs to be updated so PictureLayerImpl can get its picture.

LayerTreeHostCommon does not clip or skip updating layers that are outside the
viewport.  The only time we drop layers from a RSLL is when they are outside a
clipping render surface (not the root), because we can't change the size of that
clip rect from the compositor thread.

I still think this check is wrong for WebView.

[danakj, 2013-08-02 21:49:13]

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:681: if (device_viewport_size().IsEmpty())
On 2013/08/02 18:41:49, enne wrote:
> On 2013/08/02 18:35:11, danakj wrote:
> 
> > I think checkerboards should be fine in this case. In general, any layer
could
> > be outside the viewport and not get updated, and then we draw with a
different
> > viewport. I'm not sure we need to make this work specially when the first
> > viewport was empty. It will still clip everything, and we'll activate
without
> > any tiles rastered.
> 
> Checkerboarding would be wrong for software raster in WebView, as the
> PictureLayer needs to be updated so PictureLayerImpl can get its picture.
> 
> LayerTreeHostCommon does not clip or skip updating layers that are outside the
> viewport.  The only time we drop layers from a RSLL is when they are outside a
> clipping render surface (not the root), because we can't change the size of
that
> clip rect from the compositor thread.
> 
> I still think this check is wrong for WebView.

Ah, okie.

[danakj, 2013-08-02 21:57:28]

PTAL

https://codereview.chromium.org/21839004/diff/15001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/21839004/diff/15001/cc/layers/layer.cc#newcod...
cc/layers/layer.cc:851: DCHECK(saved_paint_properties_);
On 2013/08/02 17:00:48, enne wrote:
> This seems like something that could be easily skipped if codereview doesn't
> catch it.  What about saved_paint_properties =>
> last_save_paint_properties_frame? Then you wouldn't have to reset this var.
> 
> Maybe that var could also just go in the saved paint properties struct too?

Done.

[enne (OOO), 2013-08-02 22:04:55]

lgtm

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:748: bool did_paint = false;
I think you could just initialize did_paint to contents_scaling_layer_updated
and you wouldn't need the DCHECK above?

https://codereview.chromium.org/21839004/diff/32001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/21839004/diff/32001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:4081:
root_layer_->SetAnchorPoint(gfx::PointF());
Are the anchor point and position settings necessary here?

[danakj, 2013-08-02 22:05:58]

https://codereview.chromium.org/21839004/diff/32001/cc/trees/layer_tree_host_...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/21839004/diff/32001/cc/trees/layer_tree_host_...
cc/trees/layer_tree_host_unittest.cc:4081:
root_layer_->SetAnchorPoint(gfx::PointF());
On 2013/08/02 22:04:55, enne wrote:
> Are the anchor point and position settings necessary here?

I guess not, it just makes the layer be positioned with its center on the
viewport's origin. The position is probably just redundant entirely.

[danakj, 2013-08-02 22:11:27]

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:748: bool did_paint = false;
On 2013/08/02 22:04:55, enne wrote:
> I think you could just initialize did_paint to contents_scaling_layer_updated
> and you wouldn't need the DCHECK above?

Ah true, I think I'd like to rename this var to "updated" and just only have one
bool here, and match other layer types.

[danakj, 2013-08-02 22:12:10]

PTAL, thanks! Renamed did_paint

[enne (OOO), 2013-08-02 22:13:20]

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/21839004/diff/32001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:748: bool did_paint = false;
On 2013/08/02 22:11:27, danakj wrote:
> On 2013/08/02 22:04:55, enne wrote:
> > I think you could just initialize did_paint to
contents_scaling_layer_updated
> > and you wouldn't need the DCHECK above?
> 
> Ah true, I think I'd like to rename this var to "updated" and just only have
one
> bool here, and match other layer types.

sgtm!

[commit-bot: I haz the power, 2013-08-02 22:14:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/21839004/44001

[danakj, 2013-08-02 22:28:33]

Well.. we can't use the layer tree host unconditionally inside
SavePaintProperties, since tests call CalcDrawProps without a LTH.. We can

1. Make all tests with CalcDrawProps have a LTH.
2. Use a bool again.
3. Only use the LTH if it is not null with a comment that this is for tests.
4. ?

[commit-bot: I haz the power, 2013-08-02 22:56:43]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-03 02:35:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/21839004/61001

[commit-bot: I haz the power, 2013-08-03 03:39:30]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-08-03 14:21:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/21839004/61001

[commit-bot: I haz the power, 2013-08-03 14:48:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/21839004/83002

[commit-bot: I haz the power, 2013-08-03 14:55:56]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...

[commit-bot: I haz the power, 2013-08-03 14:57:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/21839004/83002

[commit-bot: I haz the power, 2013-08-03 18:44:31]

Change committed as 215512