Put transform tree building in DisplayItemPropertyTreeBuilder.

Put transform tree building in DisplayItemPropertyTreeBuilder.

This lays the groundwork for doing transform and clip tree construction in one
place. It also cleans up the part of the interface which is exposed in the Blink
API, and separates range records from the tree itself.

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

[jbroman, 2015-08-17 19:39:41]

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

[jbroman, 2015-08-17 19:39:42]

This lays the groundwork for building clip trees, but doesn't currently include
any conditions in which a clip is actually added.

I can split this in two CLs if you'd like, but the reason for the refactor is
less obvious before DIPTB knows about at least two kinds of tree.

[pdr., 2015-08-17 22:25:06]

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

[pdr., 2015-08-17 22:25:08]

+cc Wei, could you please review this as well?

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
File Source/core/compositing/DisplayListCompositingBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
Source/core/compositing/DisplayListCompositingBuilder.cpp:21: // TODO(pdr,
jbroman): Also release other trees, and use range records to
Nit: TODO(jbroman)

We just use the person who added the todo now, but it doesn't mean you're on the
hook for this.

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemClipTree.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemClipTree.cpp:17: WebFloatRect
infiniteRect(-infinity, -infinity, infinity, infinity);
add WebFloatRect::infiniteRect()?

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp:65: struct
BeginDisplayItemClassification {
This seems overabstracted.

WDYT about just adding light classification such as
requiresTransformNode(BeginTransform3DDisplayItem&) that returns
NeedsNode/DoesNotNeedNode? Instead of this intermediate classification, we'd
just copy out the matrix in processBeginItem(...).

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:40: struct
RangeRecord {
Why don't we store the range on each prop tree node instead of off to the side?

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:70: struct State
{
Lets choose a more descriptive name here, maybe even PropertyTreeState?

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemClipTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemClipTree.h:30: enum : size_t { kInvalidIndex =
static_cast<size_t>(-1) };
kInvalidIndex -> kRootIndex?

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemTransformTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemTransformTree.h:59: WebPrivateOwnPtr<const
DisplayItemTransformTree> m_private;
Why "m_private" instead of "m_transformTree"?

[jbroman, 2015-08-17 23:57:19]

At home right now, but a few quick thoughts in response.

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
File Source/core/compositing/DisplayListCompositingBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
Source/core/compositing/DisplayListCompositingBuilder.cpp:21: // TODO(pdr,
jbroman): Also release other trees, and use range records to
That's not what the code style says, nor what I was told when I started at
Google.

https://www.chromium.org/blink/coding-style ->
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments

says "TODOs should include the string TODO in all caps, followed by the name,
e-mail address, or other identifier of the person with the best context about
the problem referenced by the TODO."

My understanding is that it's supposed to be more "who knows what this means"
than "who added it" or "who should do it".

I can change it to just TODO(jbroman) if you prefer, but I'm unconvinced that
I'm only allowed to write TODO(jbroman).

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp:65: struct
BeginDisplayItemClassification {
On 2015/08/17 at 22:25:07, pdr wrote:
> This seems overabstracted.
> 
> WDYT about just adding light classification such as
requiresTransformNode(BeginTransform3DDisplayItem&) that returns
NeedsNode/DoesNotNeedNode? Instead of this intermediate classification, we'd
just copy out the matrix in processBeginItem(...).

Then the "copying out the matrix" bit will require the same logic as the
classification, as well. Transform3D is the only case here now, but it isn't the
only one -- scroll and TransformDisplayItem are missing, at least. If I'm
already going to go to the effort of figuring out what kind of display item I
have here, why write that again?

Similarly there is more than one kind of clip. ClipDisplayItem requires
intersecting an int rect with several float rects, FloatClipDisplayItem requires
something else, etc.

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:40: struct
RangeRecord {
On 2015/08/17 at 22:25:08, pdr wrote:
> Why don't we store the range on each prop tree node instead of off to the
side?

There can be more than one range (even with one property tree, as it was in
advance of this CL). Consider:

<transform id="root">
  A
  B
  <transform id="rotation">
    C
    D
  </transform>
  E
</transform>

AB and E share a transform node, but are discontiguous. Two separate ranges are
needed to specify which parts of the display list belong to this transform node.

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:70: struct State
{
On 2015/08/17 at 22:25:08, pdr wrote:
> Lets choose a more descriptive name here, maybe even PropertyTreeState?

It's a inner struct (we have plenty of "class Client" in Blink as well), and
it's private at that, so I'm not sure it's likely to be confused.

I can rename it if you'd like. It's really the state of the builder more than
the state of the property trees, and
blink::DisplayItemPropertyTreeBuilder::DisplayItemPropertyTreeBuilderState
seemed like a long name.

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemClipTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemClipTree.h:30: enum : size_t { kInvalidIndex =
static_cast<size_t>(-1) };
On 2015/08/17 at 22:25:08, pdr wrote:
> kInvalidIndex -> kRootIndex?

This is replicating WebDisplayItemTransformTree.h, but anyhow my reasoning is
that in this case the parent isn't the root, the node itself is the root (i.e.
it has no parent).

I'd expect kRootIndex to be 0 (i.e. the index of the root element).

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemTransformTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemTransformTree.h:59: WebPrivateOwnPtr<const
DisplayItemTransformTree> m_private;
On 2015/08/17 at 22:25:08, pdr wrote:
> Why "m_private" instead of "m_transformTree"?

The majority of cases of this pattern use that name:
https://code.google.com/p/chromium/codesearch#search/&q=file:%5Esrc/third_par...

[pdr., 2015-08-18 04:31:42]

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
File Source/core/compositing/DisplayListCompositingBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/core/compositing...
Source/core/compositing/DisplayListCompositingBuilder.cpp:21: // TODO(pdr,
jbroman): Also release other trees, and use range records to
On 2015/08/17 at 23:57:19, jbroman wrote:
> That's not what the code style says, nor what I was told when I started at
Google.
> 
> https://www.chromium.org/blink/coding-style ->
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments
> 
> says "TODOs should include the string TODO in all caps, followed by the name,
e-mail address, or other identifier of the person with the best context about
the problem referenced by the TODO."
> 
> My understanding is that it's supposed to be more "who knows what this means"
than "who added it" or "who should do it".
> 
> I can change it to just TODO(jbroman) if you prefer, but I'm unconvinced that
I'm only allowed to write TODO(jbroman).

I re-read the recent thread and you're correct. Disregard!

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp:65: struct
BeginDisplayItemClassification {
On 2015/08/17 at 23:57:19, jbroman wrote:
> On 2015/08/17 at 22:25:07, pdr wrote:
> > This seems overabstracted.
> > 
> > WDYT about just adding light classification such as
requiresTransformNode(BeginTransform3DDisplayItem&) that returns
NeedsNode/DoesNotNeedNode? Instead of this intermediate classification, we'd
just copy out the matrix in processBeginItem(...).
> 
> Then the "copying out the matrix" bit will require the same logic as the
classification, as well. Transform3D is the only case here now, but it isn't the
only one -- scroll and TransformDisplayItem are missing, at least. If I'm
already going to go to the effort of figuring out what kind of display item I
have here, why write that again?
> 
> Similarly there is more than one kind of clip. ClipDisplayItem requires
intersecting an int rect with several float rects, FloatClipDisplayItem requires
something else, etc.

I still don't follow but I may be missing something simple. Is there a case
where we'd have a single display item be both a transform kind and a clip kind?

If so, I like your approach as-is.

If not, the classification and independent switch statements seem tightly
coupled. Why not something like the following?

void DisplayItemPropertyTreeBuilder::processBeginItem(const DisplayItem&
displayItem)
{
  if (isTransform3DType(displayItem.type()) &&
addTransformNodeIfNeeded(displayItem))
    return;
  if (isClipType(displayItem.type()) && addClipNodeIfNeeded(displayItem))
    return;
  currentState().ignoredBeginCount++;
}

bool addTransformNodeIfNeeded(const DisplayItem& displayItem)
{
  if ( ... is identity or translation ... ) {
    State& nextState = finishAndPushNextState();
    nextState.offset += translation;
    return true;
  }
  if ( ... is 3d ... ) {
    State& nextState = finishAndPushNextState();
    ...
    return true;
  }
  return false;
}

bool addClipNodeIfNeeded(const DisplayItem& displayItem)
{
  // compute some clip properties.

  if ( ... no clip needed ... )
    return false;
  if ( ... float clip needed ... ) {
    State& nextState = finishAndPushNextState();
    ...
    return true;
  }
  return false;
}

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:40: struct
RangeRecord {
On 2015/08/17 at 23:57:19, jbroman wrote:
> On 2015/08/17 at 22:25:08, pdr wrote:
> > Why don't we store the range on each prop tree node instead of off to the
side?
> 
> There can be more than one range (even with one property tree, as it was in
advance of this CL). Consider:
> 
> <transform id="root">
>   A
>   B
>   <transform id="rotation">
>     C
>     D
>   </transform>
>   E
> </transform>
> 
> AB and E share a transform node, but are discontiguous. Two separate ranges
are needed to specify which parts of the display list belong to this transform
node.

What will the separate ranges containing {A,B} and {E} be used for?

SimpleLayers will need to store contiguous ranges of items, so as we're building
them up we will create one for {A,B}, one for {C,D}, and one for {E}. This is
needed to ensure the compositor has enough information to know whether {E} can
squash or not which depends on the SimpleLayer containing {C,D}. Because we need
these separated in the SimpleLayers, I think separate ranges will be redundant.

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemClipTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemClipTree.h:30: enum : size_t { kInvalidIndex =
static_cast<size_t>(-1) };
On 2015/08/17 at 23:57:19, jbroman wrote:
> On 2015/08/17 at 22:25:08, pdr wrote:
> > kInvalidIndex -> kRootIndex?
> 
> This is replicating WebDisplayItemTransformTree.h, but anyhow my reasoning is
that in this case the parent isn't the root, the node itself is the root (i.e.
it has no parent).
> 
> I'd expect kRootIndex to be 0 (i.e. the index of the root element).

SGTM

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
File public/platform/WebDisplayItemTransformTree.h (right):

https://codereview.chromium.org/1296963002/diff/60001/public/platform/WebDisp...
public/platform/WebDisplayItemTransformTree.h:59: WebPrivateOwnPtr<const
DisplayItemTransformTree> m_private;
On 2015/08/17 at 23:57:19, jbroman wrote:
> On 2015/08/17 at 22:25:08, pdr wrote:
> > Why "m_private" instead of "m_transformTree"?
> 
> The majority of cases of this pattern use that name:
>
https://code.google.com/p/chromium/codesearch#search/&q=file:%5Esrc/third_par...

Sadness. But, when in Rome..

[jbroman, 2015-08-18 14:18:16]

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp:65: struct
BeginDisplayItemClassification {
On 2015/08/18 at 04:31:42, pdr wrote:
> On 2015/08/17 at 23:57:19, jbroman wrote:
> > On 2015/08/17 at 22:25:07, pdr wrote:
> > > This seems overabstracted.
> > > 
> > > WDYT about just adding light classification such as
requiresTransformNode(BeginTransform3DDisplayItem&) that returns
NeedsNode/DoesNotNeedNode? Instead of this intermediate classification, we'd
just copy out the matrix in processBeginItem(...).
> > 
> > Then the "copying out the matrix" bit will require the same logic as the
classification, as well. Transform3D is the only case here now, but it isn't the
only one -- scroll and TransformDisplayItem are missing, at least. If I'm
already going to go to the effort of figuring out what kind of display item I
have here, why write that again?
> > 
> > Similarly there is more than one kind of clip. ClipDisplayItem requires
intersecting an int rect with several float rects, FloatClipDisplayItem requires
something else, etc.
> 
> I still don't follow but I may be missing something simple. Is there a case
where we'd have a single display item be both a transform kind and a clip kind?
> 
> If so, I like your approach as-is.
> 
> If not, the classification and independent switch statements seem tightly
coupled. Why not something like the following?
> 
> void DisplayItemPropertyTreeBuilder::processBeginItem(const DisplayItem&
displayItem)
> {
>   if (isTransform3DType(displayItem.type()) &&
addTransformNodeIfNeeded(displayItem))
>     return;
>   if (isClipType(displayItem.type()) && addClipNodeIfNeeded(displayItem))
>     return;
>   currentState().ignoredBeginCount++;
> }
> 
> bool addTransformNodeIfNeeded(const DisplayItem& displayItem)
> {
>   if ( ... is identity or translation ... ) {
>     State& nextState = finishAndPushNextState();
>     nextState.offset += translation;
>     return true;
>   }
>   if ( ... is 3d ... ) {
>     State& nextState = finishAndPushNextState();
>     ...
>     return true;
>   }
>   return false;
> }
> 
> bool addClipNodeIfNeeded(const DisplayItem& displayItem)
> {
>   // compute some clip properties.
> 
>   if ( ... no clip needed ... )
>     return false;
>   if ( ... float clip needed ... ) {
>     State& nextState = finishAndPushNextState();
>     ...
>     return true;
>   }
>   return false;
> }

Yes, I do expect that display items may create nodes in multiple property trees.
A scroll display item creates both a transform node and a scroll node. A clip
node (with a rounded rect) creates both a clip node and an effect node. And so
on.

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:40: struct
RangeRecord {
On 2015/08/18 at 04:31:42, pdr wrote:
> On 2015/08/17 at 23:57:19, jbroman wrote:
> > On 2015/08/17 at 22:25:08, pdr wrote:
> > > Why don't we store the range on each prop tree node instead of off to the
side?
> > 
> > There can be more than one range (even with one property tree, as it was in
advance of this CL). Consider:
> > 
> > <transform id="root">
> >   A
> >   B
> >   <transform id="rotation">
> >     C
> >     D
> >   </transform>
> >   E
> > </transform>
> > 
> > AB and E share a transform node, but are discontiguous. Two separate ranges
are needed to specify which parts of the display list belong to this transform
node.
> 
> What will the separate ranges containing {A,B} and {E} be used for?
> 
> SimpleLayers will need to store contiguous ranges of items, so as we're
building them up we will create one for {A,B}, one for {C,D}, and one for {E}.

Yes, exactly.

> This is needed to ensure the compositor has enough information to know whether
{E} can squash or not which depends on the SimpleLayer containing {C,D}. Because
we need these separated in the SimpleLayers, I think separate ranges will be
redundant.

If I don't do this conversion, then all I've done is marked display items as
creating or not creating transform nodes -- I haven't actually associated
display items with their transform nodes.

My previous document for transform trees discussed this:
https://docs.google.com/document/d/1VWjdq8hCJlNbak5ZyAsnLh-0--Hl_wht0xyuagODl...

However, these ranges /is/ sufficient information to cheaply find out which
transform (and clip, etc.) applies to a given drawing display item (equivalent
to a per-drawing m_transformNode field). Further, I think that these range
records, with a trivial transformation, constitute unoptimized simple layers.

https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:70: struct State
{
On 2015/08/17 at 23:57:19, jbroman wrote:
> On 2015/08/17 at 22:25:08, pdr wrote:
> > Lets choose a more descriptive name here, maybe even PropertyTreeState?
> 
> It's a inner struct (we have plenty of "class Client" in Blink as well), and
it's private at that, so I'm not sure it's likely to be confused.
> 
> I can rename it if you'd like. It's really the state of the builder more than
the state of the property trees, and
blink::DisplayItemPropertyTreeBuilder::DisplayItemPropertyTreeBuilderState
seemed like a long name.

Renamed BuilderState. WDYT?

[pdr., 2015-08-18 20:31:55]

On 2015/08/18 at 14:18:16, jbroman wrote:
>
https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.cpp:65: struct
BeginDisplayItemClassification {
> On 2015/08/18 at 04:31:42, pdr wrote:
> > On 2015/08/17 at 23:57:19, jbroman wrote:
> > > On 2015/08/17 at 22:25:07, pdr wrote:
> > > > This seems overabstracted.
> > > > 
> > > > WDYT about just adding light classification such as
requiresTransformNode(BeginTransform3DDisplayItem&) that returns
NeedsNode/DoesNotNeedNode? Instead of this intermediate classification, we'd
just copy out the matrix in processBeginItem(...).
> > > 
> > > Then the "copying out the matrix" bit will require the same logic as the
classification, as well. Transform3D is the only case here now, but it isn't the
only one -- scroll and TransformDisplayItem are missing, at least. If I'm
already going to go to the effort of figuring out what kind of display item I
have here, why write that again?
> > > 
> > > Similarly there is more than one kind of clip. ClipDisplayItem requires
intersecting an int rect with several float rects, FloatClipDisplayItem requires
something else, etc.
> > 
> > I still don't follow but I may be missing something simple. Is there a case
where we'd have a single display item be both a transform kind and a clip kind?
> > 
> > If so, I like your approach as-is.
> > 
> > If not, the classification and independent switch statements seem tightly
coupled. Why not something like the following?
> > 
> > void DisplayItemPropertyTreeBuilder::processBeginItem(const DisplayItem&
displayItem)
> > {
> >   if (isTransform3DType(displayItem.type()) &&
addTransformNodeIfNeeded(displayItem))
> >     return;
> >   if (isClipType(displayItem.type()) && addClipNodeIfNeeded(displayItem))
> >     return;
> >   currentState().ignoredBeginCount++;
> > }
> > 
> > bool addTransformNodeIfNeeded(const DisplayItem& displayItem)
> > {
> >   if ( ... is identity or translation ... ) {
> >     State& nextState = finishAndPushNextState();
> >     nextState.offset += translation;
> >     return true;
> >   }
> >   if ( ... is 3d ... ) {
> >     State& nextState = finishAndPushNextState();
> >     ...
> >     return true;
> >   }
> >   return false;
> > }
> > 
> > bool addClipNodeIfNeeded(const DisplayItem& displayItem)
> > {
> >   // compute some clip properties.
> > 
> >   if ( ... no clip needed ... )
> >     return false;
> >   if ( ... float clip needed ... ) {
> >     State& nextState = finishAndPushNextState();
> >     ...
> >     return true;
> >   }
> >   return false;
> > }
> 
> Yes, I do expect that display items may create nodes in multiple property
trees. A scroll display item creates both a transform node and a scroll node. A
clip node (with a rounded rect) creates both a clip node and an effect node. And
so on.

SGTM.

> 
>
https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):
> 
>
https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:40: struct
RangeRecord {
> On 2015/08/18 at 04:31:42, pdr wrote:
> > On 2015/08/17 at 23:57:19, jbroman wrote:
> > > On 2015/08/17 at 22:25:08, pdr wrote:
> > > > Why don't we store the range on each prop tree node instead of off to
the side?
> > > 
> > > There can be more than one range (even with one property tree, as it was
in advance of this CL). Consider:
> > > 
> > > <transform id="root">
> > >   A
> > >   B
> > >   <transform id="rotation">
> > >     C
> > >     D
> > >   </transform>
> > >   E
> > > </transform>
> > > 
> > > AB and E share a transform node, but are discontiguous. Two separate
ranges are needed to specify which parts of the display list belong to this
transform node.
> > 
> > What will the separate ranges containing {A,B} and {E} be used for?
> > 
> > SimpleLayers will need to store contiguous ranges of items, so as we're
building them up we will create one for {A,B}, one for {C,D}, and one for {E}.
> 
> Yes, exactly.
> 
> > This is needed to ensure the compositor has enough information to know
whether {E} can squash or not which depends on the SimpleLayer containing {C,D}.
Because we need these separated in the SimpleLayers, I think separate ranges
will be redundant.
> 
> If I don't do this conversion, then all I've done is marked display items as
creating or not creating transform nodes -- I haven't actually associated
display items with their transform nodes.
> 
> My previous document for transform trees discussed this:
https://docs.google.com/document/d/1VWjdq8hCJlNbak5ZyAsnLh-0--Hl_wht0xyuagODl...
> 
> However, these ranges /is/ sufficient information to cheaply find out which
transform (and clip, etc.) applies to a given drawing display item (equivalent
to a per-drawing m_transformNode field). Further, I think that these range
records, with a trivial transformation, constitute unoptimized simple layers.

This is the part I didn't understand, but it makes sense to begin building the
simple layers. SGTM

> 
>
https://codereview.chromium.org/1296963002/diff/60001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:70: struct
State {
> On 2015/08/17 at 23:57:19, jbroman wrote:
> > On 2015/08/17 at 22:25:08, pdr wrote:
> > > Lets choose a more descriptive name here, maybe even PropertyTreeState?
> > 
> > It's a inner struct (we have plenty of "class Client" in Blink as well), and
it's private at that, so I'm not sure it's likely to be confused.
> > 
> > I can rename it if you'd like. It's really the state of the builder more
than the state of the property trees, and
blink::DisplayItemPropertyTreeBuilder::DisplayItemPropertyTreeBuilderState
seemed like a long name.
> 
> Renamed BuilderState. WDYT?

SGTM

LGTM, thanks for sticking with me through the review.

[weiliangc, 2015-08-18 20:44:15]

LGTM

(In the future we could think about de-duplicate clip and transform tree, but I
don't think it is this patch's concern)

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:104:
Vector<BuilderState, 40> m_stateStack;
What is the 40 for?

[jbroman, 2015-08-18 20:50:13]

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:104:
Vector<BuilderState, 40> m_stateStack;
On 2015/08/18 at 20:44:15, weiliangc wrote:
> What is the 40 for?

WTF::Vector supports inline capacity. This puts room for 40 states on the stack
without having to do heap allocation. Purely a performance optimization, and
probably a number that should be tweaked based on real world data closer to
shipping.

[weiliangc, 2015-08-18 20:52:00]

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:104:
Vector<BuilderState, 40> m_stateStack;
On 2015/08/18 at 20:50:13, jbroman wrote:
> On 2015/08/18 at 20:44:15, weiliangc wrote:
> > What is the 40 for?
> 
> WTF::Vector supports inline capacity. This puts room for 40 states on the
stack without having to do heap allocation. Purely a performance optimization,
and probably a number that should be tweaked based on real world data closer to
shipping.

Could you make up a constant for this instead just use 40?

[jbroman, 2015-08-18 21:14:08]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2015-08-18 21:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
weiliangc@chromium.org
Link to the patchset: https://codereview.chromium.org/1296963002/#ps100001
(title: "comment for inline capacity")

[jbroman, 2015-08-18 21:14:11]

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):

https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:104:
Vector<BuilderState, 40> m_stateStack;
On 2015/08/18 at 20:51:59, weiliangc wrote:
> On 2015/08/18 at 20:50:13, jbroman wrote:
> > On 2015/08/18 at 20:44:15, weiliangc wrote:
> > > What is the 40 for?
> > 
> > WTF::Vector supports inline capacity. This puts room for 40 states on the
stack without having to do heap allocation. Purely a performance optimization,
and probably a number that should be tweaked based on real world data closer to
shipping.
> 
> Could you make up a constant for this instead just use 40?

Discussed over IM. Blink pretty overwhelmingly uses plain numbers here (for
better or for worse), but I've added a comment noting that we should figure out
what the right number actually is in the future.

[commit-bot: I haz the power, 2015-08-18 21:14:18]

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

[weiliangc, 2015-08-18 21:15:10]

On 2015/08/18 at 21:14:11, jbroman wrote:
>
https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
> File Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h (right):
> 
>
https://codereview.chromium.org/1296963002/diff/80001/Source/platform/graphic...
> Source/platform/graphics/paint/DisplayItemPropertyTreeBuilder.h:104:
Vector<BuilderState, 40> m_stateStack;
> On 2015/08/18 at 20:51:59, weiliangc wrote:
> > On 2015/08/18 at 20:50:13, jbroman wrote:
> > > On 2015/08/18 at 20:44:15, weiliangc wrote:
> > > > What is the 40 for?
> > > 
> > > WTF::Vector supports inline capacity. This puts room for 40 states on the
stack without having to do heap allocation. Purely a performance optimization,
and probably a number that should be tweaked based on real world data closer to
shipping.
> > 
> > Could you make up a constant for this instead just use 40?
> 
> Discussed over IM. Blink pretty overwhelmingly uses plain numbers here (for
better or for worse), but I've added a comment noting that we should figure out
what the right number actually is in the future.

Thanks for following through. :)

[commit-bot: I haz the power, 2015-08-18 23:04:06]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=200764