blink: Add backdrop-filter support

blink: Add backdrop-filter support

BUG=497522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201314

[hendrikw, 2015-08-19 23:37:45]

hendrikw@chromium.org changed reviewers:
+ pdr@chromium.org

[hendrikw, 2015-08-19 23:37:46]

Much of this is "Do what filters do", which might be wrong in some situations.

PTAL

It's not done yet, I'm still trying to figure out a few things, maybe you can
help.

Some questions:

1. FilterStyleInterpolation::toInterpolableValue(), it passes
CSSPropertyWebkitFilter to LengthStyleInterpolation::toInterpolableValue, but
I'm using this for both CSSPropertyWebkitFilter and CSSPropertyBackdropFilter,
so this is likely wrong, am I going to need to create a new interpolation class
for CSSPropertyBackdropFilter?

2. CanvasRenderingContext2DState references filters all over, I'm not really
sure when this class is used yet, but I guess that it needs to do the same with
backdrop filters as it does with filters?  Anyway, I'll look a little closer at
this.

3. DeprecatedPaintLayer::backgroundIsKnownToBeOpaqueInRect, I didn't add
anything for backdrop filter, I think that you'd need some transparency on the
object for it to work (so the other if's in the function should be enough)

4. I have a bunch of questions for DeprecatedPaintLayer, I don't think the
backdrop filter needs to show up everywhere that filter does, and it's working
without it, and I don't think the filters that are built here are even used to
render. I'll need to look closer at this too.

[alancutter (OOO until 2018), 2015-08-20 01:48:02]

On 2015/08/19 at 23:37:46, hendrikw wrote:
> 1. FilterStyleInterpolation::toInterpolableValue(), it passes
CSSPropertyWebkitFilter to LengthStyleInterpolation::toInterpolableValue, but
I'm using this for both CSSPropertyWebkitFilter and CSSPropertyBackdropFilter,
so this is likely wrong, am I going to need to create a new interpolation class
for CSSPropertyBackdropFilter?

CSS Animations are in the middle of a refactor and some of the code will go away
in the near future.
You should be fine to just update CSSAnimatableValueFactory and
AnimatedStyleBuilder (as you've done) without having to touch
FilterStyleInterpolation.cpp.

Please add a LayoutTest for animation of this new property to ensure it works.
I'm guessing you'll be able to just s/-webkit-filter/backdrop-filter/ the
contents of LayoutTests/animations/interpolation/filter-interpolation.html.

[jbroman, 2015-08-20 02:49:46]

Sorry to intrude, but setTimeout scared me. :)

https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
File LayoutTests/css3/filters/backdrop-filter-rendering.html (right):

https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
LayoutTests/css3/filters/backdrop-filter-rendering.html:10:
testRunner.notifyDone();
Drive-by: please no setTimeout in layout tests. You probably want
testRunner.layoutAndPaintAsyncThen, or runAfterLayoutAndPaint, which wraps it.

(That is, if you can't just make the test ready in time. It's not clear to me
why you're changing the className of the first element to also be
backdrop_filter, but I'll leave that to your reviewer.)

And, the obligatory: could this be a ref test?

[hendrikw, 2015-08-20 17:46:12]

On 2015/08/20 01:48:02, alancutter wrote:
> On 2015/08/19 at 23:37:46, hendrikw wrote:
> > 1. FilterStyleInterpolation::toInterpolableValue(), it passes
> CSSPropertyWebkitFilter to LengthStyleInterpolation::toInterpolableValue, but
> I'm using this for both CSSPropertyWebkitFilter and CSSPropertyBackdropFilter,
> so this is likely wrong, am I going to need to create a new interpolation
class
> for CSSPropertyBackdropFilter?
> 
> CSS Animations are in the middle of a refactor and some of the code will go
away
> in the near future.
> You should be fine to just update CSSAnimatableValueFactory and
> AnimatedStyleBuilder (as you've done) without having to touch
> FilterStyleInterpolation.cpp.
> 
> Please add a LayoutTest for animation of this new property to ensure it works.
> I'm guessing you'll be able to just s/-webkit-filter/backdrop-filter/ the
> contents of LayoutTests/animations/interpolation/filter-interpolation.html.

Cool thanks, will do!

[hendrikw, 2015-08-20 17:52:15]

On 2015/08/20 02:49:46, jbroman wrote:
> Sorry to intrude, but setTimeout scared me. :)
> 
>
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> File LayoutTests/css3/filters/backdrop-filter-rendering.html (right):
> 
>
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> LayoutTests/css3/filters/backdrop-filter-rendering.html:10:
> testRunner.notifyDone();
> Drive-by: please no setTimeout in layout tests. You probably want
> testRunner.layoutAndPaintAsyncThen, or runAfterLayoutAndPaint, which wraps it.
> 

Thanks, I based this on an existing layout test, can you point me to a test I
should base mine on?

> (That is, if you can't just make the test ready in time. It's not clear to me
> why you're changing the className of the first element to also be
> backdrop_filter, but I'll leave that to your reviewer.)

First element is "backdrop_object" (unless I'm misunderstanding what you're
asking?)

> 
> And, the obligatory: could this be a ref test?

No idea what a ref test is :D

[jbroman, 2015-08-20 18:08:17]

On 2015/08/20 at 17:52:15, hendrikw wrote:
> On 2015/08/20 02:49:46, jbroman wrote:
> > Sorry to intrude, but setTimeout scared me. :)
> > 
> >
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> > File LayoutTests/css3/filters/backdrop-filter-rendering.html (right):
> > 
> >
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> > LayoutTests/css3/filters/backdrop-filter-rendering.html:10:
> > testRunner.notifyDone();
> > Drive-by: please no setTimeout in layout tests. You probably want
> > testRunner.layoutAndPaintAsyncThen, or runAfterLayoutAndPaint, which wraps
it.
> > 
> 
> Thanks, I based this on an existing layout test, can you point me to a test I
should base mine on?

svg/filters/filter-repaint.html is an example (randomly selected from code
search) that uses runAfterLayoutAndPaint

> > (That is, if you can't just make the test ready in time. It's not clear to
me
> > why you're changing the className of the first element to also be
> > backdrop_filter, but I'll leave that to your reviewer.)
> 
> First element is "backdrop_object" (unless I'm misunderstanding what you're
asking?)

Yeah, so you're selecting the element with class backdrop_object, and then
removing that class and replacing it with the class backdrop_filter.

Unless you're testing how we do repainting, you should be able to just style
things the way you want without modifying it in script.

> > 
> > And, the obligatory: could this be a ref test?
> 
> No idea what a ref test is :D

Tests that compare against another .html file instead of a per-platform .png.
Doesn't fit every case, but they don't need lots of baselines like pixel tests
do.

[hendrikw, 2015-08-20 18:32:52]

On 2015/08/20 18:08:17, jbroman wrote:
> On 2015/08/20 at 17:52:15, hendrikw wrote:
> > On 2015/08/20 02:49:46, jbroman wrote:
> > > Sorry to intrude, but setTimeout scared me. :)
> > > 
> > >
>
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> > > File LayoutTests/css3/filters/backdrop-filter-rendering.html (right):
> > > 
> > >
>
https://codereview.chromium.org/1299323005/diff/20001/LayoutTests/css3/filter...
> > > LayoutTests/css3/filters/backdrop-filter-rendering.html:10:
> > > testRunner.notifyDone();
> > > Drive-by: please no setTimeout in layout tests. You probably want
> > > testRunner.layoutAndPaintAsyncThen, or runAfterLayoutAndPaint, which wraps
> it.
> > > 
> > 
> > Thanks, I based this on an existing layout test, can you point me to a test
I
> should base mine on?
> 
> svg/filters/filter-repaint.html is an example (randomly selected from code
> search) that uses runAfterLayoutAndPaint

Cool!

> 
> > > (That is, if you can't just make the test ready in time. It's not clear to
> me
> > > why you're changing the className of the first element to also be
> > > backdrop_filter, but I'll leave that to your reviewer.)
> > 
> > First element is "backdrop_object" (unless I'm misunderstanding what you're
> asking?)
> 
> Yeah, so you're selecting the element with class backdrop_object, and then
> removing that class and replacing it with the class backdrop_filter.
> 
> Unless you're testing how we do repainting, you should be able to just style
> things the way you want without modifying it in script.

Oh, yeah, eww.  I thought this was some weird testing requirement, but now it
seems I chose a poor example.  I'll rework it, thanks!!!!!!!!!!!!!!!!!!

> 
> > > 
> > > And, the obligatory: could this be a ref test?
> > 
> > No idea what a ref test is :D
> 
> Tests that compare against another .html file instead of a per-platform .png.
> Doesn't fit every case, but they don't need lots of baselines like pixel tests
> do.

k, thanks, I'll see what I can do.

[Justin Novosad, 2015-08-20 19:04:10]

The changes to modules/canvas2d are dead code as far as I can tell. Style has no
effect CanvasRenderingContext2D so you should not have anything to do there.

[hendrikw, 2015-08-20 19:44:18]

On 2015/08/20 19:04:10, Justin Novosad wrote:
> The changes to modules/canvas2d are dead code as far as I can tell. Style has
no
> effect CanvasRenderingContext2D so you should not have anything to do there.

Good, I was hoping for that!  Thanks!

[hendrikw, 2015-08-21 02:25:56]

Alright, I think I may be done, maybe.

PTAL, thanks!

[pdr., 2015-08-21 21:44:26]

pdr@chromium.org changed reviewers:
+ alancutter@chromium.org, jbroman@chromium.org, junov@chromium.org

[pdr., 2015-08-21 22:18:33]

LGTM, nice patch.

Since this isn't time critical, please give junov and jbroman a chance to
comment before landing.

[jbroman, 2015-08-22 18:41:38]

LayoutTests/css3/filters/backdrop-filter-rendering.html LGTM

[hendrikw, 2015-08-25 21:35:09]

On 2015/08/22 18:41:38, jbroman wrote:
> LayoutTests/css3/filters/backdrop-filter-rendering.html LGTM

junov is ooo, I'll address any concerns he may have in a followup.

Thanks!!!!!!

[hendrikw, 2015-08-25 21:35:15]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-08-25 21:35:26]

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

[commit-bot: I haz the power, 2015-08-25 21:37:19]

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

[commit-bot: I haz the power, 2015-08-25 21:37:20]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2015-08-25 22:08:15]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-25 22:08:16]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299323005/#ps160001
(title: "merge again")

[commit-bot: I haz the power, 2015-08-25 22:08:46]

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

[commit-bot: I haz the power, 2015-08-25 23:04:57]

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

[commit-bot: I haz the power, 2015-08-25 23:04:58]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2015-08-26 00:00:55]

On 2015/08/25 23:04:58, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

I don't even know why this is failing, trying again? :(

[hendrikw, 2015-08-26 00:01:01]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-08-26 00:01:49]

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

[commit-bot: I haz the power, 2015-08-26 00:04:44]

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

[commit-bot: I haz the power, 2015-08-26 00:04:45]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2015-08-26 00:18:01]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-26 00:18:01]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299323005/#ps180001
(title: "another merge - :/")

[commit-bot: I haz the power, 2015-08-26 00:18:34]

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

[commit-bot: I haz the power, 2015-08-26 00:20:40]

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

[commit-bot: I haz the power, 2015-08-26 00:20:41]

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

[hendrikw, 2015-08-26 00:31:15]

On 2015/08/26 00:20:41, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

So, I've learned something about TestExpectations, changing it sucks.

[hendrikw, 2015-08-26 00:38:00]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-26 00:38:00]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299323005/#ps220001
(title: "remove the newline that would no doubt result in a merge conflict")

[commit-bot: I haz the power, 2015-08-26 00:38:13]

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

[commit-bot: I haz the power, 2015-08-26 01:45:42]

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

[commit-bot: I haz the power, 2015-08-26 01:45:43]

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

[hendrikw, 2015-08-26 02:09:32]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-08-26 02:09:40]

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

[chrishtr, 2015-08-26 02:37:09]

On 2015/08/26 at 00:31:15, hendrikw wrote:
> On 2015/08/26 00:20:41, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> So, I've learned something about TestExpectations, changing it sucks.

Pro tip: put your edits in a random place halfway through the file, to minimize
chance of a patch conflict.

[commit-bot: I haz the power, 2015-08-26 03:27:00]

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

[commit-bot: I haz the power, 2015-08-26 03:27:01]

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

[pdr., 2015-08-26 03:32:05]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2015-08-26 03:32:15]

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

[commit-bot: I haz the power, 2015-08-26 04:16:56]

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

[commit-bot: I haz the power, 2015-08-26 04:16:57]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hendrikw, 2015-08-26 16:49:26]

On 2015/08/26 04:16:57, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

I've removed the expecting failing test, it was actually working (I thought it
required one more change to work), and I've updated the baseline for the
backdrop-filters interpolation test, the javascript has special case code for
-webkit-, so the baselines for the filters and backdrop filters differ.  I'll
try the commit again, but I'm not hopeful, try jobs failing with this "Task
failed, retrying after a sleep of 100ms: http request failed: Internal Server
Error (HTTP 500)"

[hendrikw, 2015-08-26 16:49:34]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-26 16:49:34]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299323005/#ps240001
(title: "fix expected.txt")

[commit-bot: I haz the power, 2015-08-26 16:49:48]

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

[hendrikw, 2015-08-26 16:50:31]

The CQ bit was unchecked by hendrikw@chromium.org

[hendrikw, 2015-08-26 16:52:28]

On 2015/08/26 16:49:26, hendrikw wrote:
> On 2015/08/26 04:16:57, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> I've removed the expecting failing test, it was actually working (I thought it
> required one more change to work), and I've updated the baseline for the
> backdrop-filters interpolation test, the javascript has special case code for
> -webkit-, so the baselines for the filters and backdrop filters differ.  I'll
> try the commit again, but I'm not hopeful, try jobs failing with this "Task
> failed, retrying after a sleep of 100ms: http request failed: Internal Server
> Error (HTTP 500)"

Alright, PSA email described the problem, I'll wait until they've fixed it.

[hendrikw, 2015-08-26 20:30:25]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-08-26 20:31:00]

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

[commit-bot: I haz the power, 2015-08-26 21:43:01]

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

[commit-bot: I haz the power, 2015-08-26 21:43:02]

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

[hendrikw, 2015-08-27 14:07:40]

The CQ bit was checked by hendrikw@chromium.org

[hendrikw, 2015-08-27 14:07:41]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299323005/#ps280001
(title: "one more")

[commit-bot: I haz the power, 2015-08-27 14:07:50]

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

[commit-bot: I haz the power, 2015-08-27 14:13:32]

Committed patchset #15 (id:280001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201314

[chrishtr, 2015-09-26 00:50:32]

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

[chrishtr, 2015-09-26 00:50:34]

https://codereview.chromium.org/1299323005/diff/280001/Source/core/animation/...
File Source/core/animation/CompositorAnimations.cpp (right):

https://codereview.chromium.org/1299323005/diff/280001/Source/core/animation/...
Source/core/animation/CompositorAnimations.cpp:119: const bool
affectsBackdropFilter =
effectToAdd.affects(PropertyHandle(CSSPropertyBackdropFilter));
Should we support compositor animations of background filters? Ask dstockwell to
see if it's easy to implement.