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 a union of the subtree's visual overflow rects.
This is similar to LayoutObject::absoluteElementBoundingBoxRect(),
but in local coordinates and float precision.

BUG=554169

[Stephen White, 2015-11-17 16:24:35]

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

[commit-bot: I haz the power, 2015-11-17 16:25:55]

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

[Stephen White, 2015-11-17 19:05:06]

Description was changed from

==========
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 a union of the subtree's visual overflow rects.
This is similar to LayoutObject::absoluteElementBoundingBoxRect(),
but in local coordinates and float precision.

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 a union of the subtree's visual overflow rects.
This is similar to LayoutObject::absoluteElementBoundingBoxRect(),
but in local coordinates and float precision.

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

[Stephen White, 2015-11-17 19:05:07]

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

[Stephen White, 2015-11-17 19:05:31]

senorblanco@chromium.org changed reviewers:
+ fs@opera.com

[commit-bot: I haz the power, 2015-11-17 19:05:56]

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

[commit-bot: I haz the power, 2015-11-17 19:05:56]

Dry run: This issue passed the CQ dry run.

[Stephen White, 2015-11-17 19:06:23]

chrishtr@ || fs@: PTAL. Thanks!

[chrishtr, 2015-11-17 19:14:45]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105:
element->layoutObject()->addElementVisualOverflowRects(rects, LayoutPoint());
addElementVisualOverflowRects doesn't take into account transform scale as far
as I can see, just offset..

[Stephen White, 2015-11-17 19:22:19]

On 2015/11/17 19:14:45, chrishtr wrote:
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> (right):
> 
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105:
> element->layoutObject()->addElementVisualOverflowRects(rects, LayoutPoint());
> addElementVisualOverflowRects doesn't take into account transform scale as far
> as I can see, just offset..

Empirically, borderBoxRect() and visualOverflowRect() do take transforms into 
account, but only on the given element (otherwise the -expected.html case would
be broken too, even without this change). Since I'm computing the union of the 
visual overflow rects of the subtree, it gets the child transforms as well.

[fs, 2015-11-17 20:22:35]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
(Note) I couldn't help but wonder why we'd want to always have this be
"zero-based", but I suppose this is how things currently work (for filtered
HTML) - and the user-space isn't particularly well-defined there either I guess.
Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check what
Gecko does...)

[Stephen White, 2015-11-17 20:44:22]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
On 2015/11/17 20:22:35, fs wrote:
> (Note) I couldn't help but wonder why we'd want to always have this be
> "zero-based", but I suppose this is how things currently work (for filtered
> HTML) - and the user-space isn't particularly well-defined there either I
guess.
> Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check what
> Gecko does...)

The crop rect in Skia (where the filter region ends up) is 
relative to the origin at the time saveLayer() is called. Over in
FilterDisplayItem::Raster(), we translate to the primitive's origin
before calling saveLayer(). We need to do that already because light 
positions are also expressed relative to the origin. So that one 
translate() call positions both the lights and crop rects in the 
correct place.

[fs, 2015-11-17 21:10:48]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> On 2015/11/17 20:22:35, fs wrote:
> > (Note) I couldn't help but wonder why we'd want to always have this be
> > "zero-based", but I suppose this is how things currently work (for filtered
> > HTML) - and the user-space isn't particularly well-defined there either I
guess.
> > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check what
> > Gecko does...)
> 
> The crop rect in Skia (where the filter region ends up) is 
> relative to the origin at the time saveLayer() is called. Over in
> FilterDisplayItem::Raster(), we translate to the primitive's origin
> before calling saveLayer(). We need to do that already because light 
> positions are also expressed relative to the origin. So that one 
> translate() call positions both the lights and crop rects in the 
> correct place.

Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in FilterPainter,
yes? So the origin used here would presumably match up with that. (FWIW, what I
expected in my test was for the cyan rect to cover blue rect - like in the SVG
case. Presumably lights pointing at <0,0> would end up in the top-left of the
filter region too. I'm perfectly fine with that not being dug into further
though...)

[Stephen White, 2015-11-18 17:18:35]

On 2015/11/17 19:22:19, Stephen White - PST wrote:
> On 2015/11/17 19:14:45, chrishtr wrote:
> >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105:
> > element->layoutObject()->addElementVisualOverflowRects(rects,
LayoutPoint());
> > addElementVisualOverflowRects doesn't take into account transform scale as
far
> > as I can see, just offset..
> 
> Empirically, borderBoxRect() and visualOverflowRect() do take transforms into 
> account, but only on the given element (otherwise the -expected.html case
would
> be broken too, even without this change). Since I'm computing the union of the

> visual overflow rects of the subtree, it gets the child transforms as well.

Since I don't know this code that well, I thought I'd dig a bit deeper to prove 
this more conclusively: the transformation matrix is computed during style 
computation, and then applied by this call stack:

LayoutBoxModelObject::addOutlineRectsForDescendant()
LayoutBoxModelObject::addOutlineRectsForNormalChildren()
LayoutBlock::addOutlineRects()
LayoutObject::addElementVisualOverflowRects()

in particular, line 453 of LayoutBoxModelObject::addOutlineRectsForDescendant():


FloatQuad quadInBox =
toLayoutBoxModelObject(descendant).localToContainerQuad(FloatQuad(FloatRect(layerOutlineRects[i])),
this);

That said, as fs@ points out, this really should be the same computation that's
done in
for FilterDisplayItem::bounds_. If you'd prefer I use
PaintLayer::boundingBoxForCompositing(), I could do 
that instead.

[Stephen White, 2015-11-18 22:15:10]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
On 2015/11/17 21:10:48, fs wrote:
> On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > On 2015/11/17 20:22:35, fs wrote:
> > > (Note) I couldn't help but wonder why we'd want to always have this be
> > > "zero-based", but I suppose this is how things currently work (for
filtered
> > > HTML) - and the user-space isn't particularly well-defined there either I
> guess.
> > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check
what
> > > Gecko does...)
> > 
> > The crop rect in Skia (where the filter region ends up) is 
> > relative to the origin at the time saveLayer() is called. Over in
> > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > before calling saveLayer(). We need to do that already because light 
> > positions are also expressed relative to the origin. So that one 
> > translate() call positions both the lights and crop rects in the 
> > correct place.
> 
> Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
FilterPainter,
> yes? So the origin used here would presumably match up with that. (FWIW, what
I
> expected in my test was for the cyan rect to cover blue rect - like in the SVG
> case. Presumably lights pointing at <0,0> would end up in the top-left of the
> filter region too. I'm perfectly fine with that not being dug into further
> though...)

Yeah, basically we should be using the same rect here that we're using 
over there (in FilterPainter::FilterPainter). Part of the motivation 
to move it here (landed in https://codereview.chromium.org/1235293003) 
is that we were using the saveLayer bounds has a hard crop, but this 
isn't the way saveLayer is supposed to work -- the Skia team wants to 
make it a hint, so that you could safely set the bounds to null and 
get the same results. So merging it into the crop rect should allow us 
to do that, presuming we can set the same rect.

[chrishtr, 2015-11-18 22:54:29]

On 2015/11/18 at 22:15:10, senorblanco wrote:
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
> On 2015/11/17 21:10:48, fs wrote:
> > On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > > On 2015/11/17 20:22:35, fs wrote:
> > > > (Note) I couldn't help but wonder why we'd want to always have this be
> > > > "zero-based", but I suppose this is how things currently work (for
filtered
> > > > HTML) - and the user-space isn't particularly well-defined there either
I
> > guess.
> > > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check
what
> > > > Gecko does...)
> > > 
> > > The crop rect in Skia (where the filter region ends up) is 
> > > relative to the origin at the time saveLayer() is called. Over in
> > > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > > before calling saveLayer(). We need to do that already because light 
> > > positions are also expressed relative to the origin. So that one 
> > > translate() call positions both the lights and crop rects in the 
> > > correct place.
> > 
> > Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
FilterPainter,
> > yes? So the origin used here would presumably match up with that. (FWIW,
what I
> > expected in my test was for the cyan rect to cover blue rect - like in the
SVG
> > case. Presumably lights pointing at <0,0> would end up in the top-left of
the
> > filter region too. I'm perfectly fine with that not being dug into further
> > though...)
> 
> Yeah, basically we should be using the same rect here that we're using 
> over there (in FilterPainter::FilterPainter). Part of the motivation 
> to move it here (landed in https://codereview.chromium.org/1235293003) 
> is that we were using the saveLayer bounds has a hard crop, but this 
> isn't the way saveLayer is supposed to work -- the Skia team wants to 
> make it a hint, so that you could safely set the bounds to null and 
> get the same results. So merging it into the crop rect should allow us 
> to do that, presuming we can set the same rect.

Does physicalBoundingBoxIncludingReflectionAndStackingChildren work?

[chrishtr, 2015-11-18 22:55:42]

On 2015/11/18 at 17:18:35, senorblanco wrote:
> On 2015/11/17 19:22:19, Stephen White - PST wrote:
> > On 2015/11/17 19:14:45, chrishtr wrote:
> > >
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> > > (right):
> > > 
> > >
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105:
> > > element->layoutObject()->addElementVisualOverflowRects(rects,
LayoutPoint());
> > > addElementVisualOverflowRects doesn't take into account transform scale as
far
> > > as I can see, just offset..
> > 
> > Empirically, borderBoxRect() and visualOverflowRect() do take transforms
into 
> > account, but only on the given element (otherwise the -expected.html case
would
> > be broken too, even without this change). Since I'm computing the union of
the 
> > visual overflow rects of the subtree, it gets the child transforms as well.
> 
> Since I don't know this code that well, I thought I'd dig a bit deeper to
prove 
> this more conclusively: the transformation matrix is computed during style 
> computation, and then applied by this call stack:
> 
> LayoutBoxModelObject::addOutlineRectsForDescendant()
> LayoutBoxModelObject::addOutlineRectsForNormalChildren()
> LayoutBlock::addOutlineRects()
> LayoutObject::addElementVisualOverflowRects()
> 
> in particular, line 453 of
LayoutBoxModelObject::addOutlineRectsForDescendant(): 
> 
> FloatQuad quadInBox =
toLayoutBoxModelObject(descendant).localToContainerQuad(FloatQuad(FloatRect(layerOutlineRects[i])),
this);

So it's this that takes into account the transform? If so, why does it work for
elements with filter and transform on the same element? The transform is only
applied when going towards parents...

> 
> That said, as fs@ points out, this really should be the same computation
that's done in
> for FilterDisplayItem::bounds_. If you'd prefer I use
PaintLayer::boundingBoxForCompositing(), I could do 
> that instead.

[Stephen White, 2015-11-18 23:13:18]

On 2015/11/18 22:54:29, chrishtr wrote:
> On 2015/11/18 at 22:15:10, senorblanco wrote:
> >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
> referenceBox = FloatRect(FloatPoint(), size);
> > On 2015/11/17 21:10:48, fs wrote:
> > > On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > > > On 2015/11/17 20:22:35, fs wrote:
> > > > > (Note) I couldn't help but wonder why we'd want to always have this be
> > > > > "zero-based", but I suppose this is how things currently work (for
> filtered
> > > > > HTML) - and the user-space isn't particularly well-defined there
either
> I
> > > guess.
> > > > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check
> what
> > > > > Gecko does...)
> > > > 
> > > > The crop rect in Skia (where the filter region ends up) is 
> > > > relative to the origin at the time saveLayer() is called. Over in
> > > > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > > > before calling saveLayer(). We need to do that already because light 
> > > > positions are also expressed relative to the origin. So that one 
> > > > translate() call positions both the lights and crop rects in the 
> > > > correct place.
> > > 
> > > Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
> FilterPainter,
> > > yes? So the origin used here would presumably match up with that. (FWIW,
> what I
> > > expected in my test was for the cyan rect to cover blue rect - like in the
> SVG
> > > case. Presumably lights pointing at <0,0> would end up in the top-left of
> the
> > > filter region too. I'm perfectly fine with that not being dug into further
> > > though...)
> > 
> > Yeah, basically we should be using the same rect here that we're using 
> > over there (in FilterPainter::FilterPainter). Part of the motivation 
> > to move it here (landed in https://codereview.chromium.org/1235293003) 
> > is that we were using the saveLayer bounds has a hard crop, but this 
> > isn't the way saveLayer is supposed to work -- the Skia team wants to 
> > make it a hint, so that you could safely set the bounds to null and 
> > get the same results. So merging it into the crop rect should allow us 
> > to do that, presuming we can set the same rect.
> 
> Does physicalBoundingBoxIncludingReflectionAndStackingChildren work?

That one crashes css3/filters/effect-reference-on-span-crash.html:

#0  logging::CheckOpResult::message (this=0x8)
    at ../../base/memory/scoped_ptr.h:247
#1  0x00005555562b2289 in LocalSharedObjectsContainer::appcaches (this=0x0)
    at ../../chrome/browser/content_settings/local_shared_objects_container.h:41
#2  0x000055555a4c5045 in blink::LayoutObject::hasFlippedBlocksWritingMode (
    this=0x0) at ../../third_party/WebKit/Source/core/layout/LayoutObject.h:655
#3  0x000055555a6a8710 in blink::LayoutBox::flipForWritingMode (this=0x0, 
    rect=...) at ../../third_party/WebKit/Source/core/layout/LayoutBox.h:766
#4  0x000055555a6a2d19 in blink::flippedLogicalBoundingBox (boundingBox=..., 
    layoutObjects=0x1d03db8400b0)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2082
#5  0x000055555a69fe41 in blink::PaintLayer::physicalBoundingBox (
    this=0x1d03db80c400, offsetFromRoot=...)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2095
#6  0x000055555a6a3302 in
blink::PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren
(this=0x1d03db80c400, offsetFromRoot=...)
    at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2143
#7  0x000055555aac0338 in blink::ReferenceFilterBuilder::build (zoom=1, 
    element=0x38febc210340, previousEffect=0x0, filterOperation=...)
    at
../../third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105
#8  0x000055555a6a45e0 in blink::(anonymous
namespace)::computeFilterOperationsHandleReferenceFilters (filters=...,
effectiveZoom=1, enclosingElement=
    0x38febc210340)

Maybe because we're in the middle of lazy attach? (just a WAG -- I'm really
out of my depth here).

[chrishtr, 2015-11-18 23:14:53]

On 2015/11/18 at 23:13:18, senorblanco wrote:
> On 2015/11/18 22:54:29, chrishtr wrote:
> > On 2015/11/18 at 22:15:10, senorblanco wrote:
> > >
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
> > referenceBox = FloatRect(FloatPoint(), size);
> > > On 2015/11/17 21:10:48, fs wrote:
> > > > On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > > > > On 2015/11/17 20:22:35, fs wrote:
> > > > > > (Note) I couldn't help but wonder why we'd want to always have this
be
> > > > > > "zero-based", but I suppose this is how things currently work (for
> > filtered
> > > > > > HTML) - and the user-space isn't particularly well-defined there
either
> > I
> > > > guess.
> > > > > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self:
Check
> > what
> > > > > > Gecko does...)
> > > > > 
> > > > > The crop rect in Skia (where the filter region ends up) is 
> > > > > relative to the origin at the time saveLayer() is called. Over in
> > > > > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > > > > before calling saveLayer(). We need to do that already because light 
> > > > > positions are also expressed relative to the origin. So that one 
> > > > > translate() call positions both the lights and crop rects in the 
> > > > > correct place.
> > > > 
> > > > Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
> > FilterPainter,
> > > > yes? So the origin used here would presumably match up with that. (FWIW,
> > what I
> > > > expected in my test was for the cyan rect to cover blue rect - like in
the
> > SVG
> > > > case. Presumably lights pointing at <0,0> would end up in the top-left
of
> > the
> > > > filter region too. I'm perfectly fine with that not being dug into
further
> > > > though...)
> > > 
> > > Yeah, basically we should be using the same rect here that we're using 
> > > over there (in FilterPainter::FilterPainter). Part of the motivation 
> > > to move it here (landed in https://codereview.chromium.org/1235293003) 
> > > is that we were using the saveLayer bounds has a hard crop, but this 
> > > isn't the way saveLayer is supposed to work -- the Skia team wants to 
> > > make it a hint, so that you could safely set the bounds to null and 
> > > get the same results. So merging it into the crop rect should allow us 
> > > to do that, presuming we can set the same rect.
> > 
> > Does physicalBoundingBoxIncludingReflectionAndStackingChildren work?
> 
> That one crashes css3/filters/effect-reference-on-span-crash.html:
> 
> #0  logging::CheckOpResult::message (this=0x8)
>     at ../../base/memory/scoped_ptr.h:247
> #1  0x00005555562b2289 in LocalSharedObjectsContainer::appcaches (this=0x0)
>     at
../../chrome/browser/content_settings/local_shared_objects_container.h:41
> #2  0x000055555a4c5045 in blink::LayoutObject::hasFlippedBlocksWritingMode (
>     this=0x0) at
../../third_party/WebKit/Source/core/layout/LayoutObject.h:655
> #3  0x000055555a6a8710 in blink::LayoutBox::flipForWritingMode (this=0x0, 
>     rect=...) at ../../third_party/WebKit/Source/core/layout/LayoutBox.h:766
> #4  0x000055555a6a2d19 in blink::flippedLogicalBoundingBox (boundingBox=..., 
>     layoutObjects=0x1d03db8400b0)
>     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2082
> #5  0x000055555a69fe41 in blink::PaintLayer::physicalBoundingBox (
>     this=0x1d03db80c400, offsetFromRoot=...)
>     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2095
> #6  0x000055555a6a3302 in
blink::PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren
(this=0x1d03db80c400, offsetFromRoot=...)
>     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2143
> #7  0x000055555aac0338 in blink::ReferenceFilterBuilder::build (zoom=1, 
>     element=0x38febc210340, previousEffect=0x0, filterOperation=...)
>     at
../../third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105
> #8  0x000055555a6a45e0 in blink::(anonymous
namespace)::computeFilterOperationsHandleReferenceFilters (filters=...,
effectiveZoom=1, enclosingElement=
>     0x38febc210340)
> 
> Maybe because we're in the middle of lazy attach? (just a WAG -- I'm really
> out of my depth here).

Is there a null pointer dereference happening?

[Stephen White, 2015-11-18 23:23:52]

On 2015/11/18 23:14:53, chrishtr wrote:
> On 2015/11/18 at 23:13:18, senorblanco wrote:
> > On 2015/11/18 22:54:29, chrishtr wrote:
> > > On 2015/11/18 at 22:15:10, senorblanco wrote:
> > > >
> > >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > > File
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
> > > (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > > >
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
> > > referenceBox = FloatRect(FloatPoint(), size);
> > > > On 2015/11/17 21:10:48, fs wrote:
> > > > > On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > > > > > On 2015/11/17 20:22:35, fs wrote:
> > > > > > > (Note) I couldn't help but wonder why we'd want to always have
this
> be
> > > > > > > "zero-based", but I suppose this is how things currently work (for
> > > filtered
> > > > > > > HTML) - and the user-space isn't particularly well-defined there
> either
> > > I
> > > > > guess.
> > > > > > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self:
> Check
> > > what
> > > > > > > Gecko does...)
> > > > > > 
> > > > > > The crop rect in Skia (where the filter region ends up) is 
> > > > > > relative to the origin at the time saveLayer() is called. Over in
> > > > > > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > > > > > before calling saveLayer(). We need to do that already because light

> > > > > > positions are also expressed relative to the origin. So that one 
> > > > > > translate() call positions both the lights and crop rects in the 
> > > > > > correct place.
> > > > > 
> > > > > Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
> > > FilterPainter,
> > > > > yes? So the origin used here would presumably match up with that.
(FWIW,
> > > what I
> > > > > expected in my test was for the cyan rect to cover blue rect - like in
> the
> > > SVG
> > > > > case. Presumably lights pointing at <0,0> would end up in the top-left
> of
> > > the
> > > > > filter region too. I'm perfectly fine with that not being dug into
> further
> > > > > though...)
> > > > 
> > > > Yeah, basically we should be using the same rect here that we're using 
> > > > over there (in FilterPainter::FilterPainter). Part of the motivation 
> > > > to move it here (landed in https://codereview.chromium.org/1235293003) 
> > > > is that we were using the saveLayer bounds has a hard crop, but this 
> > > > isn't the way saveLayer is supposed to work -- the Skia team wants to 
> > > > make it a hint, so that you could safely set the bounds to null and 
> > > > get the same results. So merging it into the crop rect should allow us 
> > > > to do that, presuming we can set the same rect.
> > > 
> > > Does physicalBoundingBoxIncludingReflectionAndStackingChildren work?
> > 
> > That one crashes css3/filters/effect-reference-on-span-crash.html:
> > 
> > #0  logging::CheckOpResult::message (this=0x8)
> >     at ../../base/memory/scoped_ptr.h:247
> > #1  0x00005555562b2289 in LocalSharedObjectsContainer::appcaches (this=0x0)
> >     at
> ../../chrome/browser/content_settings/local_shared_objects_container.h:41
> > #2  0x000055555a4c5045 in blink::LayoutObject::hasFlippedBlocksWritingMode (
> >     this=0x0) at
> ../../third_party/WebKit/Source/core/layout/LayoutObject.h:655
> > #3  0x000055555a6a8710 in blink::LayoutBox::flipForWritingMode (this=0x0, 
> >     rect=...) at ../../third_party/WebKit/Source/core/layout/LayoutBox.h:766
> > #4  0x000055555a6a2d19 in blink::flippedLogicalBoundingBox (boundingBox=...,

> >     layoutObjects=0x1d03db8400b0)
> >     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2082
> > #5  0x000055555a69fe41 in blink::PaintLayer::physicalBoundingBox (
> >     this=0x1d03db80c400, offsetFromRoot=...)
> >     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2095
> > #6  0x000055555a6a3302 in
> blink::PaintLayer::physicalBoundingBoxIncludingReflectionAndStackingChildren
> (this=0x1d03db80c400, offsetFromRoot=...)
> >     at ../../third_party/WebKit/Source/core/paint/PaintLayer.cpp:2143
> > #7  0x000055555aac0338 in blink::ReferenceFilterBuilder::build (zoom=1, 
> >     element=0x38febc210340, previousEffect=0x0, filterOperation=...)
> >     at
> ../../third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:105
> > #8  0x000055555a6a45e0 in blink::(anonymous
> namespace)::computeFilterOperationsHandleReferenceFilters (filters=...,
> effectiveZoom=1, enclosingElement=
> >     0x38febc210340)
> > 
> > Maybe because we're in the middle of lazy attach? (just a WAG -- I'm really
> > out of my depth here).
> 
> Is there a null pointer dereference happening?

Yep. See the this=0x0 in hasFlippedBlocksWritingMode. The
layoutObjects->containingBlock()
seems to be null, in flippedLogicalBoundingBox().

[fs, 2015-11-18 23:35:43]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
On 2015/11/18 at 22:15:10, Stephen White - PST wrote:
> On 2015/11/17 21:10:48, fs wrote:
> > On 2015/11/17 at 20:44:21, Stephen White - PST wrote:
> > > On 2015/11/17 20:22:35, fs wrote:
> > > > (Note) I couldn't help but wonder why we'd want to always have this be
> > > > "zero-based", but I suppose this is how things currently work (for
filtered
> > > > HTML) - and the user-space isn't particularly well-defined there either
I
> > guess.
> > > > Testcase pondering: http://jsfiddle.net/v4rxzurc/ (Note-to-self: Check
what
> > > > Gecko does...)
> > > 
> > > The crop rect in Skia (where the filter region ends up) is 
> > > relative to the origin at the time saveLayer() is called. Over in
> > > FilterDisplayItem::Raster(), we translate to the primitive's origin
> > > before calling saveLayer(). We need to do that already because light 
> > > positions are also expressed relative to the origin. So that one 
> > > translate() call positions both the lights and crop rects in the 
> > > correct place.
> > 
> > Right, but (cc::)FilterDisplayItem::bounds_ is what's derived in
FilterPainter,
> > yes? So the origin used here would presumably match up with that. (FWIW,
what I
> > expected in my test was for the cyan rect to cover blue rect - like in the
SVG
> > case. Presumably lights pointing at <0,0> would end up in the top-left of
the
> > filter region too. I'm perfectly fine with that not being dug into further
> > though...)
> 
> Yeah, basically we should be using the same rect here that we're using 
> over there (in FilterPainter::FilterPainter). Part of the motivation 
> to move it here (landed in https://codereview.chromium.org/1235293003) 
> is that we were using the saveLayer bounds has a hard crop, but this 
> isn't the way saveLayer is supposed to work -- the Skia team wants to 
> make it a hint, so that you could safely set the bounds to null and 
> get the same results.

Yeah, that makes sense.

> So merging it into the crop rect should allow us 
> to do that, presuming we can set the same rect.

And then bounds_ could be just an offset_ to get the right origin for the
filter, or? (I see that the ZOOM filter - used by the ui/compositor - needs the
bound dimensions still...)

I think that there could be an issue currently with when this code is called (I
think that the reason for the crashstack/assert) - it could currently be called
during (after) style-change, when neither of the suggested may be valid. I've
been trying to untangle that a bit in the bug I'm currently working on, and
hopefully it will work out in the end... So it seems to me we might be in touch
spot for which method to pick (for now)...

[Stephen White, 2015-11-18 23:47:00]

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):

https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
On 2015/11/18 23:35:43, fs wrote:
> And then bounds_ could be just an offset_ to get the right origin for the
> filter, or? (I see that the ZOOM filter - used by the ui/compositor - needs
the
> bound dimensions still...)

Yes, getting to just an offset would be cool. I'd like to wean the 
zoom filter / SkMagnifierImageFilter off its need for the bounds too; 
I don't think it should need it, conceptually at least.

> I think that there could be an issue currently with when this code is called
(I
> think that the reason for the crashstack/assert) - it could currently be
called
> during (after) style-change, when neither of the suggested may be valid.

It's definitely being called during PaintLayer::styleChanged().

> I've
> been trying to untangle that a bit in the bug I'm currently working on, and
> hopefully it will work out in the end... So it seems to me we might be in
touch
> spot for which method to pick (for now)...

Hmm. That's unfortunate. I'd rather not leave in this regression in 48 
if possible. Do you know what problems / test cases I could look for 
wrt the addElementVisualOverflowRects() flavour? It seems less 
problematic than the flavour using 
physicalBoundingBoxIncludingReflectionAndStackingChildrenAndBearsOhMy().

[chrishtr, 2015-11-19 01:09:21]

On 2015/11/18 at 23:47:00, senorblanco wrote:
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
> On 2015/11/18 23:35:43, fs wrote:
> > And then bounds_ could be just an offset_ to get the right origin for the
> > filter, or? (I see that the ZOOM filter - used by the ui/compositor - needs
the
> > bound dimensions still...)
> 
> Yes, getting to just an offset would be cool. I'd like to wean the 
> zoom filter / SkMagnifierImageFilter off its need for the bounds too; 
> I don't think it should need it, conceptually at least.
> 
> > I think that there could be an issue currently with when this code is called
(I
> > think that the reason for the crashstack/assert) - it could currently be
called
> > during (after) style-change, when neither of the suggested may be valid.
> 
> It's definitely being called during PaintLayer::styleChanged().
> 
> > I've
> > been trying to untangle that a bit in the bug I'm currently working on, and
> > hopefully it will work out in the end... So it seems to me we might be in
touch
> > spot for which method to pick (for now)...
> 
> Hmm. That's unfortunate. I'd rather not leave in this regression in 48 
> if possible. Do you know what problems / test cases I could look for 
> wrt the addElementVisualOverflowRects() flavour? It seems less 
> problematic than the flavour using 
> physicalBoundingBoxIncludingReflectionAndStackingChildrenAndBearsOhMy().

The crashing example you encountered demonstrates why it's just not correct to
be computing positions, or depending on style of children, via callbacks from
styleChanged().
It appears that you only need PaintLayer::filterInfo() to be set during
painting, in particular via call sites from FilterPainter.
How about refactoring as follows:

1. In the styleChanged callback, set a dirty bit that filters need recomputation
2. From FilterPainter, recompute the filter DAG as necessary if the dirty bit is
set

[fs, 2015-11-19 10:15:33]

On 2015/11/19 at 01:09:21, chrishtr wrote:
> On 2015/11/18 at 23:47:00, senorblanco wrote:
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > File third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp
(right):
> > 
> >
https://codereview.chromium.org/1452233002/diff/1/third_party/WebKit/Source/c...
> > third_party/WebKit/Source/core/layout/svg/ReferenceFilterBuilder.cpp:107:
referenceBox = FloatRect(FloatPoint(), size);
> > On 2015/11/18 23:35:43, fs wrote:
> > > And then bounds_ could be just an offset_ to get the right origin for the
> > > filter, or? (I see that the ZOOM filter - used by the ui/compositor -
needs the
> > > bound dimensions still...)
> > 
> > Yes, getting to just an offset would be cool. I'd like to wean the 
> > zoom filter / SkMagnifierImageFilter off its need for the bounds too; 
> > I don't think it should need it, conceptually at least.
> > 
> > > I think that there could be an issue currently with when this code is
called (I
> > > think that the reason for the crashstack/assert) - it could currently be
called
> > > during (after) style-change, when neither of the suggested may be valid.
> > 
> > It's definitely being called during PaintLayer::styleChanged().
> > 
> > > I've
> > > been trying to untangle that a bit in the bug I'm currently working on,
and
> > > hopefully it will work out in the end... So it seems to me we might be in
touch
> > > spot for which method to pick (for now)...
> > 
> > Hmm. That's unfortunate. I'd rather not leave in this regression in 48 
> > if possible.

Yes, that wouldn't be great...

> > Do you know what problems / test cases I could look for 
> > wrt the addElementVisualOverflowRects() flavour? It seems less 
> > problematic than the flavour using 
> > physicalBoundingBoxIncludingReflectionAndStackingChildrenAndBearsOhMy().
> 
> The crashing example you encountered demonstrates why it's just not correct to
be computing positions, or depending on style of children, via callbacks from
styleChanged().
> It appears that you only need PaintLayer::filterInfo() to be set during
painting, in particular via call sites from FilterPainter.
> How about refactoring as follows:
> 
> 1. In the styleChanged callback, set a dirty bit that filters need
recomputation
> 2. From FilterPainter, recompute the filter DAG as necessary if the dirty bit
is set

I think there's potentially a "but" here, since computing the outsets require
computing/validating the filter (chain) - so FilterPainter will (could) be too
late. Otherwise a "dirty bit" could probably just be the cached effect chain
itself.

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

On 2015/11/19 01:09:21, chrishtr wrote:
> The crashing example you encountered demonstrates why it's just not correct to
> be computing positions, or depending on style of children, via callbacks from
> styleChanged().
> It appears that you only need PaintLayer::filterInfo() to be set during
> painting, in particular via call sites from FilterPainter.
> How about refactoring as follows:
> 
> 1. In the styleChanged callback, set a dirty bit that filters need
recomputation
> 2. From FilterPainter, recompute the filter DAG as necessary if the dirty bit
is
> set

I gave this a try over here: https://codereview.chromium.org/1459953002/

This works, but as fs@ guessed, the filter outsets are problematic. I've put in
a hack to update them, but it feels ugly and fragile. There's also a callsite
in LayoutBox that I'm unsure about (although it doesn't cause any css3/filters 
tests to fail).

[fs, 2015-11-19 16:29:52]

On 2015/11/19 at 16:13:40, senorblanco wrote:
> On 2015/11/19 01:09:21, chrishtr wrote:
> > The crashing example you encountered demonstrates why it's just not correct
to
> > be computing positions, or depending on style of children, via callbacks
from
> > styleChanged().
> > It appears that you only need PaintLayer::filterInfo() to be set during
> > painting, in particular via call sites from FilterPainter.
> > How about refactoring as follows:
> > 
> > 1. In the styleChanged callback, set a dirty bit that filters need
recomputation
> > 2. From FilterPainter, recompute the filter DAG as necessary if the dirty
bit is
> > set
> 
> I gave this a try over here: https://codereview.chromium.org/1459953002/
> 
> This works, but as fs@ guessed, the filter outsets are problematic. I've put
in
> a hack to update them, but it feels ugly and fragile. There's also a callsite
> in LayoutBox that I'm unsure about (although it doesn't cause any css3/filters

> tests to fail).

Maybe you could make that look slightly better by moving the outset computation
into PaintLayer (and create the FilterEffectBuilder
updateOrRemoveFilterEffectBuilder but reset it's state.)

[fs, 2015-11-19 17:18:58]

On 2015/11/19 at 16:29:52, fs wrote:
> On 2015/11/19 at 16:13:40, senorblanco wrote:
> > On 2015/11/19 01:09:21, chrishtr wrote:
> > > The crashing example you encountered demonstrates why it's just not
correct to
> > > be computing positions, or depending on style of children, via callbacks
from
> > > styleChanged().
> > > It appears that you only need PaintLayer::filterInfo() to be set during
> > > painting, in particular via call sites from FilterPainter.
> > > How about refactoring as follows:
> > > 
> > > 1. In the styleChanged callback, set a dirty bit that filters need
recomputation
> > > 2. From FilterPainter, recompute the filter DAG as necessary if the dirty
bit is
> > > set
> > 
> > I gave this a try over here: https://codereview.chromium.org/1459953002/
> > 
> > This works, but as fs@ guessed, the filter outsets are problematic. I've put
in
> > a hack to update them, but it feels ugly and fragile. There's also a
callsite
> > in LayoutBox that I'm unsure about (although it doesn't cause any
css3/filters 
> > tests to fail).
> 
> Maybe you could make that look slightly better by moving the outset
computation into PaintLayer (and create the FilterEffectBuilder
updateOrRemoveFilterEffectBuilder but reset it's state.)

Uploaded a sketch of what I was thinking of:
https://codereview.chromium.org/1455213006 (untested beyond compilation.)

[Stephen White, 2015-11-19 17:19:03]

On 2015/11/19 16:29:52, fs wrote:
> On 2015/11/19 at 16:13:40, senorblanco wrote:
> > On 2015/11/19 01:09:21, chrishtr wrote:
> > > The crashing example you encountered demonstrates why it's just not
correct
> to
> > > be computing positions, or depending on style of children, via callbacks
> from
> > > styleChanged().
> > > It appears that you only need PaintLayer::filterInfo() to be set during
> > > painting, in particular via call sites from FilterPainter.
> > > How about refactoring as follows:
> > > 
> > > 1. In the styleChanged callback, set a dirty bit that filters need
> recomputation
> > > 2. From FilterPainter, recompute the filter DAG as necessary if the dirty
> bit is
> > > set
> > 
> > I gave this a try over here: https://codereview.chromium.org/1459953002/
> > 
> > This works, but as fs@ guessed, the filter outsets are problematic. I've put
> in
> > a hack to update them, but it feels ugly and fragile. There's also a
callsite
> > in LayoutBox that I'm unsure about (although it doesn't cause any
css3/filters
> 
> > tests to fail).
> 
> Maybe you could make that look slightly better by moving the outset
computation
> into PaintLayer (and create the FilterEffectBuilder
> updateOrRemoveFilterEffectBuilder but reset it's state.)

It's tricky to move all of the filter outset computation, since the compositor
path uses the outsets directly from style.filters.outsets(). In the most recent
patch on that issue, I centralized the ugly in a single function on PaintLayer,
but that still feels fragile (esp. for the LayoutBox call, which I haven't
touched).

I've uploaded a new patch to this issue using
physicalBoundingBoxIncludingReflectionAndStackingChildren(),
and adding a nullptr check for the <span> case. This feels less intrusive to me,
and a
better candidate for 48. Let me know what you think.

[chrishtr, 2015-11-19 17:21:48]

On 2015/11/19 at 17:19:03, senorblanco wrote:
> On 2015/11/19 16:29:52, fs wrote:
> > On 2015/11/19 at 16:13:40, senorblanco wrote:
> > > On 2015/11/19 01:09:21, chrishtr wrote:
> > > > The crashing example you encountered demonstrates why it's just not
correct
> > to
> > > > be computing positions, or depending on style of children, via callbacks
> > from
> > > > styleChanged().
> > > > It appears that you only need PaintLayer::filterInfo() to be set during
> > > > painting, in particular via call sites from FilterPainter.
> > > > How about refactoring as follows:
> > > > 
> > > > 1. In the styleChanged callback, set a dirty bit that filters need
> > recomputation
> > > > 2. From FilterPainter, recompute the filter DAG as necessary if the
dirty
> > bit is
> > > > set
> > > 
> > > I gave this a try over here: https://codereview.chromium.org/1459953002/
> > > 
> > > This works, but as fs@ guessed, the filter outsets are problematic. I've
put
> > in
> > > a hack to update them, but it feels ugly and fragile. There's also a
callsite
> > > in LayoutBox that I'm unsure about (although it doesn't cause any
css3/filters
> > 
> > > tests to fail).
> > 
> > Maybe you could make that look slightly better by moving the outset
computation
> > into PaintLayer (and create the FilterEffectBuilder
> > updateOrRemoveFilterEffectBuilder but reset it's state.)
> 
> It's tricky to move all of the filter outset computation, since the compositor
> path uses the outsets directly from style.filters.outsets(). In the most
recent
> patch on that issue, I centralized the ugly in a single function on
PaintLayer,
> but that still feels fragile (esp. for the LayoutBox call, which I haven't
> touched).
> 
> I've uploaded a new patch to this issue using
physicalBoundingBoxIncludingReflectionAndStackingChildren(),
> and adding a nullptr check for the <span> case. This feels less intrusive to
me, and a
> better candidate for 48. Let me know what you think.

I think the other CL is better, and IMO is safe enough to merge to 48.

[Stephen White, 2015-11-20 16:42:59]

Closing this in favour of https://codereview.chromium.org/1459953002/, which has
landed.