Fix the reference box computation for CSS filter effects.

Fix the reference box computation for CSS filter effects.

Previously, we had been using the element's border bounding box, but
this does not take transforms applied to child elements into account.
The fix is to use
PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren.

In order to prevent calling this method at the wrong time in the
document lifecycle, this patch defers creation of the
FilterEffectBuilder until after style computation.

BUG=554169
Committed: https://crrev.com/97aae274311bc2da4388dedf9c25e26692ff1f98
Cr-Commit-Position: refs/heads/master@{#360692}

[Stephen White, 2015-11-19 16:13:57]

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

[commit-bot: I haz the power, 2015-11-19 16:14:41]

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

[Stephen White, 2015-11-19 16:42:15]

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

[commit-bot: I haz the power, 2015-11-19 16:42:40]

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

[commit-bot: I haz the power, 2015-11-19 17:01:45]

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

[commit-bot: I haz the power, 2015-11-19 17:01:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[chrishtr, 2015-11-19 17:20:43]

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

[chrishtr, 2015-11-19 17:20:43]

This approach looks generally fine, modulo code cleanup.

I checked, and it all of the use cases for filters or filter outsets are in
paint invalidation, paint or scrolling code (*)

In all of these cases, the document lifecycle will have advance to style, layout
and compositing clean by the the uses cases need to be executed. Therefore it's
safe to call physicalBoundingBoxIncludingReflectionAndStackingChildren, which
depends on layout and compositing information.

You should asserts to this that the DocumentLifecycle is at a state >=
CompositingClean to enforce this.


(*) hasFilterOutsets is called from FrameView scrolling code

[Stephen White, 2015-11-19 19:02:53]

On 2015/11/19 17:20:43, chrishtr wrote:
> This approach looks generally fine, modulo code cleanup.

The const mess has been cleaned up (inspired by fs@'s patch).

> I checked, and it all of the use cases for filters or filter outsets are in
> paint invalidation, paint or scrolling code (*)
> 
> In all of these cases, the document lifecycle will have advance to style,
layout
> and compositing clean by the the uses cases need to be executed. Therefore
it's
> safe to call physicalBoundingBoxIncludingReflectionAndStackingChildren, which
> depends on layout and compositing information.
> 
> You should asserts to this that the DocumentLifecycle is at a state >=
> CompositingClean to enforce this.

Done.

> (*) hasFilterOutsets is called from FrameView scrolling code

[chrishtr, 2015-11-19 19:12:05]

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

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2629: filterEffectBuilder();
The naming is weird here. ensureFilterEffectBuilder()?

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:425: FilterEffectBuilder*
filterEffectBuilder() const;
I think you don't actually need filterEffectBuilder() to be public, just
lastEffect()

[Stephen White, 2015-11-19 19:34:51]

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

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2601:
ASSERT(layoutObject()->document().lifecycle().state() >=
DocumentLifecycle::CompositingClean);
Problem: this assert seems to be firing on many tests (e.g.,
css3/filters/filter-change-repaint.html).

e.g.,

#0  0x000055555a6a4731 in blink::PaintLayer::filterEffectBuilder (
    this=0x10739580c400)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2616
#1  0x000055555a69e550 in blink::PaintLayer::filterOutsets (
    this=0x10739580c400)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2631
#2  0x000055555a6a31c8 in blink::PaintLayer::boundingBoxForCompositing (
    this=0x10739580c400, ancestorLayer=0x10739580c400, 
    options=blink::PaintLayer::NeverIncludeTransformForAncestorLayer)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2198
#3  0x000055555a6a2f28 in
blink::PaintLayer::boundingBoxForCompositingOverlapTest (this=0x10739580c400)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2114
#4  0x000055555aaa910f in blink::CompositingInputsUpdater::updateRecursive (
    this=0x7fffc3c563d8, layer=0x10739580c400, 
    updateType=blink::CompositingInputsUpdater::ForceUpdate, info=...)
    at
../../third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:116
#5  0x000055555aaa9763 in blink::CompositingInputsUpdater::updateRecursive (
    this=0x7fffc3c563d8, layer=0x10739580c160, 
    updateType=blink::CompositingInputsUpdater::ForceUpdate, info=...)
    at
../../third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:178
#6  0x000055555aaa9763 in blink::CompositingInputsUpdater::updateRecursive (
    this=0x7fffc3c563d8, layer=0x10739580c010, 
    updateType=blink::CompositingInputsUpdater::ForceUpdate, info=...)
    at
../../third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:178
#7  0x000055555aaa8f0e in blink::CompositingInputsUpdater::update (
    this=0x7fffc3c563d8)
    at
../../third_party/WebKit/Source/core/layout/compositing/CompositingInputsUpdater.cpp:29
#8  0x000055555aa34fb1 in blink::PaintLayerCompositor::updateIfNeeded (
    this=0x3889486d18d0)
    at
../../third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:367
#9  0x000055555aa34bda in blink::PaintLayerCompositor::updateIfNeededRecursive
    (this=0x3889486d18d0)
    at
../../third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp:231
#10 0x000055555a4bb590 in blink::FrameView::updateLifecyclePhasesInternal (
    this=0x388948738010, 
    phases=blink::FrameView::OnlyUpToCompositingCleanPlusScrolling)
    at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2384
#11 0x000055555a4bb812 in
blink::FrameView::updateLifecycleToCompositingCleanPlusScrolling
(this=0x388948738010)
    at ../../third_party/WebKit/Source/core/frame/FrameView.cpp:2354
#12 0x000055555a66f3d1 in
blink::PageAnimator::updateLifecycleToCompositingCleanPlusScrolling
(this=0x388948649fb0, rootFrame=...)
    at ../../third_party/WebKit/Source/core/page/PageAnimator.cpp:94
#13 0x0000555559639795 in
blink::PageWidgetDelegate::updateLifecycleToCompositingCleanPlusScrolling
(page=..., root=...)
    at ../../third_party/WebKit/Source/web/PageWidgetDelegate.cpp:63
#14 0x00005555595f45ba in blink::WebViewImpl::updateAllLifecyclePhases (
    this=0x388948668010)
    at ../../third_party/WebKit/Source/web/WebViewImpl.cpp:1920
#15 0x000055555cd7f351 in content::RenderWidgetCompositor::UpdateLayerTreeHost
    (this=0x2ca8683810c0)

[Stephen White, 2015-11-19 19:53:28]

Had to remove the document lifecycle assert for now.

Let me know if there's a way to fix it.

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

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2629: filterEffectBuilder();
On 2015/11/19 19:12:05, chrishtr wrote:
> The naming is weird here. ensureFilterEffectBuilder()?

Changed to updateFilterEffectBuilder() (ensure usually promises that it's
non-null).

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/paint/PaintLayer.h (right):

https://codereview.chromium.org/1459953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/paint/PaintLayer.h:425: FilterEffectBuilder*
filterEffectBuilder() const;
On 2015/11/19 19:12:05, chrishtr wrote:
> I think you don't actually need filterEffectBuilder() to be public, just
> lastEffect()

Done.

[Stephen White, 2015-11-19 20:06:29]

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

[commit-bot: I haz the power, 2015-11-19 20:08:30]

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

[Stephen White, 2015-11-19 20:55:49]

Description was changed from

==========
Fix the reference box computation for CSS filter effects.

WIP: this is a riff on https://codereview.chromium.org/1452233002/
which uses physicalBoundingBoxIncludingReflectionAndStackingChildren.
In order to get that to work without crashing, we defer the creation
of ReferenceFilterBuilder until filter outsets are needed. Since
those calls are often in const methods, we have to const-cast the
layer. :(

Previously, we had been using the element's border bounding box, but
this does not take transforms applied to child elements into account.
The fix is to use
PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren.

BUG=554169
==========

to

==========
Fix the reference box computation for CSS filter effects.

Previously, we had been using the element's border bounding box, but
this does not take transforms applied to child elements into account.
The fix is to use
PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren.

In order to prevent calling this method at the wrong time in the 
document lifecycle, this patch defers creation of the 
FilterEffectBuilder until after style computation.

BUG=554169
==========

[chrishtr, 2015-11-19 21:02:27]

Sad times, the compositing overlap test depends on compositing state. Forgot
about that bad bad situation...

https://codereview.chromium.org/1459953002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1459953002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2601: if
(!paintsWithFilters())
Add a TODO(chrishtr) to assert that compositing is clean here.

[chrishtr, 2015-11-19 21:02:45]

lgtm

Looks good modulo the requested documentation & comments.

[Stephen White, 2015-11-19 21:06:31]

https://codereview.chromium.org/1459953002/diff/110001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right):

https://codereview.chromium.org/1459953002/diff/110001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:2601: if
(!paintsWithFilters())
On 2015/11/19 21:02:27, chrishtr wrote:
> Add a TODO(chrishtr) to assert that compositing is clean here.

Done.

[Stephen White, 2015-11-19 21:06:35]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2015-11-19 21:06:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/1459953002/#ps130001
(title: "Add TODO per review")

[commit-bot: I haz the power, 2015-11-19 21:09:45]

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

[commit-bot: I haz the power, 2015-11-19 23:15:41]

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

[commit-bot: I haz the power, 2015-11-19 23:15:42]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)

[Stephen White, 2015-11-19 23:25:37]

The CQ bit was checked by senorblanco@chromium.org

[commit-bot: I haz the power, 2015-11-19 23:27:40]

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

[commit-bot: I haz the power, 2015-11-20 00:12:12]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2015-11-20 00:13:27]

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