layout-tests: Add flag for including compositingReasons in layerTreeAsText().

layout-tests: Add flag for including compositingReasons in layerTreeAsText().

Add internals.LAYER_TREE_INCLUDES_COMPOSITING_REASONS flag for including
compositingReasons in the output of internals.layerTreeAsText() (only the
short name, description is included with debug-info). This allows a layout
test to verify that a layer is being created for the right reasons.

BUG=436952
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195927

[sadrul, 2015-05-25 20:54:14]

sadrul@chromium.org changed reviewers:
+ esprehn@chromium.org, vollick@chromium.org

[esprehn, 2015-05-25 21:05:44]

lgtm but use String#includes.

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
File LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html (right):

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html:21:
assert_not_equals(docLayer.children[0].children[0].compositingReasons.indexOf("compositorProxy"),
-1);
assert_true(...compositingReasons.includes("compositorProxy"))

[sadrul, 2015-05-25 21:08:45]

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
File LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html (right):

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html:21:
assert_not_equals(docLayer.children[0].children[0].compositingReasons.indexOf("compositorProxy"),
-1);
On 2015/05/25 21:05:43, esprehn wrote:
> assert_true(...compositingReasons.includes("compositorProxy"))

compositingReasons is actually an array, and there doesn't appear to be an
includes() there :-(

[esprehn, 2015-05-25 21:13:13]

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
File LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html (right):

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html:21:
assert_not_equals(docLayer.children[0].children[0].compositingReasons.indexOf("compositorProxy"),
-1);
Oh then you want to use assert_in_array then.

[sadrul, 2015-05-25 21:19:32]

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
File LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html (right):

https://codereview.chromium.org/1153023002/diff/1/LayoutTests/fast/dom/Compos...
LayoutTests/fast/dom/CompositorProxy/proxy-forces-layer.html:21:
assert_not_equals(docLayer.children[0].children[0].compositingReasons.indexOf("compositorProxy"),
-1);
On 2015/05/25 21:13:13, esprehn wrote:
> Oh then you want to use assert_in_array then.

Nice! Done. Thanks! (I looked for something like assert_contains)

[sadrul, 2015-05-25 21:57:51]

There's a whole bunch of test-failures on the trybots because a lot of tests
seem to have expectations about the direct output of layerTreeAsText(). Do you
have any advice on how to address those? (e.g. should I update all these failing
tests manually in this CL? Should I introduce a flag to
internals.layerTreeAsText() which dictates whether or not to include
compositingReasons? etc.) Thanks!

[esprehn, 2015-05-25 23:11:00]

On 2015/05/25 at 21:57:51, sadrul wrote:
> There's a whole bunch of test-failures on the trybots because a lot of tests
seem to have expectations about the direct output of layerTreeAsText(). Do you
have any advice on how to address those? (e.g. should I update all these failing
tests manually in this CL? Should I introduce a flag to
internals.layerTreeAsText() which dictates whether or not to include
compositingReasons? etc.) Thanks!

Would those other tests benefit from having that information? Would they test
more stuff or just become more brittle?

You can rebaseline all the tests with NeedsRebaseline in the TestExpectations
file which will automatically update the tests. You can also introduce a flag if
you think having the new output in those new tests is not useful.

[sadrul, 2015-05-26 02:56:11]

On 2015/05/25 23:11:00, esprehn wrote:
> On 2015/05/25 at 21:57:51, sadrul wrote:
> > There's a whole bunch of test-failures on the trybots because a lot of tests
> seem to have expectations about the direct output of layerTreeAsText(). Do you
> have any advice on how to address those? (e.g. should I update all these
failing
> tests manually in this CL? Should I introduce a flag to
> internals.layerTreeAsText() which dictates whether or not to include
> compositingReasons? etc.) Thanks!
> 
> Would those other tests benefit from having that information? Would they test
> more stuff or just become more brittle?
> 
> You can rebaseline all the tests with NeedsRebaseline in the TestExpectations
> file which will automatically update the tests. You can also introduce a flag
if
> you think having the new output in those new tests is not useful.

I think it could be useful for some tests, but definitely not all of them. So I
have added a new flag, so that the tests can choose whether or not they care
about the compositing-reasons.

[Ian Vollick, 2015-05-26 03:18:16]

On 2015/05/26 at 02:56:11, sadrul wrote:
> On 2015/05/25 23:11:00, esprehn wrote:
> > On 2015/05/25 at 21:57:51, sadrul wrote:
> > > There's a whole bunch of test-failures on the trybots because a lot of
tests
> > seem to have expectations about the direct output of layerTreeAsText(). Do
you
> > have any advice on how to address those? (e.g. should I update all these
failing
> > tests manually in this CL? Should I introduce a flag to
> > internals.layerTreeAsText() which dictates whether or not to include
> > compositingReasons? etc.) Thanks!
> > 
> > Would those other tests benefit from having that information? Would they
test
> > more stuff or just become more brittle?
> > 
> > You can rebaseline all the tests with NeedsRebaseline in the
TestExpectations
> > file which will automatically update the tests. You can also introduce a
flag if
> > you think having the new output in those new tests is not useful.
> 
> I think it could be useful for some tests, but definitely not all of them. So
I have added a new flag, so that the tests can choose whether or not they care
about the compositing-reasons.

I like it much better with the flag, but I think the description now needs to be
updated.

[sadrul, 2015-05-26 03:26:38]

On 2015/05/26 03:18:16, vollick wrote:
> On 2015/05/26 at 02:56:11, sadrul wrote:
> > On 2015/05/25 23:11:00, esprehn wrote:
> > > On 2015/05/25 at 21:57:51, sadrul wrote:
> > > > There's a whole bunch of test-failures on the trybots because a lot of
> tests
> > > seem to have expectations about the direct output of layerTreeAsText(). Do
> you
> > > have any advice on how to address those? (e.g. should I update all these
> failing
> > > tests manually in this CL? Should I introduce a flag to
> > > internals.layerTreeAsText() which dictates whether or not to include
> > > compositingReasons? etc.) Thanks!
> > > 
> > > Would those other tests benefit from having that information? Would they
> test
> > > more stuff or just become more brittle?
> > > 
> > > You can rebaseline all the tests with NeedsRebaseline in the
> TestExpectations
> > > file which will automatically update the tests. You can also introduce a
> flag if
> > > you think having the new output in those new tests is not useful.
> > 
> > I think it could be useful for some tests, but definitely not all of them.
So
> I have added a new flag, so that the tests can choose whether or not they care
> about the compositing-reasons.
> 
> I like it much better with the flag, but I think the description now needs to
be
> updated.

Good catch. Updated the CL description.

[sadrul, 2015-05-26 22:04:08]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2015-05-26 22:04:08]

The patchset sent to the CQ was uploaded after l-g-t-m from esprehn@chromium.org
Link to the patchset: https://codereview.chromium.org/1153023002/#ps40001
(title: ".")

[commit-bot: I haz the power, 2015-05-26 22:04:45]

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

[commit-bot: I haz the power, 2015-05-26 22:08:34]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195927