GeometryMapper: Support computing visual rects in spaces that are not direct ancestors.

GeometryMapper: Support computing visual rects in spaces that are not direct ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

Also adds in some extra debugging info attributing property tree nodes to
layout objects in PaintPropertyTreePrinter.

BUG=611379
Committed: https://crrev.com/3d6dfca4dfae148adecbe787cceeb368442b738b
Cr-Commit-Position: refs/heads/master@{#403574}

[chrishtr, 2016-06-30 16:15:42]

Description was changed from

==========
none


none


none


none


none


none


none


none


none


none


none


none


Merge branch 'master' into parentpaintproperties


none

BUG=
==========

to

==========
GeometryMapper: Support computing visual rects in spaces that are not direct
ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

BUG=611379
==========

[chrishtr, 2016-06-30 16:49:33]

Description was changed from

==========
GeometryMapper: Support computing visual rects in spaces that are not direct
ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

BUG=611379
==========

to

==========
GeometryMapper: Support computing visual rects in spaces that are not direct
ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

Also adds in some extra debugging info attributing property tree nodes to
layout objects in PaintPropertyTreePrinter.

BUG=611379
==========

[chrishtr, 2016-06-30 16:49:54]

chrishtr@chromium.org changed reviewers:
+ pdr@chromium.org, wangxianzhu@chromium.org, wkorman@chromium.org

[chrishtr, 2016-06-30 16:49:55]

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380:
object.getMutableForPainting().clearObjectPaintProperties();
Now allocating an ObjectPaintProperties object for all boxes, to support
representation of property tree states for all
of them in O(1) time. This is needed for paint invalidation.

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
(right):

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp:101:
CHECK_VISUAL_RECT(target1->layoutObject(), frameView->layoutView());
I plan to fix target2 in the next CL.

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h
(right):

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:28:
RefPtr<TransformPaintPropertyNode> transform;
Changed to RefPtr so that it can be safely stored in ObjectPaintProperties.

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:37:
PassRefPtr<TransformPaintPropertyNode> parentRef() const { return m_parent; }
Needed so that we can store a refptr to a parent in a PropertyTreeState, which
happens when passing to the
least-common-ancestor.

[pdr., 2016-06-30 21:21:50]

Looks good overall.

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380:
object.getMutableForPainting().clearObjectPaintProperties();
On 2016/06/30 at 16:49:55, chrishtr wrote:
> Now allocating an ObjectPaintProperties object for all boxes, to support
representation of property tree states for all
> of them in O(1) time. This is needed for paint invalidation.

To enforce this and ensure we don't create too many of these, can you add:
if (!object.isBox())
    return;

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp (right):

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp:98:
printer.addPropertyNode(paintOffsetTranslation, "PaintOffsetTranslation (" +
object.debugName() + ")");
+1, these debugnames are useful

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
(right):

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp:39: 
Nit: extra newline

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp:352:
EXPECT_RECT_EQ(expected, result);
Can you use real numbers for this test? Maybe transform1 = rotate(90) and
transform2 = rotate(-180), then verify the rect is transformed by -90deg?

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:37:
PassRefPtr<TransformPaintPropertyNode> parentRef() const { return m_parent; }
On 2016/06/30 at 16:49:55, chrishtr wrote:
> Needed so that we can store a refptr to a parent in a PropertyTreeState, which
happens when passing to the
> least-common-ancestor.

This doesn't appear to be used in this patch.

[chrishtr, 2016-07-01 17:52:24]

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:380:
object.getMutableForPainting().clearObjectPaintProperties();
On 2016/06/30 at 21:21:50, pdr. wrote:
> On 2016/06/30 at 16:49:55, chrishtr wrote:
> > Now allocating an ObjectPaintProperties object for all boxes, to support
representation of property tree states for all
> > of them in O(1) time. This is needed for paint invalidation.
> 
> To enforce this and ensure we don't create too many of these, can you add:
> if (!object.isBox())
>     return;

Done (in updateLocalBorderBoxContext)

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp
(right):

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapperTest.cpp:39: 
On 2016/06/30 at 21:21:50, pdr. wrote:
> Nit: extra newline

Done.

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

https://codereview.chromium.org/2105273004/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/TransformPaintPropertyNode.h:37:
PassRefPtr<TransformPaintPropertyNode> parentRef() const { return m_parent; }
On 2016/06/30 at 21:21:50, pdr. wrote:
> On 2016/06/30 at 16:49:55, chrishtr wrote:
> > Needed so that we can store a refptr to a parent in a PropertyTreeState,
which happens when passing to the
> > least-common-ancestor.
> 
> This doesn't appear to be used in this patch.

Removed. The test was crashing due to refptr counting, then I added this to fix
it, but it's now not crashing any more. I can easily re-add later if needed.

[Xianzhu, 2016-07-01 18:18:02]

lgtm

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:59: bool*
success);
Nit: Should |success| be a reference if it's required?

[pdr., 2016-07-01 18:39:40]

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

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384:
object.getMutableForPainting().clearObjectPaintProperties();
Won't this create an object paint properties for all layout objects? I think we
still need an isBox check here (or I may misunderstand).

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:17:
HashSet<RefPtr<TransformPaintPropertyNode>> intermediates;
We may want to use a lowest common ancestor approach that doesn't allocate like
nearestCommonAncestor in PaintArtifactCompositor.

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:198: return
m_identity;
Instead of using a member, you can use a static like so:
static const AffineTransform identity;

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:102: //
Returns the matrix used in |LocalToAncestorRect|. Sets |success| to failse iff
|localTransformNode| is not
Nit: failse -> false

[wkorman, 2016-07-01 19:19:12]

lgtm

LGTM

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:52: // the
the inverse transform mapping from the least common ancestor to
|destinationState.transform|.
double the

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:59: bool*
success);
On 2016/07/01 at 18:18:01, Xianzhu wrote:
> Nit: Should |success| be a reference if it's required?

I think this is the new Chromium-style way to show planned-for-mutation params?

[wkorman, 2016-07-01 19:19:13]

lgtm

LGTM

[chrishtr, 2016-07-01 22:01:12]

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

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:384:
object.getMutableForPainting().clearObjectPaintProperties();
On 2016/07/01 at 18:39:39, pdr. wrote:
> Won't this create an object paint properties for all layout objects? I think
we still need an isBox check here (or I may misunderstand).

This is clearObjectPaintProperties(), which removes it if present.

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:17:
HashSet<RefPtr<TransformPaintPropertyNode>> intermediates;
On 2016/07/01 at 18:39:40, pdr. wrote:
> We may want to use a lowest common ancestor approach that doesn't allocate
like nearestCommonAncestor in PaintArtifactCompositor.

Refactored to reuse that code, templated for all property tree types.

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:198: return
m_identity;
On 2016/07/01 at 18:39:40, pdr. wrote:
> Instead of using a member, you can use a static like so:
> static const AffineTransform identity;

That doesn't work, because it requires a global constructor, which is disallowed
in our compilation flags. The other alternative is to have a static that is
demand-allocated,
but IMO the code as-is seems ok.

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):

https://codereview.chromium.org/2105273004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:59: bool*
success);
On 2016/07/01 at 19:19:12, wkorman wrote:
> On 2016/07/01 at 18:18:01, Xianzhu wrote:
> > Nit: Should |success| be a reference if it's required?
> 
> I think this is the new Chromium-style way to show planned-for-mutation
params?

I followed Xianhzu's advice. It simplifies the code a bit and makes it less
error prone, since you can compare boolean equality of pointers by mistake
rather than their dereferenced value.

Re Chromium: the Blink->Chromium style change explicitly allows non-const
references for such use cases.

[chrishtr, 2016-07-01 22:02:36]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-01 22:02:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2105273004/#ps80001
(title: "none")

[commit-bot: I haz the power, 2016-07-01 22:02:58]

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-01 22:06:20]

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

[commit-bot: I haz the power, 2016-07-01 22:06:21]

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

[chrishtr, 2016-07-01 22:54:24]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-01 22:54:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2105273004/#ps120001
(title: "none")

[commit-bot: I haz the power, 2016-07-01 22:54:44]

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

[pdr., 2016-07-01 22:56:27]

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:11:
#include "wtf/HashSet.h"
No longer needed

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:37: return
destinationToLca.inverse().mapRect(result);
Do we need to intersect clip rects along the way like
localToVisualRectInAncestorSpace does?

[chrishtr, 2016-07-01 22:58:19]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2016-07-01 23:01:14]

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:11:
#include "wtf/HashSet.h"
On 2016/07/01 at 22:56:26, pdr. wrote:
> No longer needed

Done.

https://codereview.chromium.org/2105273004/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:37: return
destinationToLca.inverse().mapRect(result);
On 2016/07/01 at 22:56:26, pdr. wrote:
> Do we need to intersect clip rects along the way like
localToVisualRectInAncestorSpace does?

We basically give up on clipping at this point, because it may be ill-defined.

[chrishtr, 2016-07-01 23:01:49]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-01 23:02:00]

The CQ bit was unchecked by chrishtr@chromium.org

[chrishtr, 2016-07-01 23:03:50]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-07-01 23:03:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2105273004/#ps140001
(title: "none")

[commit-bot: I haz the power, 2016-07-01 23:04:17]

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

[pdr., 2016-07-01 23:04:42]

LGTM, I like it

[commit-bot: I haz the power, 2016-07-02 00:20:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-02 00:20:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-02 00:22:07]

Description was changed from

==========
GeometryMapper: Support computing visual rects in spaces that are not direct
ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

Also adds in some extra debugging info attributing property tree nodes to
layout objects in PaintPropertyTreePrinter.

BUG=611379
==========

to

==========
GeometryMapper: Support computing visual rects in spaces that are not direct
ancestors.

position: fixed is one example of this.

Demonstrate this by mapping an overflow rect for a fixed-position element using
property trees, and comparing with the legacy methods.

Also adds in some extra debugging info attributing property tree nodes to
layout objects in PaintPropertyTreePrinter.

BUG=611379

Committed: https://crrev.com/3d6dfca4dfae148adecbe787cceeb368442b738b
Cr-Commit-Position: refs/heads/master@{#403574}
==========

[commit-bot: I haz the power, 2016-07-02 00:22:08]

Patchset 8 (id:??) landed as
https://crrev.com/3d6dfca4dfae148adecbe787cceeb368442b738b
Cr-Commit-Position: refs/heads/master@{#403574}