[SPv2] Implement CSS clip property

[SPv2] Implement CSS clip property

This CL creates clip nodes for CSS clip in PaintPropertyTreeBuilder.

Committed: https://crrev.com/e5eb731a308ab62d98bcab39637155d066bbea68
Cr-Commit-Position: refs/heads/master@{#383654}

[trchen, 2016-03-23 00:08:24]

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

[trchen, 2016-03-23 00:08:25]

Uploaded for preview. Will add unit tests if the concept looks okay to you!

[pdr., 2016-03-23 00:53:31]

I think this looks pretty good. I tested a few sites and it seems to work
properly, and the code looks sound.

Can you investigate the following two failures though?
fast/images/color-profile-clip.html
fast/css/fixed-overlaps-absolute-in-clip.html

In addition to unit tests, can you add a new virtual directory so fast/clip/
gets run too?

https://codereview.chromium.org/1829493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1829493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:237: //
TODO(pdr): Remove the !object.isLayoutView() condition when removing FrameView
TODO(trchen)! :)

[trchen, 2016-03-23 01:45:42]

On 2016/03/23 00:53:31, pdr wrote:
> I think this looks pretty good. I tested a few sites and it seems to work
> properly, and the code looks sound.
> 
> Can you investigate the following two failures though?
> fast/images/color-profile-clip.html
> fast/css/fixed-overlaps-absolute-in-clip.html

Both are incorrectly culled due to crbug.com/593596

Also fast/css/fixed-overlaps-absolute-in-clip.html is expected to crash, so is
fast/clip/010.html . Those are the difficult case "paint chunk in a space that
is not a descendant space where its clip is in". PAC can't handle the case
correctly. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> In addition to unit tests, can you add a new virtual directory so fast/clip/
> gets run too?

Sure! I just tried it locally and most of them looked good.

https://codereview.chromium.org/1829493002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right):

https://codereview.chromium.org/1829493002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:237: //
TODO(pdr): Remove the !object.isLayoutView() condition when removing FrameView
On 2016/03/23 00:53:31, pdr wrote:
> TODO(trchen)! :)

It is copied from updateOutOfFlowContext(). The condition needs to be consistent
so better assign to the same person. I can take over both if you prefer. :)

[pdr., 2016-03-23 01:56:12]

Awesome, looking forward to the updated patch.

Feel free to leave the TODO(pdr). Your comment got me thinking... I wonder if it
would be cleaner for the fixedpos clip to be added in updateOutOfFlowContext?

[trchen, 2016-03-23 03:33:09]

On 2016/03/23 01:56:12, pdr wrote:
> Awesome, looking forward to the updated patch.

Haven't finished the unit test yet. Will upload another patch set later.

> Feel free to leave the TODO(pdr). Your comment got me thinking... I wonder if
it
> would be cleaner for the fixedpos clip to be added in updateOutOfFlowContext?

Yes I also felt risky about the mutual assumption. I uploaded your alternative
implementation and I felt it is cleaner too. You can look at both and let me
know which looker better to you.

[jbroman, 2016-03-23 17:16:51]

Doesn't seem unreasonable at a high level, though I worry about how complicated
this is becoming. I wonder if there's a nicer way to factor this, but I haven't
thought about it much as yet.

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

https://codereview.chromium.org/1829493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:332: static
void updateOutOfFlowContext(RefPtr<ClipPaintPropertyNode>&
newClipNodeForCSSClipFixedPosition, const LayoutObject& object,
PaintPropertyTreeBuilderContext& context, ClipPaintPropertyNode*
newClipNodeForCSSClip)
This argument order is very surprising to me. Normally input arguments precede
output arguments (and the Google C++ style guide strongly encourages that).

https://codereview.chromium.org/1829493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:366:
const_cast<TransformPaintPropertyNode*>(newClipNodeForCSSClip->localTransformSpace()),
const_cast makes me sad. I suspect one of these things should have its type
changed so this is unnecessary.

[trchen, 2016-03-24 00:53:58]

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

https://codereview.chromium.org/1829493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:332: static
void updateOutOfFlowContext(RefPtr<ClipPaintPropertyNode>&
newClipNodeForCSSClipFixedPosition, const LayoutObject& object,
PaintPropertyTreeBuilderContext& context, ClipPaintPropertyNode*
newClipNodeForCSSClip)
On 2016/03/23 17:16:51, jbroman wrote:
> This argument order is very surprising to me. Normally input arguments precede
> output arguments (and the Google C++ style guide strongly encourages that).

Done.

I learned Intel syntax when I was young and found out everybody else on this
planet use AT&T syntax. :'‑(

https://codereview.chromium.org/1829493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:366:
const_cast<TransformPaintPropertyNode*>(newClipNodeForCSSClip->localTransformSpace()),
On 2016/03/23 17:16:51, jbroman wrote:
> const_cast makes me sad. I suspect one of these things should have its type
> changed so this is unnecessary.

I don't know... We need it non-const only for reference counting. In Blink we
almost never have const pointer that is permanent (non-auto scope). I personally
feel reference counter should be mutable but I bet that must have been debated
100 times.

[trchen, 2016-03-24 00:54:08]

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

[commit-bot: I haz the power, 2016-03-24 00:54:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1829493002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1829493002/60001

[jbroman, 2016-03-24 01:03:39]

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

https://codereview.chromium.org/1829493002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:366:
const_cast<TransformPaintPropertyNode*>(newClipNodeForCSSClip->localTransformSpace()),
On 2016/03/24 at 00:53:58, trchen wrote:
> On 2016/03/23 17:16:51, jbroman wrote:
> > const_cast makes me sad. I suspect one of these things should have its type
> > changed so this is unnecessary.
> 
> I don't know... We need it non-const only for reference counting. In Blink we
almost never have const pointer that is permanent (non-auto scope). I personally
feel reference counter should be mutable but I bet that must have been debated
100 times.

Oh, weird. Both Chromium's and Skia's have it as mutable, so I didn't even
realize WTF::RefCounted<T> didn't have that property.

[commit-bot: I haz the power, 2016-03-24 02:08:10]

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

[commit-bot: I haz the power, 2016-03-24 02:08:11]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-03-25 03:43:47]

LGTM

<strikethrough>hurry up and land while Jeremy is OOO</strikethrough>In the
standup Jeremy and I talked about some ideas to simplify
PaintPropertyTreeBuilder. This patch doesn't add significant complexity beyond
continuing the existing design, though I think we all agree that we've
accumulated a lot of CSS complexity in one class. Lets keep thinking of ways to
cleanup PaintPropertyTreeBuilder in the future.

[trchen, 2016-03-25 22:50:13]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-03-25 22:50:30]

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

[commit-bot: I haz the power, 2016-03-26 01:59:37]

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

[commit-bot: I haz the power, 2016-03-26 01:59:38]

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

[trchen, 2016-03-29 01:07:18]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-03-29 01:07:30]

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

[commit-bot: I haz the power, 2016-03-29 02:13:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-29 02:14:34]

Description was changed from

==========
[SPv2] Implement CSS clip property

This CL creates clip nodes for CSS clip in PaintPropertyTreeBuilder.
==========

to

==========
[SPv2] Implement CSS clip property

This CL creates clip nodes for CSS clip in PaintPropertyTreeBuilder.

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

[commit-bot: I haz the power, 2016-03-29 02:14:35]

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