[New Multicolumn] Correct paint rectangle calculation for compositing.

[New Multicolumn] Correct paint rectangle calculation for compositing.

This allows for enabling composited layers inside flow threads, since it's
not completely broken anymore.

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

[mstensho (USE GERRIT), 2014-10-03 14:15:16]

mstensho@opera.com changed reviewers:
+ abarth@chromium.org, jchaffraix@chromium.org

[Julien - ping for review, 2014-10-03 17:50:21]

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos-simple.html (right):

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos-simple.html:3: <div
style="transform:translateZ(0); width:3em; height:3em;
background:yellow;"></div>
In this test, the composited element is not in the flow-thread. Don't we want
one where it is actually in the flow-thread?

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos-simple.html:10: <div
style="position:relative; margin:10px; background:yellow;">PASS</div>
Do we need the position: relative here?

[mstensho (USE GERRIT), 2014-10-03 18:51:40]

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos-simple.html (right):

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos-simple.html:3: <div
style="transform:translateZ(0); width:3em; height:3em;
background:yellow;"></div>
On 2014/10/03 17:50:21, Julien Chaffraix - PST wrote:
> In this test, the composited element is not in the flow-thread. Don't we want
> one where it is actually in the flow-thread?

It seems to be what it takes to trigger compositing on stuff inside the
multicol. Is that a bug?

But sure, let's move it inside.

Done.

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos-simple.html:10: <div
style="position:relative; margin:10px; background:yellow;">PASS</div>
On 2014/10/03 17:50:21, Julien Chaffraix - PST wrote:
> Do we need the position: relative here?

Yes, then it becomes a layer, and it would fail to display without my changes in
RenderLayer.cpp (when allowing compositing on stuff inside flow threads in
RenderLayerCompositor::canBeComposited())

[mstensho (USE GERRIT), 2014-10-06 12:39:49]

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
File LayoutTests/fast/multicol/composited-relpos-simple.html (right):

https://codereview.chromium.org/625903004/diff/1/LayoutTests/fast/multicol/co...
LayoutTests/fast/multicol/composited-relpos-simple.html:10: <div
style="position:relative; margin:10px; background:yellow;">PASS</div>
The reason why this one gets composited is CompositingReasonAssumedOverlap.

CompositingReasonAssumedOverlap is set in
CompositingRequirementsUpdater::updateRecursive() because
currentRecursionData.m_subtreeIsCompositing is set. It's set because of the
translateZ'ed DIV earlier in the document.

I've changed the test case again now, so that the translateZ'ed box and the
relpos actually overlap, in case someone should decide to optimize this in the
future and not just assume that they overlap. That should hopefully make this
test a bit more robust.

[mstensho (USE GERRIT), 2014-10-06 14:52:59]

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1497:
rootLayer->convertToLayerCoords(enclosingPaginationLayer(),
offsetOfPaginationLayerFromRoot);
This isn't really going to work right now (unless you're in the first column).
convertToLayerCoords() gives you the visual offset in the multicol container,
not the flow thread single-strip logical coordinates, which I was sooo certain
that it did, that I didn't even bother to double-check.

[mstensho (USE GERRIT), 2014-10-06 14:55:23]

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1497:
rootLayer->convertToLayerCoords(enclosingPaginationLayer(),
offsetOfPaginationLayerFromRoot);
On 2014/10/06 14:52:58, mstensho wrote:
> This isn't really going to work right now (unless you're in the first column).
> convertToLayerCoords() gives you the visual offset in the multicol container,
> not the flow thread single-strip logical coordinates, which I was sooo certain
> that it did, that I didn't even bother to double-check.

Done.

With the latest patch, where I remove a hack from RenderLayer::location(), this
is now the right thing to do.

[Julien - ping for review, 2014-10-06 22:12:32]

lgtm

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1497:
rootLayer->convertToLayerCoords(enclosingPaginationLayer(),
offsetOfPaginationLayerFromRoot);
On 2014/10/06 14:55:22, mstensho wrote:
> On 2014/10/06 14:52:58, mstensho wrote:
> > This isn't really going to work right now (unless you're in the first
column).
> > convertToLayerCoords() gives you the visual offset in the multicol
container,
> > not the flow thread single-strip logical coordinates, which I was sooo
certain
> > that it did, that I didn't even bother to double-check.
> 
> Done.
> 
> With the latest patch, where I remove a hack from RenderLayer::location(),
this
> is now the right thing to do.

Could we add some visual testing for the second column case as the original
change broke it?

[mstensho (USE GERRIT), 2014-10-07 14:55:35]

@Julien, could you please take another look? Thanks to incorrect documentation
(some of it even written by a younger version of myself) and a generally
confusing environment with generous invalidation rectangles and pairs of bugs
canceling each other out, it turned out that my fix wasn't right.

My intention with this CL was to keep it simple and small, and provide the base
necessities to make compositing work inside of new multicol without breaking too
much. I have more CLs in the pipeline once this one has landed, for further
improvements.

I think this CL looks okay now, and that I'm on the right track, but feel free
to claim otherwise. :)

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/625903004/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1497:
rootLayer->convertToLayerCoords(enclosingPaginationLayer(),
offsetOfPaginationLayerFromRoot);
On 2014/10/06 22:12:32, Julien Chaffraix - PST wrote:
> On 2014/10/06 14:55:22, mstensho wrote:
> > On 2014/10/06 14:52:58, mstensho wrote:
> > > This isn't really going to work right now (unless you're in the first
> column).
> > > convertToLayerCoords() gives you the visual offset in the multicol
> container,
> > > not the flow thread single-strip logical coordinates, which I was sooo
> certain
> > > that it did, that I didn't even bother to double-check.
> > 
> > Done.
> > 
> > With the latest patch, where I remove a hack from RenderLayer::location(),
> this
> > is now the right thing to do.
> 
> Could we add some visual testing for the second column case as the original
> change broke it?

Done.

But then I had to fix even more. :-P It turned out that the documentation of
"dirty rect" was wrong, and therefore my fix was wrong too. Now it should be
right.

[andersr, 2014-10-07 15:51:20]

andersr@opera.com changed reviewers:
+ andersr@opera.com

[andersr, 2014-10-07 15:51:21]

https://codereview.chromium.org/625903004/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/625903004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:828: localPoint +=
parent()->renderer()->columnOffset(localPoint);
Composited RenderLayers now render at flow-thread coordinates, ignoring the
multicolness completely.

If that's somehow intended for new-multicol, then we should at least avoid
breaking old-multicol (further).

[mstensho (USE GERRIT), 2014-10-07 16:35:59]

https://codereview.chromium.org/625903004/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/625903004/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:828: localPoint +=
parent()->renderer()->columnOffset(localPoint);
On 2014/10/07 15:51:21, andersr wrote:
> Composited RenderLayers now render at flow-thread coordinates, ignoring the
> multicolness completely.
> 
> If that's somehow intended for new-multicol, then we should at least avoid
> breaking old-multicol (further).

Done.

[mstensho (USE GERRIT), 2014-10-08 20:57:57]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2014-10-08 20:58:58]

I'll just land this. It blocks all my compositing work.

[commit-bot: I haz the power, 2014-10-08 20:59:06]

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

[commit-bot: I haz the power, 2014-10-09 02:13:15]

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

[commit-bot: I haz the power, 2014-10-09 02:13:16]

Exceeded time limit waiting for builds to trigger.

[pdr., 2014-10-09 02:16:20]

On 2014/10/09 at 02:13:16, commit-bot wrote:
> Exceeded time limit waiting for builds to trigger.

(network issues unrelated to this patch. Re-trying...)

[pdr., 2014-10-09 02:16:32]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2014-10-09 02:19:47]

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

[commit-bot: I haz the power, 2014-10-09 04:26:06]

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

[commit-bot: I haz the power, 2014-10-09 04:26:07]

Exceeded time limit waiting for builds to trigger.

[mstensho (USE GERRIT), 2014-10-09 05:44:13]

The CQ bit was checked by mstensho@opera.com

[commit-bot: I haz the power, 2014-10-09 05:44:53]

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

[commit-bot: I haz the power, 2014-10-09 06:32:13]

Committed patchset #6 (id:100001) as 183437