Add test for paint underinvalidation fix with animated transform overflow.

Add test for paint underinvalidation fix with animated transform overflow.

I landed a quick fix to merge in r452170 but omitted a test. This patch adds a
test for that bug fix.

BUG=683814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2713773003
Cr-Commit-Position: refs/heads/master@{#453692}
Committed: https://chromium.googlesource.com/chromium/src/+/24a657a98b6352e7a3b4a4c121b2398d6d42c3be

[bokan, 2017-02-23 18:05:03]

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

[bokan, 2017-02-23 18:05:04]

Hey pdr,

Here's a test for that fix I landed. You originally wanted to avoid the
computeScrollbarExistence below if visualViewportSuppliesScrollbars but I'm not
sure about that anymore. I'm worried about transition cases where we go from
!vVSS to vVSS and not properly recalculating geometry and layouts.
computeScrollbarExistence already has a correct early-out for the
visualViewportSuppliedScrollbar case so I think this is fine. WDYT?

[bokan, 2017-02-23 18:10:16]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-23 18:10:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-02-23 18:24:33]

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp (right):

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
invalidationArrayContains(JSONArray* invalidations, LayoutObject& object) {
This unittest is basically equivalent to a layout test, but I don't think we
need to go all the way to invalidation rects. Can you DCHECK that the layout
view item does not need paint invalidation before the animation, then DCHECK
that it does afterwards? This must have taken a long time to write so I feel a
little bad about deleting it :(

[bokan, 2017-02-23 18:41:17]

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp (right):

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
invalidationArrayContains(JSONArray* invalidations, LayoutObject& object) {
On 2017/02/23 18:24:33, pdr. wrote:
> This unittest is basically equivalent to a layout test, but I don't think we
> need to go all the way to invalidation rects. Can you DCHECK that the layout
> view item does not need paint invalidation before the animation, then DCHECK
> that it does afterwards? This must have taken a long time to write so I feel a
> little bad about deleting it :(

I don't mind deleting it if I can (I feel bad having yet another place checking
invalidation) but I don't think we can do it though a layout test reliably as it
relies on checking the invalidation mid-animation which is hard to do in a
non-flaky way with layout tests. Worse, it requires turning on the viewport
enabled setting which is a dubious thing to do from JS as the page is already
loaded. We could run this in a virtual test suite but that seems like overkill
and we still have the animation induced in-determinism.

I also can't DCHECK for needs invalidation since the beginFrame both ticks the
animation and then does a full lifecycle update so once we're back in test code
the invalidation is already cleared.

PS: I think this is superior to the layout test since it's less brittle. The
layout test guards against any changes in invalidation which can be problematic
when new features/optimizations cause additional/fewer unrelated invalidations.
I've had this happen before where a new invalidation caused me to have to rebase
a bunch of unrelated tests. By contrast, this only checks that we invalidated
the layout view in some way -- regardless of ordering and number of times.

[pdr., 2017-02-23 19:42:08]

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp (right):

https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
invalidationArrayContains(JSONArray* invalidations, LayoutObject& object) {
On 2017/02/23 at 18:41:17, bokan wrote:
> On 2017/02/23 18:24:33, pdr. wrote:
> > This unittest is basically equivalent to a layout test, but I don't think we
> > need to go all the way to invalidation rects. Can you DCHECK that the layout
> > view item does not need paint invalidation before the animation, then DCHECK
> > that it does afterwards? This must have taken a long time to write so I feel
a
> > little bad about deleting it :(
> 
> I don't mind deleting it if I can (I feel bad having yet another place
checking invalidation) but I don't think we can do it though a layout test
reliably as it relies on checking the invalidation mid-animation which is hard
to do in a non-flaky way with layout tests. Worse, it requires turning on the
viewport enabled setting which is a dubious thing to do from JS as the page is
already loaded. We could run this in a virtual test suite but that seems like
overkill and we still have the animation induced in-determinism.
> 
> I also can't DCHECK for needs invalidation since the beginFrame both ticks the
animation and then does a full lifecycle update so once we're back in test code
the invalidation is already cleared.
> 
> PS: I think this is superior to the layout test since it's less brittle. The
layout test guards against any changes in invalidation which can be problematic
when new features/optimizations cause additional/fewer unrelated invalidations.
I've had this happen before where a new invalidation caused me to have to rebase
a bunch of unrelated tests. By contrast, this only checks that we invalidated
the layout view in some way -- regardless of ordering and number of times.

Do we need to run the full lifecycle though? In other words, can you do
something like:
1) dcheck that the view does not yet need paint invalidation
2) update lifecycle to style clean
3) dcheck that the view does need paint invalidation

[commit-bot: I haz the power, 2017-02-23 19:46:45]

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

[commit-bot: I haz the power, 2017-02-23 19:46:48]

Dry run: This issue passed the CQ dry run.

[bokan, 2017-02-23 19:55:44]

On 2017/02/23 19:42:08, pdr. wrote:
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp (right):
> 
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
> invalidationArrayContains(JSONArray* invalidations, LayoutObject& object) {
> On 2017/02/23 at 18:41:17, bokan wrote:
> > On 2017/02/23 18:24:33, pdr. wrote:
> > > This unittest is basically equivalent to a layout test, but I don't think
we
> > > need to go all the way to invalidation rects. Can you DCHECK that the
layout
> > > view item does not need paint invalidation before the animation, then
DCHECK
> > > that it does afterwards? This must have taken a long time to write so I
feel
> a
> > > little bad about deleting it :(
> > 
> > I don't mind deleting it if I can (I feel bad having yet another place
> checking invalidation) but I don't think we can do it though a layout test
> reliably as it relies on checking the invalidation mid-animation which is hard
> to do in a non-flaky way with layout tests. Worse, it requires turning on the
> viewport enabled setting which is a dubious thing to do from JS as the page is
> already loaded. We could run this in a virtual test suite but that seems like
> overkill and we still have the animation induced in-determinism.
> > 
> > I also can't DCHECK for needs invalidation since the beginFrame both ticks
the
> animation and then does a full lifecycle update so once we're back in test
code
> the invalidation is already cleared.
> > 
> > PS: I think this is superior to the layout test since it's less brittle. The
> layout test guards against any changes in invalidation which can be
problematic
> when new features/optimizations cause additional/fewer unrelated
invalidations.
> I've had this happen before where a new invalidation caused me to have to
rebase
> a bunch of unrelated tests. By contrast, this only checks that we invalidated
> the layout view in some way -- regardless of ordering and number of times.
> 
> Do we need to run the full lifecycle though? In other words, can you do
> something like:
> 1) dcheck that the view does not yet need paint invalidation
> 2) update lifecycle to style clean
> 3) dcheck that the view does need paint invalidation

The lifecycle update occurs inside SimCompositor::beginFrame which updates the
lifecycle and does a paint. I could break this up and manually kick off an
animation frame without the lifecycle update and paint but I feel like that's
worse for maintainability and realistic simulation than just tracking paint
invalidation, no? Is there a reason to avoid the tracking?

[pdr., 2017-02-23 20:12:38]

On 2017/02/23 at 19:55:44, bokan wrote:
> On 2017/02/23 19:42:08, pdr. wrote:
> >
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp (right):
> > 
> >
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
> > invalidationArrayContains(JSONArray* invalidations, LayoutObject& object) {
> > On 2017/02/23 at 18:41:17, bokan wrote:
> > > On 2017/02/23 18:24:33, pdr. wrote:
> > > > This unittest is basically equivalent to a layout test, but I don't
think we
> > > > need to go all the way to invalidation rects. Can you DCHECK that the
layout
> > > > view item does not need paint invalidation before the animation, then
DCHECK
> > > > that it does afterwards? This must have taken a long time to write so I
feel
> > a
> > > > little bad about deleting it :(
> > > 
> > > I don't mind deleting it if I can (I feel bad having yet another place
> > checking invalidation) but I don't think we can do it though a layout test
> > reliably as it relies on checking the invalidation mid-animation which is
hard
> > to do in a non-flaky way with layout tests. Worse, it requires turning on
the
> > viewport enabled setting which is a dubious thing to do from JS as the page
is
> > already loaded. We could run this in a virtual test suite but that seems
like
> > overkill and we still have the animation induced in-determinism.
> > > 
> > > I also can't DCHECK for needs invalidation since the beginFrame both ticks
the
> > animation and then does a full lifecycle update so once we're back in test
code
> > the invalidation is already cleared.
> > > 
> > > PS: I think this is superior to the layout test since it's less brittle.
The
> > layout test guards against any changes in invalidation which can be
problematic
> > when new features/optimizations cause additional/fewer unrelated
invalidations.
> > I've had this happen before where a new invalidation caused me to have to
rebase
> > a bunch of unrelated tests. By contrast, this only checks that we
invalidated
> > the layout view in some way -- regardless of ordering and number of times.
> > 
> > Do we need to run the full lifecycle though? In other words, can you do
> > something like:
> > 1) dcheck that the view does not yet need paint invalidation
> > 2) update lifecycle to style clean
> > 3) dcheck that the view does need paint invalidation
> 
> The lifecycle update occurs inside SimCompositor::beginFrame which updates the
lifecycle and does a paint. I could break this up and manually kick off an
animation frame without the lifecycle update and paint but I feel like that's
worse for maintainability and realistic simulation than just tracking paint
invalidation, no? Is there a reason to avoid the tracking?

It's just a lot of complexity to have to run a full integration test producing
json representing an internal structure, then parse the json, when we really
just want to directly check the invalidation bit.

Do we even need to call beginFrame?

For example, could we use something similar to PaintPropertyTreeUpdateTest's
NoPaintPropertyUpdateOnBackgroundChange test?

[bokan, 2017-02-23 20:31:10]

On 2017/02/23 20:12:38, pdr. wrote:
> On 2017/02/23 at 19:55:44, bokan wrote:
> > On 2017/02/23 19:42:08, pdr. wrote:
> > >
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > > File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp
(right):
> > > 
> > >
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > > third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
> > > invalidationArrayContains(JSONArray* invalidations, LayoutObject& object)
{
> > > On 2017/02/23 at 18:41:17, bokan wrote:
> > > > On 2017/02/23 18:24:33, pdr. wrote:
> > > > > This unittest is basically equivalent to a layout test, but I don't
> think we
> > > > > need to go all the way to invalidation rects. Can you DCHECK that the
> layout
> > > > > view item does not need paint invalidation before the animation, then
> DCHECK
> > > > > that it does afterwards? This must have taken a long time to write so
I
> feel
> > > a
> > > > > little bad about deleting it :(
> > > > 
> > > > I don't mind deleting it if I can (I feel bad having yet another place
> > > checking invalidation) but I don't think we can do it though a layout test
> > > reliably as it relies on checking the invalidation mid-animation which is
> hard
> > > to do in a non-flaky way with layout tests. Worse, it requires turning on
> the
> > > viewport enabled setting which is a dubious thing to do from JS as the
page
> is
> > > already loaded. We could run this in a virtual test suite but that seems
> like
> > > overkill and we still have the animation induced in-determinism.
> > > > 
> > > > I also can't DCHECK for needs invalidation since the beginFrame both
ticks
> the
> > > animation and then does a full lifecycle update so once we're back in test
> code
> > > the invalidation is already cleared.
> > > > 
> > > > PS: I think this is superior to the layout test since it's less brittle.
> The
> > > layout test guards against any changes in invalidation which can be
> problematic
> > > when new features/optimizations cause additional/fewer unrelated
> invalidations.
> > > I've had this happen before where a new invalidation caused me to have to
> rebase
> > > a bunch of unrelated tests. By contrast, this only checks that we
> invalidated
> > > the layout view in some way -- regardless of ordering and number of times.
> > > 
> > > Do we need to run the full lifecycle though? In other words, can you do
> > > something like:
> > > 1) dcheck that the view does not yet need paint invalidation
> > > 2) update lifecycle to style clean
> > > 3) dcheck that the view does need paint invalidation
> > 
> > The lifecycle update occurs inside SimCompositor::beginFrame which updates
the
> lifecycle and does a paint. I could break this up and manually kick off an
> animation frame without the lifecycle update and paint but I feel like that's
> worse for maintainability and realistic simulation than just tracking paint
> invalidation, no? Is there a reason to avoid the tracking?
> 
> It's just a lot of complexity to have to run a full integration test producing
> json representing an internal structure, then parse the json, when we really
> just want to directly check the invalidation bit.
> 
> Do we even need to call beginFrame?
> 
> For example, could we use something similar to PaintPropertyTreeUpdateTest's
> NoPaintPropertyUpdateOnBackgroundChange test?

Yah, the reason for the beginFrame is that I need to tick the animation system
forward in a deterministic way.

[bokan, 2017-02-23 20:41:59]

On 2017/02/23 20:31:10, bokan wrote:
> On 2017/02/23 20:12:38, pdr. wrote:
> > On 2017/02/23 at 19:55:44, bokan wrote:
> > > On 2017/02/23 19:42:08, pdr. wrote:
> > > >
> >
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > > > File third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp
> (right):
> > > > 
> > > >
> >
>
https://codereview.chromium.org/2713773003/diff/1/third_party/WebKit/Source/w...
> > > > third_party/WebKit/Source/web/tests/PaintInvalidationTest.cpp:34: bool
> > > > invalidationArrayContains(JSONArray* invalidations, LayoutObject&
object)
> {
> > > > On 2017/02/23 at 18:41:17, bokan wrote:
> > > > > On 2017/02/23 18:24:33, pdr. wrote:
> > > > > > This unittest is basically equivalent to a layout test, but I don't
> > think we
> > > > > > need to go all the way to invalidation rects. Can you DCHECK that
the
> > layout
> > > > > > view item does not need paint invalidation before the animation,
then
> > DCHECK
> > > > > > that it does afterwards? This must have taken a long time to write
so
> I
> > feel
> > > > a
> > > > > > little bad about deleting it :(
> > > > > 
> > > > > I don't mind deleting it if I can (I feel bad having yet another place
> > > > checking invalidation) but I don't think we can do it though a layout
test
> > > > reliably as it relies on checking the invalidation mid-animation which
is
> > hard
> > > > to do in a non-flaky way with layout tests. Worse, it requires turning
on
> > the
> > > > viewport enabled setting which is a dubious thing to do from JS as the
> page
> > is
> > > > already loaded. We could run this in a virtual test suite but that seems
> > like
> > > > overkill and we still have the animation induced in-determinism.
> > > > > 
> > > > > I also can't DCHECK for needs invalidation since the beginFrame both
> ticks
> > the
> > > > animation and then does a full lifecycle update so once we're back in
test
> > code
> > > > the invalidation is already cleared.
> > > > > 
> > > > > PS: I think this is superior to the layout test since it's less
brittle.
> > The
> > > > layout test guards against any changes in invalidation which can be
> > problematic
> > > > when new features/optimizations cause additional/fewer unrelated
> > invalidations.
> > > > I've had this happen before where a new invalidation caused me to have
to
> > rebase
> > > > a bunch of unrelated tests. By contrast, this only checks that we
> > invalidated
> > > > the layout view in some way -- regardless of ordering and number of
times.
> > > > 
> > > > Do we need to run the full lifecycle though? In other words, can you do
> > > > something like:
> > > > 1) dcheck that the view does not yet need paint invalidation
> > > > 2) update lifecycle to style clean
> > > > 3) dcheck that the view does need paint invalidation
> > > 
> > > The lifecycle update occurs inside SimCompositor::beginFrame which updates
> the
> > lifecycle and does a paint. I could break this up and manually kick off an
> > animation frame without the lifecycle update and paint but I feel like
that's
> > worse for maintainability and realistic simulation than just tracking paint
> > invalidation, no? Is there a reason to avoid the tracking?
> > 
> > It's just a lot of complexity to have to run a full integration test
producing
> > json representing an internal structure, then parse the json, when we really
> > just want to directly check the invalidation bit.
> > 
> > Do we even need to call beginFrame?
> > 
> > For example, could we use something similar to PaintPropertyTreeUpdateTest's
> > NoPaintPropertyUpdateOnBackgroundChange test?
> 
> Yah, the reason for the beginFrame is that I need to tick the animation system
> forward in a deterministic way.

Bah, accidentally sent before I finished typing.

I don't think the test you pointed out can help in this case because of
dependency on animation. We generally haven't had a good way to test animations
in Blink deterministically and we've been pointed to do something like this by
esprehn@ so I think this kind of test will become more common soon. Given that,
I prefer ticking beginFrame like this to manually poking at Blink internals to
setup the proper state.

The JSON parse is a bit unfortunate but it's not that complex. We already do
this kind of invalidation tracking for other tests (this is essentially
mimicking the paint invalidation layout tests) so I'm not sure I see the
downside. I could just compare the JSON output to an expected JSON string which
would be the same thing the layout test is doing. I guess what I'm saying is
that I actually want the full integration-style test we're doing in the
paint-invalidation layout tests but that layout tests aren't flexible enough to
setup the state of Blink needed for this test.

[pdr., 2017-02-23 22:08:18]

On 2017/02/23 at 20:41:59, bokan wrote:
> I don't think the test you pointed out can help in this case because of
dependency on animation. We generally haven't had a good way to test animations
in Blink deterministically and we've been pointed to do something like this by
esprehn@ so I think this kind of test will become more common soon. Given that,
I prefer ticking beginFrame like this to manually poking at Blink internals to
setup the proper state.

Why is there a dependency on animation? In other words, why isn't this a general
bug of anyone calling recalcOverflowAfterStyleChange?

I think this may resolve whether an integration test is needed over a smaller
unit test. The paint invalidation system seems to be working properly before and
after your change, so we don't need to check the output of paint invalidation
any more than we need to check pixels. This is why I am arguing for a
finer-grained unit test. On the other hand, if your bug only triggers through
the interaction of animation and paint invalidation, an integration test (as
you've written) does sound appropriate.

[bokan, 2017-02-23 22:23:11]

On 2017/02/23 22:08:18, pdr. wrote:
> On 2017/02/23 at 20:41:59, bokan wrote:
> > I don't think the test you pointed out can help in this case because of
> dependency on animation. We generally haven't had a good way to test
animations
> in Blink deterministically and we've been pointed to do something like this by
> esprehn@ so I think this kind of test will become more common soon. Given
that,
> I prefer ticking beginFrame like this to manually poking at Blink internals to
> setup the proper state.
> 
> Why is there a dependency on animation? In other words, why isn't this a
general
> bug of anyone calling recalcOverflowAfterStyleChange?
> 
> I think this may resolve whether an integration test is needed over a smaller
> unit test. The paint invalidation system seems to be working properly before
and
> after your change, so we don't need to check the output of paint invalidation
> any more than we need to check pixels. This is why I am arguing for a
> finer-grained unit test. On the other hand, if your bug only triggers through
> the interaction of animation and paint invalidation, an integration test (as
> you've written) does sound appropriate.

Ah, ok, I think I see what you're saying. Let me try that and circle back.

[bokan, 2017-02-27 22:04:43]

Ok, I think this test is more along the lines you were thinking of. WDYT?

Do you know of a better place to put a unit test like this or is this fine?

[pdr., 2017-02-27 22:41:14]

On 2017/02/27 at 22:04:43, bokan wrote:
> Ok, I think this test is more along the lines you were thinking of. WDYT?

Thank you! This is exactly what I was thinking.

> 
> Do you know of a better place to put a unit test like this or is this fine?

We don't really need the full SimTest infrastructure so we could add something
like "core/paint/PaintInvalidatorTest.cpp" and copy how
BoxPaintInvalidatorTest.cpp overrides the settings (i.e., with
ScopedViewportSettingForTest or something). This would avoid needing to
CORE_EXPORT the style classes.

[bokan, 2017-02-28 15:02:05]

Description was changed from

==========
Add test for paint underinvalidation fix with animated transform overflow.

I landed a quick fix to merge in r452170 but omitted a test. This patch adds a
test for that bug fix.

BUG=683814
==========

to

==========
Add test for paint underinvalidation fix with animated transform overflow.

I landed a quick fix to merge in r452170 but omitted a test. This patch adds a
test for that bug fix.

BUG=683814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[bokan, 2017-02-28 15:03:40]

Test moved into Source/core, ptal.

[bokan, 2017-02-28 15:54:05]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 15:54:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 16:09:31]

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

[commit-bot: I haz the power, 2017-02-28 16:09:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2017-02-28 16:49:08]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-28 16:49:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 17:05:03]

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

[commit-bot: I haz the power, 2017-02-28 17:05:04]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[bokan, 2017-02-28 17:35:04]

On 2017/02/28 15:03:40, bokan wrote:
> Test moved into Source/core, ptal.

I'm still getting linker errors against StylePath and StyleTransformData. Do you
know why that is? I thought the core unit tests should have access to all the
symbols in Source/core?

[pdr., 2017-02-28 17:36:17]

LGTM! Thank you for driving this home

https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp (right):

https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
Supernit: 2017

[pdr., 2017-02-28 17:36:34]

On 2017/02/28 at 17:36:17, pdr. wrote:
> LGTM! Thank you for driving this home
> 
>
https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp (right):
> 
>
https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
> Supernit: 2017

[pdr., 2017-02-28 17:38:34]

On 2017/02/28 at 17:35:04, bokan wrote:
> On 2017/02/28 15:03:40, bokan wrote:
> > Test moved into Source/core, ptal.
> 
> I'm still getting linker errors against StylePath and StyleTransformData. Do
you know why that is? I thought the core unit tests should have access to all
the symbols in Source/core?

Hmm, we may have to core export them after all? Another option is to use the
approach in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
and just change the class name from one style to another, to avoid needing to
directly modify the style bits.

[pdr., 2017-02-28 17:39:38]

On 2017/02/28 at 17:38:34, pdr. wrote:
> On 2017/02/28 at 17:35:04, bokan wrote:
> > On 2017/02/28 15:03:40, bokan wrote:
> > > Test moved into Source/core, ptal.
> > 
> > I'm still getting linker errors against StylePath and StyleTransformData. Do
you know why that is? I thought the core unit tests should have access to all
the symbols in Source/core?
> 
> Hmm, we may have to core export them after all? Another option is to use the
approach in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
and just change the class name from one style to another, to avoid needing to
directly modify the style bits.

(This test changes style, another example that changes className is
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...)

[bokan, 2017-02-28 18:31:15]

On 2017/02/28 17:38:34, pdr. wrote:
> On 2017/02/28 at 17:35:04, bokan wrote:
> > On 2017/02/28 15:03:40, bokan wrote:
> > > Test moved into Source/core, ptal.
> > 
> > I'm still getting linker errors against StylePath and StyleTransformData. Do
> you know why that is? I thought the core unit tests should have access to all
> the symbols in Source/core?
> 
> Hmm, we may have to core export them after all? Another option is to use the
> approach in
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Pai...
> and just change the class name from one style to another, to avoid needing to
> directly modify the style bits.

Thanks, that's much better.

[bokan, 2017-02-28 18:31:22]

https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp (right):

https://codereview.chromium.org/2713773003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintInvalidationTest.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
On 2017/02/28 17:36:17, pdr. wrote:
> Supernit: 2017

Heh, living in the past. Done.

[bokan, 2017-02-28 18:31:52]

The CQ bit was checked by bokan@chromium.org

[bokan, 2017-02-28 18:31:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/2713773003/#ps100001
(title: "Set style attribute in test")

[commit-bot: I haz the power, 2017-02-28 18:32:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-28 20:45:33]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488306712039640,
"parent_rev": "540cb81483a42eb8a5754547a5700f3c04d71241", "commit_rev":
"24a657a98b6352e7a3b4a4c121b2398d6d42c3be"}

[commit-bot: I haz the power, 2017-02-28 20:46:11]

Description was changed from

==========
Add test for paint underinvalidation fix with animated transform overflow.

I landed a quick fix to merge in r452170 but omitted a test. This patch adds a
test for that bug fix.

BUG=683814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add test for paint underinvalidation fix with animated transform overflow.

I landed a quick fix to merge in r452170 but omitted a test. This patch adds a
test for that bug fix.

BUG=683814
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2713773003
Cr-Commit-Position: refs/heads/master@{#453692}
Committed:
https://chromium.googlesource.com/chromium/src/+/24a657a98b6352e7a3b4a4c121b2...
==========

[commit-bot: I haz the power, 2017-02-28 20:46:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/24a657a98b6352e7a3b4a4c121b2...