Layers that have a backdrop-filter should paints contents

Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452
Committed: https://crrev.com/b0d78204557a875d10beb43e9f6c0e634bf364f8
Cr-Commit-Position: refs/heads/master@{#373860}

[jaydasika, 2016-01-27 01:01:02]

Description was changed from

==========
Layers that have a backdrop-filter paints contents

BUG=527452
==========

to

==========
Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452
==========

[jaydasika, 2016-01-27 01:03:14]

jaydasika@chromium.org changed reviewers:
+ chrishtr@chromium.org, pdr@chromium.org

[jaydasika, 2016-01-27 01:03:15]

PTAL!

[pdr., 2016-01-27 05:52:10]

https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:441: return
hasBackgroundImageOrBackdropFilter();
Is this tested? This was my concern in the previous review.

I looked at some callsites of style->hasBackground() and I'm not sure this is
correct. Just as an example, LayoutBoxModelObject::backgroundStolenForBeingBody
has a hasBackground call... is it correct to return false if there's a backdrop
filter?

Renaming this to be hasBackgroundOrBackdropFilter might help as well, but I'm
primarily worried about the edge-case bugs this introduces.

[jaydasika, 2016-01-27 23:59:42]

https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/style/ComputedStyle.h (right):

https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/style/ComputedStyle.h:441: return
hasBackgroundImageOrBackdropFilter();
On 2016/01/27 05:52:09, pdr wrote:
> Is this tested? This was my concern in the previous review.
> 
> I looked at some callsites of style->hasBackground() and I'm not sure this is
> correct. Just as an example,
LayoutBoxModelObject::backgroundStolenForBeingBody
> has a hasBackground call... is it correct to return false if there's a
backdrop
> filter?
> 
> Renaming this to be hasBackgroundOrBackdropFilter might help as well, but I'm
> primarily worried about the edge-case bugs this introduces.

Ah, I see your point. I think we should have a separate hasBackdropFilter,
because we can't have it in hasBackground(for the reasons you mentioned) and not
in hasBoxDecorations (for reasons mentioned in the previous review). WDYT ?

[pdr., 2016-01-28 01:03:12]

On 2016/01/27 at 23:59:42, jaydasika wrote:
>
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> 
>
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/style/ComputedStyle.h:441: return
hasBackgroundImageOrBackdropFilter();
> On 2016/01/27 05:52:09, pdr wrote:
> > Is this tested? This was my concern in the previous review.
> > 
> > I looked at some callsites of style->hasBackground() and I'm not sure this
is
> > correct. Just as an example,
LayoutBoxModelObject::backgroundStolenForBeingBody
> > has a hasBackground call... is it correct to return false if there's a
backdrop
> > filter?
> > 
> > Renaming this to be hasBackgroundOrBackdropFilter might help as well, but
I'm
> > primarily worried about the edge-case bugs this introduces.
> 
> Ah, I see your point. I think we should have a separate hasBackdropFilter,
because we can't have it in hasBackground(for the reasons you mentioned) and not
in hasBoxDecorations (for reasons mentioned in the previous review). WDYT ?

There seems to be the opposite problem too, such as
TableSectionPainter::paintCell and BlockPainter::paintObject which check for
backgrounds but probably need to check for backdrop filters too.

In the previous review I mentioned auditing all hasBackground() and deciding
what to do on a case-by-case basis. My worry here is that we're getting close to
launching backdrop filters but these issues haven't really been looked at
(partially my fault). With a little work on this patch I think we can solve the
root cause of these issues.

@chrishtr, do you have thoughts here?

[chrishtr, 2016-01-28 01:23:23]

On 2016/01/28 at 01:03:12, pdr wrote:
> On 2016/01/27 at 23:59:42, jaydasika wrote:
> >
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> > 
> >
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/style/ComputedStyle.h:441: return
hasBackgroundImageOrBackdropFilter();
> > On 2016/01/27 05:52:09, pdr wrote:
> > > Is this tested? This was my concern in the previous review.
> > > 
> > > I looked at some callsites of style->hasBackground() and I'm not sure this
is
> > > correct. Just as an example,
LayoutBoxModelObject::backgroundStolenForBeingBody
> > > has a hasBackground call... is it correct to return false if there's a
backdrop
> > > filter?
> > > 
> > > Renaming this to be hasBackgroundOrBackdropFilter might help as well, but
I'm
> > > primarily worried about the edge-case bugs this introduces.
> > 
> > Ah, I see your point. I think we should have a separate hasBackdropFilter,
because we can't have it in hasBackground(for the reasons you mentioned) and not
in hasBoxDecorations (for reasons mentioned in the previous review). WDYT ?
> 
> There seems to be the opposite problem too, such as
TableSectionPainter::paintCell and BlockPainter::paintObject which check for
backgrounds but probably need to check for backdrop filters too.
> 
> In the previous review I mentioned auditing all hasBackground() and deciding
what to do on a case-by-case basis. My worry here is that we're getting close to
launching backdrop filters but these issues haven't really been looked at
(partially my fault). With a little work on this patch I think we can solve the
root cause of these issues.
> 
> @chrishtr, do you have thoughts here?

None, other than I agree we should look at each call site and decide what its
semantics should be w.r.t. this feature.

[jaydasika, 2016-01-28 01:52:18]

On 2016/01/28 01:23:23, chrishtr wrote:
> On 2016/01/28 at 01:03:12, pdr wrote:
> > On 2016/01/27 at 23:59:42, jaydasika wrote:
> > >
>
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/style/ComputedStyle.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1637143002/diff/20001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/style/ComputedStyle.h:441: return
> hasBackgroundImageOrBackdropFilter();
> > > On 2016/01/27 05:52:09, pdr wrote:
> > > > Is this tested? This was my concern in the previous review.
> > > > 
> > > > I looked at some callsites of style->hasBackground() and I'm not sure
this
> is
> > > > correct. Just as an example,
> LayoutBoxModelObject::backgroundStolenForBeingBody
> > > > has a hasBackground call... is it correct to return false if there's a
> backdrop
> > > > filter?
> > > > 
> > > > Renaming this to be hasBackgroundOrBackdropFilter might help as well,
but
> I'm
> > > > primarily worried about the edge-case bugs this introduces.
> > > 
> > > Ah, I see your point. I think we should have a separate hasBackdropFilter,
> because we can't have it in hasBackground(for the reasons you mentioned) and
not
> in hasBoxDecorations (for reasons mentioned in the previous review). WDYT ?
> > 
> > There seems to be the opposite problem too, such as
> TableSectionPainter::paintCell and BlockPainter::paintObject which check for
> backgrounds but probably need to check for backdrop filters too.
> > 
> > In the previous review I mentioned auditing all hasBackground() and deciding
> what to do on a case-by-case basis. My worry here is that we're getting close
to
> launching backdrop filters but these issues haven't really been looked at
> (partially my fault). With a little work on this patch I think we can solve
the
> root cause of these issues.
> > 
> > @chrishtr, do you have thoughts here?
> 
> None, other than I agree we should look at each call site and decide what its
> semantics should be w.r.t. this feature.

Ok, will list out all cases in a doc and we can discuss there about the
semantics of each case.

[jaydasika, 2016-02-03 03:43:31]

Changed the CL after investigating all hasBackground and hasBoxDecorations
calls.
1) moved hasBackdropFilter check to hasBoxDecorations.
2) added explicit calls to hasBackdropFilter in other places it is required.
3) Added two more tests.

[pdr., 2016-02-03 04:20:28]

This is looking great, just two requests for tests.

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:18: hasBackground =
backgroundColor.alpha() || layoutBox.style()->hasBackgroundImage() ||
layoutBox.style()->hasBackdropFilter();
Is it possible to write a test for this? It looks like it's only used for bleed
avoidance so I wonder if the backdrop filter check is needed.

One technique for writing these tests is to set hasBackground to false and see
what tests fail, then copy and modify those to use backdrop filter.

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:211: if
((row->hasBackground() || row->styleRef().hasBackdropFilter()) &&
!row->hasSelfPaintingLayer())
Can you add a test for this that fails without the patch and passes with?

[jaydasika, 2016-02-05 02:10:02]

I think there is no use case where we will need to paint the background of a
table cell when it has no background image and default background color but
still has a backdrop filter, so I have removed all checks for backdrop filters
in Table*Painters.

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/BoxDecorationData.cpp (right):

https://codereview.chromium.org/1637143002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/BoxDecorationData.cpp:18: hasBackground =
backgroundColor.alpha() || layoutBox.style()->hasBackgroundImage() ||
layoutBox.style()->hasBackdropFilter();
On 2016/02/03 04:20:28, pdr wrote:
> Is it possible to write a test for this? It looks like it's only used for
bleed
> avoidance so I wonder if the backdrop filter check is needed.
> 
> One technique for writing these tests is to set hasBackground to false and see
> what tests fail, then copy and modify those to use backdrop filter.

You are right, I have verified that its used only for bleed avoidance and so we
can remove this backdrop filter check.

[jaydasika, 2016-02-05 02:10:14]

I think there is no use case where we will need to paint the background of a
table cell when it has no background image and default background color but
still has a backdrop filter, so I have removed all checks for backdrop filters
in Table*Painters.

[pdr., 2016-02-05 02:14:42]

LGTM

[jaydasika, 2016-02-05 17:19:33]

The CQ bit was checked by jaydasika@chromium.org

[commit-bot: I haz the power, 2016-02-05 17:20:37]

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

[commit-bot: I haz the power, 2016-02-05 18:49:27]

Description was changed from

==========
Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452
==========

to

==========
Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452
==========

[commit-bot: I haz the power, 2016-02-05 18:49:28]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-05 18:50:21]

Description was changed from

==========
Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452
==========

to

==========
Layers that have a backdrop-filter should paints contents

patch from issue 1327973002 (http://crrev.com/1327973002)

BUG=527452

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

[commit-bot: I haz the power, 2016-02-05 18:50:22]

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

[please use gerrit instead, 2016-02-05 20:45:30]

Your patch may have caused a new test failure:

Regressions: Unexpected text-only failures (2)
  compositing/overflow/scroll-parent-absolute-with-backdrop-filter.html [
Failure ]
 
virtual/prefer_compositing_to_lcd_text/compositing/overflow/scroll-parent-absolute-with-backdrop-filter.html
[ Failure ]

Failing builds:
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...

If the build fails for the 3rd time in a row, I will be speculatively reverting
this patch.

[please use gerrit instead, 2016-02-05 20:50:04]

The build failed 3rd time in a row:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...

[please use gerrit instead, 2016-02-05 20:52:04]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1678443002/ by rouslan@chromium.org.

The reason for reverting is: Speculative revert to fix webkit_tests on "WebKit
Mac10.6" bot.

Failing builds:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6/builds...

Failing tests:

Regressions: Unexpected text-only failures (2)
compositing/overflow/scroll-parent-absolute-with-backdrop-filter.html[Failure]
virtual/prefer_compositing_to_lcd_text/compositing/overflow/scroll-parent-absolute-with-backdrop-filter.html[Failure].

[please use gerrit instead, 2016-02-05 23:08:09]

webkit_tests are now passing, so the revert fixed the problem.