[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html

Committed: https://crrev.com/5897d3e9488dc8596a646fd931ac7e32ec3cb6e3
Cr-Commit-Position: refs/heads/master@{#380558}

[trchen, 2016-03-10 05:53:25]

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

[commit-bot: I haz the power, 2016-03-10 05:53:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782713003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782713003/1

[trchen, 2016-03-10 05:54:08]

trchen@chromium.org changed reviewers:
+ jbroman@chromium.org

[commit-bot: I haz the power, 2016-03-10 06:39:22]

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

[commit-bot: I haz the power, 2016-03-10 06:39:23]

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

[pdr., 2016-03-10 21:10:12]

On 2016/03/10 at 06:39:23, commit-bot wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

These failures look real :/ I suspect this fix is correct though.

[pdr., 2016-03-10 21:10:18]

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

[trchen, 2016-03-10 22:59:15]

On 2016/03/10 21:10:12, pdr wrote:
> On 2016/03/10 at 06:39:23, commit-bot wrote:
> > Dry run: Try jobs failed on following builders:
> >   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> These failures look real :/ I suspect this fix is correct though.

I didn't check the numbers but I checked the rendered pixels. :)
I suspect we simply copy & pasted in the unit test expectations. Let me double
check the numbers.

[trchen, 2016-03-10 23:06:36]

On 2016/03/10 22:59:15, trchen wrote:
> On 2016/03/10 21:10:12, pdr wrote:
> > On 2016/03/10 at 06:39:23, commit-bot wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > These failures look real :/ I suspect this fix is correct though.
> 
> I didn't check the numbers but I checked the rendered pixels. :)
> I suspect we simply copy & pasted in the unit test expectations. Let me double
> check the numbers.

I'm convinced that the unit test looks correct. Something sketchy is going on...

[trchen, 2016-03-11 02:08:49]

Description was changed from

==========
[SPv2] Reverse PaintArtifactCompositor matrix accumulation order

The old order was reversed. We found it as we pull in more non-trivial tests.

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html
==========

to

==========
[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html
==========

[trchen, 2016-03-11 02:09:55]

This time I'm pretty sure we caught the culprit. :)

[trchen, 2016-03-11 02:10:03]

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

[commit-bot: I haz the power, 2016-03-11 02:10:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1782713003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1782713003/20001

[commit-bot: I haz the power, 2016-03-11 02:18:24]

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

[commit-bot: I haz the power, 2016-03-11 02:18:25]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[pdr., 2016-03-11 03:53:22]

LGTM

I'm surprised there's no preTranslate on gfx::Transform. I grepped around and
only found one other place it might be used (windowanimations.cc) so maybe it's
just not common.

[trchen, 2016-03-11 05:38:25]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-03-11 05:39:07]

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

[commit-bot: I haz the power, 2016-03-11 06:41:18]

Description was changed from

==========
[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html
==========

to

==========
[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html
==========

[commit-bot: I haz the power, 2016-03-11 06:41:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-11 06:42:59]

Description was changed from

==========
[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html
==========

to

==========
[SPv2] Reverse PaintArtifactCompositor clip layer origin undo order

As cc::Layer doesn't support shifted layer bounds, we bake the layer origin
into transform and paint with an offset. When adding a child layer to a parent
with such baked-in origin, we need to offset it first.

Before this change the combined matrix looked like:
[ transform ] * [ -parent layer origin ] * [ layer origin ]
When it should be:
[ -parent layer origin ] * [ transform ] * [ layer origin ]

TEST=virtual/spv2/fast/overflow/overflow-update-transform.html

Committed: https://crrev.com/5897d3e9488dc8596a646fd931ac7e32ec3cb6e3
Cr-Commit-Position: refs/heads/master@{#380558}
==========

[commit-bot: I haz the power, 2016-03-11 06:43:00]

Patchset 2 (id:??) landed as
https://crrev.com/5897d3e9488dc8596a646fd931ac7e32ec3cb6e3
Cr-Commit-Position: refs/heads/master@{#380558}

[jbroman, 2016-03-11 18:54:42]

Ah, that makes much more sense than the original CL, as I vividly remember
thinking about and unit testing the order of those transforms.

This I believe is an issue, but I would have liked to see a unit test in this CL
as well.