Fix CSS reference filters with negative transformed children.

Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the
bounds top-left corner as the origin for filters. This
doesn't work if the filtered primitive contains a
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which affect the size of the layer, but are not
supposed to affect layout. In cc, we had previously been
using the layer's src_rect origin to offset filter
parameters in order to compensate for clipping, etc, but
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits, cc::Layer, cc::LayerImpl, EffectNode,
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I
was kidding.)

BUG=638091
R=ajuma@chromium.org, chrishtr@chromium.org, mbarbella@chromium.org, sky@chromium.org, wkorman@chromium.org
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/5941ecb0aea741b27fea283065b25428588dbe43
Cr-Commit-Position: refs/heads/master@{#415043}

[Stephen White, 2016-08-26 01:22:35]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

BUG=638091
==========

to

==========
Fix CSS reference filters with negative transformed children.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Stephen White, 2016-08-26 01:22:35]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Fix CSS reference filters with negative transformed children.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Stephen White, 2016-08-26 01:23:26]

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

[commit-bot: I haz the power, 2016-08-26 01:23:59]

Dry run: 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-08-26 01:39:13]

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

[commit-bot: I haz the power, 2016-08-26 01:39:14]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Stephen White, 2016-08-26 14:14:46]

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

[commit-bot: I haz the power, 2016-08-26 14:15:02]

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

[Stephen White, 2016-08-26 14:17:12]

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

[commit-bot: I haz the power, 2016-08-26 14:17:31]

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

[Stephen White, 2016-08-26 15:11:00]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Stephen White, 2016-08-26 15:11:44]

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

[commit-bot: I haz the power, 2016-08-26 15:12:13]

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

[Stephen White, 2016-08-26 15:13:10]

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

[Stephen White, 2016-08-26 15:16:51]

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

[Stephen White, 2016-08-26 15:19:18]

senorblanco@chromium.org changed reviewers:
- ajuma@chromium.org

[Stephen White, 2016-08-26 15:19:35]

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

[Stephen White, 2016-08-26 15:20:16]

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

[Stephen White, 2016-08-26 15:21:56]

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

[Stephen White, 2016-08-26 15:24:48]

PTAL.

ajuma@: cc/*
christhtr@: third_party/WebKit/*

Thanks!

[Stephen White, 2016-08-26 16:07:57]

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

[commit-bot: I haz the power, 2016-08-26 16:08:40]

Dry run: 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-08-26 17:07:34]

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

[commit-bot: I haz the power, 2016-08-26 17:07:35]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[ajuma, 2016-08-26 17:30:07]

cc/ lgtm

[wkorman, 2016-08-26 17:45:51]

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

[wkorman, 2016-08-26 17:45:52]

One minor comment, the rest looks good to me.

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:75: canvas->translate(-origin_.x(),
-origin_.y());
Are we still using bounds_.x/y anywhere? If not maybe we should change it to a
Size for clarity, for example I think this would mean we wouldn't do this
moveBy() unnecessarily any more:

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/paint/Fil...

[Stephen White, 2016-08-26 19:09:27]

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

[commit-bot: I haz the power, 2016-08-26 19:10:07]

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

[Stephen White, 2016-08-26 19:22:02]

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:75: canvas->translate(-origin_.x(),
-origin_.y());
On 2016/08/26 17:45:52, wkorman wrote:
> Are we still using bounds_.x/y anywhere? If not maybe we should change it to a
> Size for clarity, for example I think this would mean we wouldn't do this
> moveBy() unnecessarily any more:

Yes, we still need a full rect as well as an origin. bounds 
represents the bounding rectangle of the original primitive and 
all its children. origin represents the top/left of the original 
primitive within those bounds, excluding children. So if the parent 
has a negatively transformed child node, (bounds.location() - origin) 
will be non-zero, which we need to pass to saveLayer().

(Notice that origin is used both to translate the CTM to the right 
place, and also to offset the saveLayer() boundaries at line 68).)

[Stephen White, 2016-08-26 19:23:29]

+jbroman as an FYI on the changes to BoxReflectionUtils.

Looks like you were working around this bug, which should
no longer be necessary.

[jbroman, 2016-08-26 19:24:46]

On 2016/08/26 at 19:23:29, senorblanco wrote:
> +jbroman as an FYI on the changes to BoxReflectionUtils.
> 
> Looks like you were working around this bug, which should
> no longer be necessary.

\o/

[wkorman, 2016-08-26 19:35:25]

lgtm

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/2278273004/diff/100001/cc/playback/filter_dis...
cc/playback/filter_display_item.cc:75: canvas->translate(-origin_.x(),
-origin_.y());
On 2016/08/26 19:22:01, Stephen White wrote:
> On 2016/08/26 17:45:52, wkorman wrote:
> > Are we still using bounds_.x/y anywhere? If not maybe we should change it to
a
> > Size for clarity, for example I think this would mean we wouldn't do this
> > moveBy() unnecessarily any more:
> 
> Yes, we still need a full rect as well as an origin. bounds 
> represents the bounding rectangle of the original primitive and 
> all its children. origin represents the top/left of the original 
> primitive within those bounds, excluding children. So if the parent 
> has a negatively transformed child node, (bounds.location() - origin) 
> will be non-zero, which we need to pass to saveLayer().
> 
> (Notice that origin is used both to translate the CTM to the right 
> place, and also to offset the saveLayer() boundaries at line 68).)

Ah, I searched for bounds_ and missed boundaries!

[chrishtr, 2016-08-26 19:54:26]

lgtm

[commit-bot: I haz the power, 2016-08-26 20:19:25]

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

[commit-bot: I haz the power, 2016-08-26 20:19:26]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[Stephen White, 2016-08-26 23:35:08]

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

[commit-bot: I haz the power, 2016-08-26 23:35:38]

Dry run: 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-08-27 01:09:36]

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

[commit-bot: I haz the power, 2016-08-27 01:09:37]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Stephen White, 2016-08-27 23:28:19]

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

[commit-bot: I haz the power, 2016-08-27 23:28:28]

Dry run: 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-08-27 23:30:38]

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

[commit-bot: I haz the power, 2016-08-27 23:30:40]

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

[Stephen White, 2016-08-27 23:51:49]

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

[commit-bot: I haz the power, 2016-08-27 23:52:06]

Dry run: 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-08-28 00:20:07]

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

[commit-bot: I haz the power, 2016-08-28 00:20:09]

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

[Stephen White, 2016-08-28 00:35:31]

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

[commit-bot: I haz the power, 2016-08-28 00:35:45]

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

[Stephen White, 2016-08-28 00:41:01]

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

[commit-bot: I haz the power, 2016-08-28 00:41:12]

Dry run: 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-08-28 00:42:59]

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

[commit-bot: I haz the power, 2016-08-28 00:43:00]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Stephen White, 2016-08-28 01:30:00]

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

[commit-bot: I haz the power, 2016-08-28 01:30:11]

Dry run: 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-08-28 03:26:33]

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

[commit-bot: I haz the power, 2016-08-28 03:26:34]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Stephen White, 2016-08-29 19:03:19]

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

[Stephen White, 2016-08-29 19:21:57]

senorblanco@chromium.org changed reviewers:
+ enne@chromium.org, jschuh@chromium.org

[Stephen White, 2016-08-29 19:23:49]

senorblanco@chromium.org changed reviewers:
+ mbarbella@chromium.org
- enne@chromium.org, jschuh@chromium.org

[Stephen White, 2016-08-29 19:23:58]

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

[Stephen White, 2016-08-29 19:28:18]

The CQ bit was unchecked by senorblanco@chromium.org

[Stephen White, 2016-08-29 19:28:27]

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

[Stephen White, 2016-08-29 19:40:35]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

On the cc side, it turns out that there are certain 
CSS properties (e.g., "outline") which affect the
size of the layer, but are not supposed to affect
layout. This delta is computed and sent to
GraphicsLayer::setOffsetFromLayoutObject(), but was
not being forwarded to cc. I've done so, with the
corresponding changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer,
cc::LayerImpl, EffectNode, RenderSurfaceImpl, and
RenderPassDrawQuad. (I wish I was kidding.)

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-29 19:45:12]

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

[Stephen White, 2016-08-29 19:47:28]

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

[Stephen White, 2016-08-29 20:03:03]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

On the cc side, it turns out that there are certain 
CSS properties (e.g., "outline") which affect the
size of the layer, but are not supposed to affect
layout. This delta is computed and sent to
GraphicsLayer::setOffsetFromLayoutObject(), but was
not being forwarded to cc. I've done so, with the
corresponding changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer,
cc::LayerImpl, EffectNode, RenderSurfaceImpl, and
RenderPassDrawQuad. (I wish I was kidding.)

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not 
supposed to affect layout. In cc, we had previously been 
using the layer's src_rect origin to offset filter 
parameters in order to compensate for clipping, etc, but 
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode, 
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I 
was kidding.)

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Stephen White, 2016-08-29 20:05:26]

After playing whack-a-mole with a series of bugs, this
now should be passing all layout tests. It required plumbing
through the filter origin to cc, which affected ~29 files (!)
Ali, could you take another look?

Marty, could you look at the IPC changes?

Thanks!

[Martin Barbella, 2016-08-29 20:26:47]

On 2016/08/29 20:05:26, Stephen White wrote:
> After playing whack-a-mole with a series of bugs, this
> now should be passing all layout tests. It required plumbing
> through the filter origin to cc, which affected ~29 files (!)
> Ali, could you take another look?
> 
> Marty, could you look at the IPC changes?
> 
> Thanks!

IPC/mojom lgtm.

[ajuma, 2016-08-29 20:28:43]

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1138: layer->SetFiltersOrigin(inputs_.filters_origin);
This shouldn't be needed (since filters_origin will get pushed with the effect
tree).

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:583: gfx::PointF filters_origin_;
Please add this to LayerImplTestProperties instead.

https://codereview.chromium.org/2278273004/diff/300001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:3178: 
Does it make sense to add a new pixel test (or some other kind of unit test)
that uses a non-zero filters_origin?

[Stephen White, 2016-08-29 21:10:54]

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc#new...
cc/layers/layer.cc:485: SetSubtreePropertyChanged();  // TODO(senorblanco) or
SetLayerPropertyChanged?
Ali: any comment on this TODO? I notice that SetFilters() uses
SetSubtreePropertyChanged(), but SetBackgroundFilters() uses
SetLayerPropertyChanged(). Any idea which one I should be using?

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc#new...
cc/layers/layer.cc:1138: layer->SetFiltersOrigin(inputs_.filters_origin);
On 2016/08/29 20:28:43, ajuma wrote:
> This shouldn't be needed (since filters_origin will get pushed with the effect
> tree).

Removed.

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer_impl.h...
cc/layers/layer_impl.h:583: gfx::PointF filters_origin_;
On 2016/08/29 20:28:43, ajuma wrote:
> Please add this to LayerImplTestProperties instead.

Done.

https://codereview.chromium.org/2278273004/diff/300001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:3178: 
On 2016/08/29 20:28:43, ajuma wrote:
> Does it make sense to add a new pixel test (or some other kind of unit test)
> that uses a non-zero filters_origin?

Done. This revealed that I forgot about the software compositor, now fixed.

[Stephen White, 2016-08-29 21:33:10]

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

[ajuma, 2016-08-29 21:39:30]

Thanks, lgtm

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc#new...
cc/layers/layer.cc:485: SetSubtreePropertyChanged();  // TODO(senorblanco) or
SetLayerPropertyChanged?
On 2016/08/29 21:10:54, Stephen White wrote:
> Ali: any comment on this TODO? I notice that SetFilters() uses
> SetSubtreePropertyChanged(), but SetBackgroundFilters() uses
> SetLayerPropertyChanged(). Any idea which one I should be using?

It should be SetSubtreePropertyChanged (since the dirty region on screen depends
on the bounds of all descendants). I think the difference for background filters
is that the dirty region depends only on the bounds of the layer itself (and on
the filter).

[Stephen White, 2016-08-29 21:43:11]

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc
File cc/layers/layer.cc (right):

https://codereview.chromium.org/2278273004/diff/300001/cc/layers/layer.cc#new...
cc/layers/layer.cc:485: SetSubtreePropertyChanged();  // TODO(senorblanco) or
SetLayerPropertyChanged?
On 2016/08/29 21:39:30, ajuma wrote:
> On 2016/08/29 21:10:54, Stephen White wrote:
> > Ali: any comment on this TODO? I notice that SetFilters() uses
> > SetSubtreePropertyChanged(), but SetBackgroundFilters() uses
> > SetLayerPropertyChanged(). Any idea which one I should be using?
> 
> It should be SetSubtreePropertyChanged (since the dirty region on screen
depends
> on the bounds of all descendants). I think the difference for background
filters
> is that the dirty region depends only on the bounds of the layer itself (and
on
> the filter).

Thanks! TODO removed.

[Stephen White, 2016-08-29 21:45:19]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not 
supposed to affect layout. In cc, we had previously been 
using the layer's src_rect origin to offset filter 
parameters in order to compensate for clipping, etc, but 
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode, 
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I 
was kidding.)

BUG=638091
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not 
supposed to affect layout. In cc, we had previously been 
using the layer's src_rect origin to offset filter 
parameters in order to compensate for clipping, etc, but 
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode, 
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I 
was kidding.)

BUG=638091
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[Stephen White, 2016-08-29 21:45:30]

The CQ bit was checked by senorblanco@chromium.org

[Stephen White, 2016-08-29 21:45:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, wkorman@chromium.org, mbarbella@chromium.org,
ajuma@chromium.org
Link to the patchset: https://codereview.chromium.org/2278273004/#ps340001
(title: "Remove TODO.")

[commit-bot: I haz the power, 2016-08-29 22:20:35]

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

[sky, 2016-08-29 22:59:58]

I'm assuming you added me for services/ui/ws/frame_generator.cc , which LGTM

[Stephen White, 2016-08-30 01:32:37]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not 
supposed to affect layout. In cc, we had previously been 
using the layer's src_rect origin to offset filter 
parameters in order to compensate for clipping, etc, but 
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode, 
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I 
was kidding.)

BUG=638091
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the
bounds top-left corner as the origin for filters. This
doesn't work if the filtered primitive contains a
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not
supposed to affect layout. In cc, we had previously been
using the layer's src_rect origin to offset filter
parameters in order to compensate for clipping, etc, but
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode,
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I
was kidding.)

BUG=638091
R=ajuma@chromium.org, chrishtr@chromium.org, mbarbella@chromium.org,
sky@chromium.org, wkorman@chromium.org
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed:
https://chromium.googlesource.com/chromium/src/+/5941ecb0aea741b27fea283065b2...
==========

[commit-bot: I haz the power, 2016-08-30 01:32:37]

Description was changed from

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the 
bounds top-left corner as the origin for filters. This 
doesn't work if the filtered primitive contains a 
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the 
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not 
supposed to affect layout. In cc, we had previously been 
using the layer's src_rect origin to offset filter 
parameters in order to compensate for clipping, etc, but 
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode, 
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I 
was kidding.)

BUG=638091
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix CSS reference filters with negative transformed children.

The software raster path for filters had always used the
bounds top-left corner as the origin for filters. This
doesn't work if the filtered primitive contains a
negative-transformed child, since the saveLayer() bounds
should be negative (non-zero) with respect to the
primitive.

The solution is to pass through the origin explicitly, and
translate to that rather than the bounds origin before
applying the filter. This will determine the origin for
crop rects, lighting positions, etc.

It turns out that there are certain CSS properties (e.g., "outline") which
affect the size of the layer, but are not
supposed to affect layout. In cc, we had previously been
using the layer's src_rect origin to offset filter
parameters in order to compensate for clipping, etc, but
this is insufficient to handle "outset", which shows up
in the layer position but shouldn't affect filter origin.

The offset that we want was already being computed
for us in GraphicsLayer::setOffsetFromLayoutObject()),
but not forwarded to cc. I've done so, with the
attendant changes to WebLayer, WebLayerImpl, mojo bindings, struct_traits,
cc::Layer, cc::LayerImpl, EffectNode,
RenderSurfaceImpl, and RenderPassDrawQuad. (I wish I
was kidding.)

BUG=638091
R=ajuma@chromium.org, chrishtr@chromium.org, mbarbella@chromium.org,
sky@chromium.org, wkorman@chromium.org
TBR=sky@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/5941ecb0aea741b27fea283065b25428588dbe43
Cr-Commit-Position: refs/heads/master@{#415043}
==========

[commit-bot: I haz the power, 2016-08-30 01:32:39]

Patchset 18 (id:??) landed as
https://crrev.com/5941ecb0aea741b27fea283065b25428588dbe43
Cr-Commit-Position: refs/heads/master@{#415043}

[Stephen White, 2016-08-30 01:32:40]

Committed patchset #18 (id:340001) manually as
5941ecb0aea741b27fea283065b25428588dbe43 (presubmit successful).