[New Multicolumn] Render transformed layers into multiple fragments.

[New Multicolumn] Render transformed layers into multiple fragments.

When composited, a layer is transformed on the graphics side, so the best thing
we can do is to carry on and just paint in all columns (without applying the
transform on our own). collectLayerFragments() used to refuse to collect a
fragment for each column involved if the layer was transformed. This is only
correct when not compositing, because then we apply the transform in software
for each column.

Note that the end result won't always be correct for transformed composited
layers. Column gap clipping may be wrong, since the gaps get transformed along
with the layer (also a problem prior to this CL). Furthermore, each fragment
should ideally be transformed separately, in order to use a different origin
for each fragment. But at least we won't lose all content outside the first
fragment, like we used to.

Most of the tests are based on work by andersr@opera.com.

BUG=359877
R=chrishtr@chromium.org

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

[chrishtr, 2015-01-15 18:49:47]

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:381:
paintFragmentByApplyingTransform(context, paintingInfo, paintFlags,
fragment.paginationOffset);
It looks like a bunch of the complexity in this CL is in re-creating the
fragment under the hood from the partial data sent through this function. Isn't
there a cleaner way to pass the fragment you already have
at this point down through the relevant interfaces?

[mstensho (USE GERRIT), 2015-01-15 20:36:48]

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:381:
paintFragmentByApplyingTransform(context, paintingInfo, paintFlags,
fragment.paginationOffset);
On 2015/01/15 18:49:47, chrishtr wrote:
> It looks like a bunch of the complexity in this CL is in re-creating the
> fragment under the hood from the partial data sent through this function.
Isn't
> there a cleaner way to pass the fragment you already have
> at this point down through the relevant interfaces?

Sorry, I'm not sure if I understand. This CL isn't about changing the behavior
in this part of the code (non-composited transforms) (although the CL is
certainly about NOT messing up things for this part of the code).

I'm only interested in being able to cancel out the single-fragment forcing,
which this code still needs. Single-fragment forcing cannot be used when
paintLayerContents() (and not paintLayer(), which gets bypassed) is called
directly from CompositedLayerMapping::doPaintTask(). That's what I'm fixing.

Or did you mean that we should change non-composited transforms (this code) to
set up only one transform (instead of one per fragment) for the entire layer, so
that it gets equally borked, compared to when compositing? Then we'd get the
problems pointed out in the commit message also when not compositing transforms
in multicol. Then again, none of the renderings (composited vs. non-composited)
are really correct. The transform's origin is wrong (although differently wrong)
in both cases. Column clipping works fine only when not compositing, though.

Correct rendering of transforms in multicol (or paged) is an unsolved mystery. I
hope it will be possible to get it right with slimming paint & co. The thing is
that if we decide that we need graphics layers for a transform inside multicol
(which is today's core/rendering/compositing/ stuff, which is going to be
removed, as far as I understand), I think we should ideally create a separate
layer for each column, so that they get separate transform origins, and get
clipped correctly against the column gaps. I think the code right here
(non-composited) is closer to that goal than what can be achieved with the
current compositing code involved. This makes me reluctant to break it.

Try this in Firefox; they get it right (Blink doesn't; not when composting and
not when not compositing):

<div style="margin:10em auto; width:30em; -webkit-columns:2; columns:2;
-moz-columns:2; line-height:2em; height:4em; column-fill:auto;
-moz-column-fill:auto; background:yellow;">
    first column<br>
    <div style="transform:rotate(45deg);">
        first column<br>
        second column<br>
    </div>
    second column<br>
</div>

All I really want to achieve with this CL is to not unconditionally lose all
transformed content in all columns but the initial one, which is part of fixing
issue 359877 as well as possible. But if you think the gain is too little to
bother with the extra code complexity, maybe I should just drop this CL?

[chrishtr, 2015-01-15 23:33:45]

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:402: paintLayerContentsAndReflection(context,
transformedPaintingInfo, paintFlags, ForceSingleFragment);
What I mean is that this call stack appears to be present:

paintLayerWithTransform
  -> paintFragmentByApplyingTransform
    -> paintLayerContentsAndReflection
      -> paintLayerContents

paintLayerContents then either collects fragments or collects one fragment based
on the fragment forcing enum parameter.
But paintLayerWithTransform already collected the fragments and is iterating
over them. Why shouldn't the code just pass down the fragment and avoid
recomputing it?

[mstensho (USE GERRIT), 2015-02-03 13:55:01]

Sorry about the delay. I had to put this on hold, since we had jchaffraix
visiting here, and we had so much to discuss and work on. :)

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
File Source/core/paint/LayerPainter.cpp (right):

https://codereview.chromium.org/801053005/diff/1/Source/core/paint/LayerPaint...
Source/core/paint/LayerPainter.cpp:402: paintLayerContentsAndReflection(context,
transformedPaintingInfo, paintFlags, ForceSingleFragment);
On 2015/01/15 23:33:45, chrishtr wrote:
> What I mean is that this call stack appears to be present:
> 
> paintLayerWithTransform
>   -> paintFragmentByApplyingTransform
>     -> paintLayerContentsAndReflection
>       -> paintLayerContents
> 
> paintLayerContents then either collects fragments or collects one fragment
based
> on the fragment forcing enum parameter.
> But paintLayerWithTransform already collected the fragments and is iterating
> over them. Why shouldn't the code just pass down the fragment and avoid
> recomputing it?

paintLayerWithTransform() collects fragment(s) for the pagination layer (the
flow thread), to set up column clipping and column translation (and to figure
out which columns we need to visit). paintLayerContents(), on the other hand,
collects fragment(s) for the layer itself (to set up clipping established by the
layer itself (overflow:hidden, for instance).

I really believe this needs to be done in separate steps. Column clipping needs
to be set before transforming (because the transform is somewhere *inside* the
column contents, and the columns shouldn't be affected by the transform), and
layer clipping is set after transforming the layer.

We could certainly calculate the layer fragment in
paintFragmentByApplyingTransform() and pass it down to paintLayerContents(),
instead of saying ForceSingleFragment and deal with it when we get there, but I
really don't think that would be cleaner. That would mean yet another place
where we "collect" fragments.

[chrishtr, 2015-02-07 00:02:45]

lgtm

[mstensho (USE GERRIT), 2015-02-08 07:56:19]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-02-08 07:57:20]

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

[commit-bot: I haz the power, 2015-02-08 08:02:51]

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

[commit-bot: I haz the power, 2015-02-08 08:02:52]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/2...)

[mstensho (USE GERRIT), 2015-02-08 09:31:58]

New patchsets have been uploaded after l-g-t-m from chrishtr@chromium.org

[mstensho (USE GERRIT), 2015-02-08 09:32:19]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2015-02-08 09:33:09]

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

[commit-bot: I haz the power, 2015-02-08 11:56:49]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189754