cc: Add strict layer property change checking and handle bounds changes during paint.

cc: Add strict layer property change checking and handle bounds changes during paint.

This fixes a problem where paint would cause layer bounds to change
and Layer::PushProperties to push an non-empty picture layer without
a recording to the impl side.

This also adds a new command line switch that make layer property
changes during paint cause a CHECK() to fail. This can be used
to diagnose situations where paint is incorrectly causing layer
properties to change.

BUG=229179
TEST=cc_unittests --gtest_filter=LayerTreeHostTestChangeLayerPropertiesInPaintContents.RunSingleThread

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194269

[danakj, 2013-04-12 16:44:39]

https://codereview.chromium.org/13939005/diff/1/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/13939005/diff/1/cc/layers/layer.cc#newcode617
cc/layers/layer.cc:617: layer->SetBounds(last_bounds_);
I think this is problematic because if the bounds change, we still use the new
bounds, even though we push the old bounds.

We should either:
1) Conditionally set the real value or a "pending" value, and copying the
pending value over the real value after pushprops.
2) Make a PaintProperties struct on the layer. Save this struct before painting.
Use the values instead of live values during painting/pushing.

[enne (OOO), 2013-04-12 22:20:12]

Can we fix this upstream in Blink and then just turn on this check
unconditionally?

[danakj, 2013-04-12 22:25:52]

On Fri, Apr 12, 2013 at 3:20 PM, <enne@chromium.org> wrote:

> Can we fix this upstream in Blink and then just turn on this check
> unconditionally?
>

From a chat with James yesterday, these are bugs and we should fix them
(hence optional crashing) but there are a lot of them and we should be
robust to them.


>
>
https://codereview.chromium.**org/13939005/<https://codereview.chromium.org/1...
>

[enne (OOO), 2013-04-12 23:03:22]

If plugins can abuse this then maybe we need to follow up with adding other
paint properties, but from a risk management perspective I'd like to only add
this one property now.

This lgtm, except for one change below.

https://codereview.chromium.org/13939005/diff/4001/cc/trees/layer_tree_host_c...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/13939005/diff/4001/cc/trees/layer_tree_host_c...
cc/trees/layer_tree_host_common.cc:1531: save_paint_properties);
I don't think you need this.  We can just always call it and use the template
specialization to compile it out.

[reveman, 2013-04-12 23:41:48]

https://codereview.chromium.org/13939005/diff/4001/cc/trees/layer_tree_host_c...
File cc/trees/layer_tree_host_common.cc (right):

https://codereview.chromium.org/13939005/diff/4001/cc/trees/layer_tree_host_c...
cc/trees/layer_tree_host_common.cc:1531: save_paint_properties);
On 2013/04/12 23:03:22, enne wrote:
> I don't think you need this.  We can just always call it and use the template
> specialization to compile it out.

Done.

[reveman, 2013-04-13 01:26:59]

+piman

as this might need to be merged to M27 for android I'm avoiding to add more
fields to PaintProperties in this patch but instead saving that for a follow.
created a bug and added a TODO for this.

[jamesr, 2013-04-15 17:31:59]

lgtm

We definitely should attempt to track down and fix as many of the Blink issues
as we can.  We can't rely on anything in the face of windowless NPAPI plugins,
but those don't exist on ChromeOS/Android at least.

[piman, 2013-04-15 17:34:55]

Why CHECK+flag rather than DCHECK? This looks like a debugging aid, and not
something we would want to be crashing the browser for. Would we ever turn
on this flag in production?


On Fri, Apr 12, 2013 at 6:27 PM, <reveman@chromium.org> wrote:

> +piman
>
> as this might need to be merged to M27 for android I'm avoiding to add more
> fields to PaintProperties in this patch but instead saving that for a
> follow.
> created a bug and added a TODO for this.
>
>
https://codereview.chromium.**org/13939005/<https://codereview.chromium.org/1...
>

[reveman, 2013-04-15 17:45:47]

On 2013/04/15 17:34:55, piman wrote:
> Why CHECK+flag rather than DCHECK? This looks like a debugging aid, and not
> something we would want to be crashing the browser for. Would we ever turn
> on this flag in production?

maybe in canary or dev on platforms without NPAPI when we get to the point where
we think all blink issues are fixed.

[reveman, 2013-04-15 17:48:06]

On 2013/04/15 17:34:55, piman wrote:
> Why CHECK+flag rather than DCHECK?

this is way too broken at the moment for DCHECK. you can't load a single page
without hitting it.

[piman, 2013-04-15 17:59:23]

On Mon, Apr 15, 2013 at 10:45 AM, <reveman@chromium.org> wrote:

> On 2013/04/15 17:34:55, piman wrote:
>
>> Why CHECK+flag rather than DCHECK? This looks like a debugging aid, and
>> not
>> something we would want to be crashing the browser for. Would we ever turn
>> on this flag in production?
>>
>
> maybe in canary or dev on platforms without NPAPI when we get to the point
> where
> we think all blink issues are fixed.
>

It would be sad that we chose to crash our users for something we otherwise
can handle, even if not perfectly (what the rest of your patch does).
IOW, I like the fix, I hate the idea of this flag.

Anyway, LGTM because we want to merge this, but it doesn't make me happy at
all.


>
>
https://codereview.chromium.**org/13939005/<https://codereview.chromium.org/1...
>

[jamesr, 2013-04-15 18:07:49]

We need to have the fix and we need a way to figure out when Blink is
misbehaving so we can track down and fix it.  An alternative way to do it would
be to have devs make a local code change to DCHECK()/whatnot when they want to
look into it.

On further thought I don't like having this be a CHECK(), however, since it's
not useful to use the flag unless you are debugging anyway.

[reveman, 2013-04-15 18:49:21]

Switch to DCHECK instead.

When NPAPI is gone I'm hoping this flag can also go. We currently suspect that a
lot of the non-NPAPI cases that trigger these DCHECKs are bugs in blink and this
flag will help us solve them.

After fixing the most critical bugs we might want to remove the flag and just
keep the DCHECKs behind some "#if !defined(ENABLE_NPAPI_PLUGINS)" thing.

[reveman, 2013-04-16 01:12:06]

Committed patchset #6 manually as r194269 (presubmit successful).