[SPv2] Respect transform-style in the Blink and cc property trees.

[SPv2] Respect transform-style in the Blink and cc property trees.

This allows the creation of 3D rendering contexts (thus 3D sorting), and of
setting the bits controlling transform flattening, according to the
CSS transform-style.

BUG=563667
Committed: https://crrev.com/cc109564b3b1277f76d0d022c5be5560b9c239be
Cr-Commit-Position: refs/heads/master@{#407381}

[jbroman, 2016-07-11 22:18:42]

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

[trchen, 2016-07-11 23:45:45]

Counter-example: http://jsbin.com/wikehihunu/
A flat element with perspective applies perspective to each children (which can
create separate 3D context).

Counter-example 2: http://jsbin.com/moqayilumo/
It is unclear what should happen when containing block penetrates stacking
context.

Both cases involve a situation that we want to flatten a group of planes into a
stacking context without flattening the transform. I still believe that
separating the concept of "transform flattening" and "stacking context
flattening" is the way to go.

[pdr., 2016-07-12 03:58:05]

On 2016/07/11 at 23:45:45, trchen wrote:
> Counter-example: http://jsbin.com/wikehihunu/
> A flat element with perspective applies perspective to each children (which
can create separate 3D context).
> 
> Counter-example 2: http://jsbin.com/moqayilumo/
> It is unclear what should happen when containing block penetrates stacking
context.
> 
> Both cases involve a situation that we want to flatten a group of planes into
a stacking context without flattening the transform. I still believe that
separating the concept of "transform flattening" and "stacking context
flattening" is the way to go.

This patch just plumbs this through to cc's
TransformNode.flattens_inherited_transform. Is it not possible to implement
these examples with cc's current flattening approach?

[jbroman, 2016-07-12 18:11:37]

On 2016/07/11 at 23:45:45, trchen wrote:
> Counter-example: http://jsbin.com/wikehihunu/
> A flat element with perspective applies perspective to each children (which
can create separate 3D context).

Ah, this was the result of me taking a shortcut. Fixed; PaintPropertyTreeBuilder
is now explicit about whether it's OK to create a 3D rendering context (which it
isn't for the perspective node). This page now renders correctly, and I've added
a unit test for this case.

> Counter-example 2: http://jsbin.com/moqayilumo/
> It is unclear what should happen when containing block penetrates stacking
context.

This is an interesting case (and one that we already have funny behaviour on),
but approximating our behaviour today seems like a reasonable step for now. I
haven't looked at how we, and other vendors, deal with cases like this today. I
wouldn't be surprised if it were inconsistent.

> Both cases involve a situation that we want to flatten a group of planes into
a stacking context without flattening the transform. I still believe that
separating the concept of "transform flattening" and "stacking context
flattening" is the way to go.

Today this isn't legal in cc; every render surface must flatten transforms.

[trchen, 2016-07-13 04:14:57]

Counter-example 3: http://jsbin.com/papogugabi/

The way I see it is that separating the concept of "transform flattening" and
"stacking context flattening (i.e. depth sorting)" is a real issue we need to
solve. Let alone the quirky corner cases, perspective is a perfect example of
it. We flatten the transform matrix above the perspective node, while different
subtree below the perspective node need to flatten separately as different
sorting context.

In today's world there are many layer property combination that are inherently
undefined, and in some corner cases we do generate those undefined combination
and take whatever cc implementation does. To make a concrete example: What is
the right thing to do when we have a layer lists that has interleaving sorting
context? This is exactly what I exploited in counter-example 3.

I don't like this CL's approach because I think we should walk away from
sorting_context_id on the transform node. It is inherently undefined when the
chunks are not contiguous, or when mixed with effect nodes. It is because depth
sorting affects composite ordering, which is defined by the effect tree, thus it
should belong to effect tree. Transform tree should only affect the geometry of
a paint chunk.

On the opposite, effect nodes shouldn't affect geometry of paint chunks, unlike
counter-example 2.

That said, I chatted with pdr@ today for a third opinion. I'm convinced that
landing this CL first makes us one step closer to the goal. At least we got the
"transform flattening" part correct. We can move "stacking context flattening"
from transform node to effect node and make it correct in follow-up CLs.

[trchen, 2016-07-13 04:39:44]

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

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122: //
TODO(jbroman, pdr): Handle flattening in SVG?
SVG doesn't support 3D transforms.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:398:
transformNode->parent() ? transformNode->parent()->flattensTransform() : true;
I'd rather avoid this ambiguity. A transform node either flattens, or apply a
local matrix, but not both.

If performance is a concern so the node need to multiplex, please resolve it in
PaintPropertyTreeBuilder than here.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
enum RenderingContextBehavior {
I don't think this belong here. This is a CSS concept that should not leak into
platform layer.

The sole purpose of it is to resolve rendering context root. why don't we
resolve it in the PropertyTreeBuilder?

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
Ditto. It is a leak of CSS concept. I think flattensInheritedTransform is more
accurate.

We don't flatten contents in the subtree into the plane. If the contents flatten
into a plane then how can we perform depth sorting on them? It is the opposite.
We flatten the inherited transform so the depth information is preserved, but
won't become an input to the inherited transform.

[chrishtr, 2016-07-13 20:29:11]

On 2016/07/13 at 04:14:57, trchen wrote:
> Counter-example 3: http://jsbin.com/papogugabi/
> 
> The way I see it is that separating the concept of "transform flattening" and
"stacking context flattening (i.e. depth sorting)" is a real issue we need to
solve. Let alone the quirky corner cases, perspective is a perfect example of
it. We flatten the transform matrix above the perspective node, while different
subtree below the perspective node need to flatten separately as different
sorting context.
> 
> In today's world there are many layer property combination that are inherently
undefined, and in some corner cases we do generate those undefined combination
and take whatever cc implementation does. To make a concrete example: What is
the right thing to do when we have a layer lists that has interleaving sorting
context? This is exactly what I exploited in counter-example 3.

I think I'm missing what example 3 demonstrates.

> 
> I don't like this CL's approach because I think we should walk away from
sorting_context_id on the transform node. It is inherently undefined when the
chunks are not contiguous, or when mixed with effect nodes. It is because depth
sorting affects composite ordering, which is defined by the effect tree, thus it
should belong to effect tree. Transform tree should only affect the geometry of
a paint chunk.
> 
> On the opposite, effect nodes shouldn't affect geometry of paint chunks,
unlike counter-example 2.
> 
> That said, I chatted with pdr@ today for a third opinion. I'm convinced that
landing this CL first makes us one step closer to the goal. At least we got the
"transform flattening" part correct. We can move "stacking context flattening"
from transform node to effect node and make it correct in follow-up CLs.

[chrishtr, 2016-07-13 20:30:49]

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

[chrishtr, 2016-07-13 20:30:50]

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

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:151: case
TransformStyle3DFlat:
Given the requirement to initialize for MSVC, might as well just write:

if (style.transformStyle3D() == TransformStyle3DPreseve3D() {
  ...
}

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:395:
// cc's notion of flattening and Blink's differ based on whether they apply
Why is there such a difference? Is there something stopping us from adopting
cc'd semantics now?

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
On 2016/07/13 at 04:39:44, trchen wrote:
> Ditto. It is a leak of CSS concept. I think flattensInheritedTransform is more
accurate.
> 
> We don't flatten contents in the subtree into the plane. If the contents
flatten into a plane then how can we perform depth sorting on them? It is the
opposite. We flatten the inherited transform so the depth information is
preserved, but won't become an input to the inherited transform.

I don't understand this comment, could you rephrase or given an example? Also,
you mentioned "inherited transform" twice but they seem to refer to different
things, no?

[chrishtr, 2016-07-13 20:32:44]

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:395:
// cc's notion of flattening and Blink's differ based on whether they apply
On 2016/07/13 at 20:30:50, chrishtr wrote:
> Why is there such a difference? Is there something stopping us from adopting
cc'd semantics now?

Sorry, mean to say "adopting cc's semantics".

[jbroman, 2016-07-13 21:47:13]

Still sketching out what a version that gets closer to cc semantics in core/
rather than in platform/ looks like, as requested. Sending a few quick responses
first.

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

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:122: //
TODO(jbroman, pdr): Handle flattening in SVG?
On 2016/07/13 at 04:39:44, trchen wrote:
> SVG doesn't support 3D transforms.

Oh good; that simplifies things. I guess I should have realized that after
seeing AffineTransform above.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp:395:
// cc's notion of flattening and Blink's differ based on whether they apply
On 2016/07/13 at 20:32:44, chrishtr wrote:
> On 2016/07/13 at 20:30:50, chrishtr wrote:
> > Why is there such a difference? Is there something stopping us from adopting
cc'd semantics now?
> 
> Sorry, mean to say "adopting cc's semantics".

We cannot adopt cc's semantics in CSS (because they are web-exposed).

That leaves us lowering them either in PaintPropertyTreeBuilder or
PaintArtifactCompositor, both of which do some amount of conversion from
CSS-like semantics to cc-like semantics. The CSS-like semantics seemed more
natural to me (IIUC cc describes it the way it does because of how it computes
total transforms, top-down), but we could have TransformPaintPropertyNode be
more cc-like in this regard, at the cost of some additional bookkeeping.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
enum RenderingContextBehavior {
On 2016/07/13 at 04:39:44, trchen wrote:
> I don't think this belong here. This is a CSS concept that should not leak
into platform layer.
> 
> The sole purpose of it is to resolve rendering context root. why don't we
resolve it in the PropertyTreeBuilder?

Because the |this| pointer doesn't exist yet there (until the |new| operator),
so I cannot provide it. (And I cannot use |nullptr| as a placeholder for it,
because that causes the perspective issue you pointed out.)

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
On 2016/07/13 at 04:39:44, trchen wrote:
> Ditto. It is a leak of CSS concept. I think flattensInheritedTransform is more
accurate.
> 
> We don't flatten contents in the subtree into the plane. If the contents
flatten into a plane then how can we perform depth sorting on them? It is the
opposite. We flatten the inherited transform so the depth information is
preserved, but won't become an input to the inherited transform.

This isn't the CSS concept, because it is not intertwined with rendering
contexts anymore; PaintPropertyTreeBuilder makes that separation.

It's essentially "flattens inherited transform", except that it's off-by-one;
PaintArtifactCompositor makes the translation. cc's flattens_inherited_transform
means that the transform applied due to ancestor nodes is flattened before it
applies to this node. This "flattens transform / preserves 3D" (perhaps I could
rename the latter, if it's confusing, but here I mean "preserves the
non-flat-ness", not "preserves the 3D rendering context") means that the
transform due to ancestor nodes _and this node_ is flattened before it applies
to descendants.

[trchen, 2016-07-13 22:31:34]

On 2016/07/13 20:29:11, chrishtr wrote:
> On 2016/07/13 at 04:14:57, trchen wrote:
> > Counter-example 3: http://jsbin.com/papogugabi/
> > 
> > The way I see it is that separating the concept of "transform flattening"
and
> "stacking context flattening (i.e. depth sorting)" is a real issue we need to
> solve. Let alone the quirky corner cases, perspective is a perfect example of
> it. We flatten the transform matrix above the perspective node, while
different
> subtree below the perspective node need to flatten separately as different
> sorting context.
> > 
> > In today's world there are many layer property combination that are
inherently
> undefined, and in some corner cases we do generate those undefined combination
> and take whatever cc implementation does. To make a concrete example: What is
> the right thing to do when we have a layer lists that has interleaving sorting
> context? This is exactly what I exploited in counter-example 3.
> 
> I think I'm missing what example 3 demonstrates.

Here is the transform tree generated by example 3:

T0 ---> T1 -+-> T2 (translateZ(0), sorting_context=1)
            +-> T3 (translateZ(-1), sorting_context=1) ---> T4 (translateZ(0))


And chunk list:

P0(green, T2, sorting_context=1) P1(red, T4)

It is unclear how should P1 depth-sort against other paint chunks in the list.

[trchen, 2016-07-13 22:31:43]

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
enum RenderingContextBehavior {
On 2016/07/13 21:47:13, jbroman wrote:
> On 2016/07/13 at 04:39:44, trchen wrote:
> > I don't think this belong here. This is a CSS concept that should not leak
> into platform layer.
> > 
> > The sole purpose of it is to resolve rendering context root. why don't we
> resolve it in the PropertyTreeBuilder?
> 
> Because the |this| pointer doesn't exist yet there (until the |new| operator),
> so I cannot provide it. (And I cannot use |nullptr| as a placeholder for it,
> because that causes the perspective issue you pointed out.)

Why do we need to TransformPaintPropertyNode* as sorting context id? Can't we
generate a serial number in PaintPropertyTreeBuilder?

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
On 2016/07/13 20:30:50, chrishtr wrote:
> On 2016/07/13 at 04:39:44, trchen wrote:
> > Ditto. It is a leak of CSS concept. I think flattensInheritedTransform is
more
> accurate.
> > 
> > We don't flatten contents in the subtree into the plane. If the contents
> flatten into a plane then how can we perform depth sorting on them? It is the
> opposite. We flatten the inherited transform so the depth information is
> preserved, but won't become an input to the inherited transform.
> 
> I don't understand this comment, could you rephrase or given an example? Also,
> you mentioned "inherited transform" twice but they seem to refer to different
> things, no?

For example:

<div style="transform-style:preserve-3d;">
  <div style="transform:rotateX(10deg);">
    <div style="transform-style:preserve-3d;">
      <div style="transform:rotateY(10deg);">A</div>
      <div style="transform:rotateY(-10deg);">B</div>
    </div>
  </div>
</div>

It's not that we flatten A and B to the flattening plane. That way the
local-to-screen transform would be:
A.local_to_screen = rotateX(10deg) * flatten(rotateY(10deg))
B.local_to_screen = rotateX(10deg) * flatten(rotateY(-10deg))

They will be co-planar (to the plane that created 3D context) thus can't perform
depth sorting.

In fact what happened was that the inherited transform getting flattened, so:
A.local_to_screen = flatten(rotateX(10deg)) * rotateY(10deg)
B.local_to_screen = flatten(rotateX(10deg)) * rotateY(-10deg)
which is equivalent to
A.local_to_screen = scaleY(0.985) * rotateY(10deg)
B.local_to_screen = scaleY(0.985) * rotateY(-10deg)

The term "flatten inherited transform" makes sense in the context of computing
local-to-screen matrix of a transform node. Basically:
this->local_to_screen = (this->flatten_Inherited_transform ?
flatten(parent->local_to_screen) : parent->local_to_screen) * this->local

I agree this is a bit of counter-intuitive, but this is what Blink/WebKit does.
You'll realize its semantics is pretty clean once you get used to it.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
On 2016/07/13 21:47:13, jbroman wrote:
> On 2016/07/13 at 04:39:44, trchen wrote:
> > Ditto. It is a leak of CSS concept. I think flattensInheritedTransform is
more
> accurate.
> > 
> > We don't flatten contents in the subtree into the plane. If the contents
> flatten into a plane then how can we perform depth sorting on them? It is the
> opposite. We flatten the inherited transform so the depth information is
> preserved, but won't become an input to the inherited transform.
> 
> This isn't the CSS concept, because it is not intertwined with rendering
> contexts anymore; PaintPropertyTreeBuilder makes that separation.
> 
> It's essentially "flattens inherited transform", except that it's off-by-one;
> PaintArtifactCompositor makes the translation. cc's
flattens_inherited_transform
> means that the transform applied due to ancestor nodes is flattened before it
> applies to this node. This "flattens transform / preserves 3D" (perhaps I
could
> rename the latter, if it's confusing, but here I mean "preserves the
> non-flat-ness", not "preserves the 3D rendering context") means that the
> transform due to ancestor nodes _and this node_ is flattened before it applies
> to descendants.

In that case, why not name it "flattensLocalToScreenTransform"? I still don't
know why we need to introduce a similar but different semantics. Can't we
resolve it to cc semantics in PropertyTreeBuilder?

[jbroman, 2016-07-14 20:34:58]

N.B. this CL now depends on https://codereview.chromium.org/2144733008, because
moving the rendering context ID tracking and flattening propagation into
PaintPropertyTreeBuilder adds a few more bits that need to be tracked per
containing block (and making sure they are all copied correctly was becoming
unwieldly).

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h
(right):

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:25:
enum RenderingContextBehavior {
On 2016/07/13 at 22:31:42, trchen wrote:
> On 2016/07/13 21:47:13, jbroman wrote:
> > On 2016/07/13 at 04:39:44, trchen wrote:
> > > I don't think this belong here. This is a CSS concept that should not leak
> > into platform layer.
> > > 
> > > The sole purpose of it is to resolve rendering context root. why don't we
> > resolve it in the PropertyTreeBuilder?
> > 
> > Because the |this| pointer doesn't exist yet there (until the |new|
operator),
> > so I cannot provide it. (And I cannot use |nullptr| as a placeholder for it,
> > because that causes the perspective issue you pointed out.)
> 
> Why do we need to TransformPaintPropertyNode* as sorting context id? Can't we
generate a serial number in PaintPropertyTreeBuilder?

It doesn't need to be; it just was (it's convenient for debugging, though).
Changed to have PaintPropertyTreeBuilder assign one.

https://codereview.chromium.org/2137173002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:87:
bool preserves3D() const { return transformStyle() == Preserve3D; }
On 2016/07/13 at 22:31:42, trchen wrote:
> Can't we resolve it to cc semantics in PropertyTreeBuilder?

We can, and now this CL does. It does require some additional bookkeeping,
though (on the other hand, it arguably simplifies TransformPaintPropertyNode
itself).

[jbroman, 2016-07-14 21:25:05]

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

[commit-bot: I haz the power, 2016-07-14 21:25:54]

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, 2016-07-14 22:39:42]

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

[commit-bot: I haz the power, 2016-07-14 22:39:43]

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

[jbroman, 2016-07-14 23:20:43]

Will look into the bot failures tomorrow; they passed locally, but I've likely
made a minor error.

[jbroman, 2016-07-15 01:15:35]

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

[commit-bot: I haz the power, 2016-07-15 01:16:15]

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

[jbroman, 2016-07-15 01:17:55]

Ah, needed to use dummyRootEffect.

[commit-bot: I haz the power, 2016-07-15 02:42:04]

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

[commit-bot: I haz the power, 2016-07-15 02:42:06]

Dry run: This issue passed the CQ dry run.

[jbroman, 2016-07-19 14:34:45]

ping

[trchen, 2016-07-19 22:19:42]

lgtm overall.

https://codereview.chromium.org/2137173002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2137173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:176:
childrenFlattenInheritedTransform = false;
Note: transform-style should be respected if and only if a PaintLayer is
created, but let's ignore the little details for now.

[jbroman, 2016-07-20 15:30:56]

https://codereview.chromium.org/2137173002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/2137173002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:176:
childrenFlattenInheritedTransform = false;
On 2016/07/19 at 22:19:42, trchen wrote:
> Note: transform-style should be respected if and only if a PaintLayer is
created, but let's ignore the little details for now.

Are you asking for a TODO, a bug, or would you like this as-is?

[jbroman, 2016-07-22 13:26:20]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-07-22 13:26:20]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2137173002/#ps140001
(title: "merge with master; add TODO for trchen's comment")

[commit-bot: I haz the power, 2016-07-22 13:26:39]

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, 2016-07-22 16:58:25]

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

[commit-bot: I haz the power, 2016-07-22 16:58:27]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jbroman, 2016-07-22 17:24:16]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-07-22 17:24:37]

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, 2016-07-22 20:34:29]

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

[commit-bot: I haz the power, 2016-07-22 20:34:32]

Failed to apply patch for
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:19
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp' with
conflicts.
  U third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp

Patch:       third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
Index: third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
diff --git a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
index
1d394cb7cd37438feba099574ccad966af718cfb..cfb00dbb26bec7ada0d061b45ddb21cd20e7b284
100644
--- a/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
+++ b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
@@ -19,9 +19,27 @@
 
 namespace blink {
 
+namespace {
+
+// Creates a transform node which uses the current rendering context ID and
+// flattening behavior from the tree builder context.
+PassRefPtr<TransformPaintPropertyNode> createTransformNode(
+    const PaintPropertyTreeBuilderContext& context,
+    const TransformationMatrix& matrix,
+    const FloatPoint3D& origin,
+    PassRefPtr<TransformPaintPropertyNode> parent)
+{
+    return TransformPaintPropertyNode::create(
+        matrix, origin, parent,
+        context.current.shouldFlattenInheritedTransform,
+        context.current.renderingContextID);
+}
+
+} // namespace
+
 void PaintPropertyTreeBuilder::buildTreeRootNodes(FrameView& rootFrame,
PaintPropertyTreeBuilderContext& context)
 {
-    RefPtr<TransformPaintPropertyNode> transformRoot =
TransformPaintPropertyNode::create(TransformationMatrix(), FloatPoint3D(),
nullptr);
+    RefPtr<TransformPaintPropertyNode> transformRoot =
TransformPaintPropertyNode::create(TransformationMatrix(), FloatPoint3D(),
nullptr, true);
     context.current.transform = context.absolutePosition.transform =
context.fixedPosition.transform = transformRoot.get();
     rootFrame.setRootTransform(std::move(transformRoot));
 
@@ -42,7 +60,7 @@ void PaintPropertyTreeBuilder::buildTreeNodes(FrameView&
frameView, PaintPropert
 
     TransformationMatrix frameTranslate;
     frameTranslate.translate(frameView.x() + context.current.paintOffset.x(),
frameView.y() + context.current.paintOffset.y());
-    RefPtr<TransformPaintPropertyNode> newTransformNodeForPreTranslation =
TransformPaintPropertyNode::create(frameTranslate, FloatPoint3D(),
context.current.transform);
+    RefPtr<TransformPaintPropertyNode> newTransformNodeForPreTranslation =
createTransformNode(context, frameTranslate, FloatPoint3D(),
context.current.transform);
 
     FloatRoundedRect contentClip(IntRect(IntPoint(),
frameView.visibleContentSize()));
     RefPtr<ClipPaintPropertyNode> newClipNodeForContentClip =
ClipPaintPropertyNode::create(newTransformNodeForPreTranslation.get(),
contentClip, context.current.clip);
@@ -58,6 +76,8 @@ void PaintPropertyTreeBuilder::buildTreeNodes(FrameView&
frameView, PaintPropert
     context.current.transform = newTransformNodeForScrollTranslation.get();
     context.current.paintOffset = LayoutPoint();
     context.current.clip = newClipNodeForContentClip.get();
+    context.current.renderingContextID = 0;
+    context.current.shouldFlattenInheritedTransform = true;
     context.absolutePosition = context.current;
     context.containerForAbsolutePosition = nullptr;
     context.fixedPosition = context.current;
@@ -88,11 +108,13 @@ void
PaintPropertyTreeBuilder::updatePaintOffsetTranslation(const LayoutObject&
     IntPoint roundedPaintOffset = roundedIntPoint(context.current.paintOffset);
     LayoutPoint fractionalPaintOffset = LayoutPoint(context.current.paintOffset
- roundedPaintOffset);
 
-    RefPtr<TransformPaintPropertyNode> paintOffsetTranslation =
TransformPaintPropertyNode::create(
+    RefPtr<TransformPaintPropertyNode> paintOffsetTranslation =
createTransformNode(
+        context,
         TransformationMatrix().translate(roundedPaintOffset.x(),
roundedPaintOffset.y()),
         FloatPoint3D(), context.current.transform);
     context.current.transform = paintOffsetTranslation.get();
     context.current.paintOffset = fractionalPaintOffset;
+    context.current.shouldFlattenInheritedTransform = false;
    
object.getMutableForPainting().ensureObjectPaintProperties().setPaintOffsetTranslation(paintOffsetTranslation.release());
 }
 
@@ -124,20 +146,44 @@ void PaintPropertyTreeBuilder::updateTransform(const
LayoutObject& object, Paint
         RefPtr<TransformPaintPropertyNode> svgTransform =
TransformPaintPropertyNode::create(
             transform, FloatPoint3D(0, 0, 0), context.current.transform);
         context.current.transform = svgTransform.get();
+        context.current.renderingContextID = 0;
+        context.current.shouldFlattenInheritedTransform = false;
        
object.getMutableForPainting().ensureObjectPaintProperties().setTransform(svgTransform.release());
         return;
     }
 
+    if (!object.isBox())
+        return;
     const ComputedStyle& style = object.styleRef();
-    if (!object.isBox() || !style.hasTransform())
+    if (!style.hasTransform() && !style.preserves3D())
         return;
 
     TransformationMatrix matrix;
     style.applyTransform(matrix, toLayoutBox(object).size(),
ComputedStyle::ExcludeTransformOrigin,
         ComputedStyle::IncludeMotionPath,
ComputedStyle::IncludeIndependentTransformProperties);
+
+    unsigned renderingContextID = context.current.renderingContextID;
+    unsigned renderingContextIDForChildren = 0;
+    bool flattensInheritedTransform =
context.current.shouldFlattenInheritedTransform;
+    bool childrenFlattenInheritedTransform = true;
+
+    // TODO(trchen): transform-style should only be respected if a PaintLayer
is
+    // created.
+    if (style.preserves3D()) {
+        // If a node with transform-style: preserve-3d does not exist in an
+        // existing rendering context, it establishes a new one.
+        if (!renderingContextID)
+            renderingContextID = PtrHash<const LayoutObject>::hash(&object);
+        renderingContextIDForChildren = renderingContextID;
+        childrenFlattenInheritedTransform = false;
+    }
+
     RefPtr<TransformPaintPropertyNode> transformNode =
TransformPaintPropertyNode::create(
-        matrix, transformOrigin(toLayoutBox(object)),
context.current.transform);
+        matrix, transformOrigin(toLayoutBox(object)),
context.current.transform,
+        flattensInheritedTransform, renderingContextID);
     context.current.transform = transformNode.get();
+    context.current.renderingContextID = renderingContextIDForChildren;
+    context.current.shouldFlattenInheritedTransform =
childrenFlattenInheritedTransform;
    
object.getMutableForPainting().ensureObjectPaintProperties().setTransform(transformNode.release());
 }
 
@@ -251,11 +297,17 @@ void PaintPropertyTreeBuilder::updatePerspective(const
LayoutObject& object, Pai
     if (!object.isBox() || !style.hasPerspective())
         return;
 
-    RefPtr<TransformPaintPropertyNode> perspective =
TransformPaintPropertyNode::create(
+    // The perspective node must not flatten (else nothing will get
+    // perspective), but it should still extend the rendering context as most
+    // transform nodes do.
+    RefPtr<TransformPaintPropertyNode> perspective = createTransformNode(
+        context,
         TransformationMatrix().applyPerspective(style.perspective()),
         perspectiveOrigin(toLayoutBox(object)) +
toLayoutSize(context.current.paintOffset),
         context.current.transform);
     context.current.transform = perspective.get();
+    context.current.shouldFlattenInheritedTransform = false;
+
    
object.getMutableForPainting().ensureObjectPaintProperties().setPerspective(perspective.release());
 }
 
@@ -277,6 +329,8 @@ void
PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform(const LayoutOb
         transformToBorderBox, FloatPoint3D(0, 0, 0),
context.current.transform);
     context.current.transform = svgLocalToBorderBoxTransform.get();
     context.current.paintOffset = LayoutPoint();
+    context.current.renderingContextID = 0;
+    context.current.shouldFlattenInheritedTransform = false;
    
object.getMutableForPainting().ensureObjectPaintProperties().setSvgLocalToBorderBoxTransform(svgLocalToBorderBoxTransform.release());
 }
 
@@ -291,11 +345,13 @@ void
PaintPropertyTreeBuilder::updateScrollTranslation(const LayoutObject& objec
     if (scrollOffset.isZero() && !layer->scrollsOverflow())
         return;
 
-    RefPtr<TransformPaintPropertyNode> scrollTranslation =
TransformPaintPropertyNode::create(
+    RefPtr<TransformPaintPropertyNode> scrollTranslation = createTransformNode(
+        context,
         TransformationMatrix().translate(-scrollOffset.width(),
-scrollOffset.height()),
         FloatPoint3D(),
         context.current.transform);
     context.current.transform = scrollTranslation.get();
+    context.current.shouldFlattenInheritedTransform = false;
    
object.getMutableForPainting().ensureObjectPaintProperties().setScrollTranslation(scrollTranslation.release());
 }
 
@@ -399,8 +455,6 @@ void PaintPropertyTreeBuilder::buildTreeNodes(const
LayoutObject& object, PaintP
     updateLocalBorderBoxContext(object, context);
     updateScrollbarPaintOffset(object, context);
     updateOverflowClip(object, context);
-    // TODO(trchen): Insert flattening transform here, as specified by
-    // http://www.w3.org/TR/css3-transforms/#transform-style-property
     updatePerspective(object, context);
     updateSvgLocalToBorderBoxTransform(object, context);
     updateScrollTranslation(object, context);

[jbroman, 2016-07-24 01:33:12]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-07-24 01:33:12]

The patchset sent to the CQ was uploaded after l-g-t-m from trchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2137173002/#ps160001
(title: "large merge (with https://codereview.chromium.org/2144823006)")

[commit-bot: I haz the power, 2016-07-24 01:33:24]

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, 2016-07-24 03:42:46]

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

[commit-bot: I haz the power, 2016-07-24 03:42:48]

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

[jbroman, 2016-07-24 03:43:40]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-07-24 03:43:47]

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, 2016-07-24 12:33:25]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-07-24 12:34:56]

Description was changed from

==========
[SPv2] Respect transform-style in the Blink and cc property trees.

This allows the creation of 3D rendering contexts (thus 3D sorting), and of
setting the bits controlling transform flattening, according to the
CSS transform-style.

BUG=563667
==========

to

==========
[SPv2] Respect transform-style in the Blink and cc property trees.

This allows the creation of 3D rendering contexts (thus 3D sorting), and of
setting the bits controlling transform flattening, according to the
CSS transform-style.

BUG=563667

Committed: https://crrev.com/cc109564b3b1277f76d0d022c5be5560b9c239be
Cr-Commit-Position: refs/heads/master@{#407381}
==========

[commit-bot: I haz the power, 2016-07-24 12:34:58]

Patchset 9 (id:??) landed as
https://crrev.com/cc109564b3b1277f76d0d022c5be5560b9c239be
Cr-Commit-Position: refs/heads/master@{#407381}