Check for empty WebFilterOperations in FilterPainter

Check for empty WebFilterOperations in FilterPainter

This works around a situation where FilterPainter thinks we should
have non-empty WebFilterOperations even though we really don't,
since the DeprecatedPaintLayer's FilterEffectBuilder doesn't realize
that the filter it refers to has changed.

This depends on Chromium CL http://crrev.com/1218543002.

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

[ajuma, 2015-06-26 17:40:17]

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

[Stephen White, 2015-06-26 17:57:38]

LGTM w/nits

https://codereview.chromium.org/1214673005/diff/20001/LayoutTests/css3/filter...
File LayoutTests/css3/filters/filter-effect-removed.html (right):

https://codereview.chromium.org/1214673005/diff/20001/LayoutTests/css3/filter...
LayoutTests/css3/filters/filter-effect-removed.html:2: <svg width="0"
xmlns="http://www.w3.org/2000/svg" width="0" height="0" version="1.1">
Nit: width="0" twice here

https://codereview.chromium.org/1214673005/diff/20001/public/platform/WebFilt...
File public/platform/WebFilterOperations.h (right):

https://codereview.chromium.org/1214673005/diff/20001/public/platform/WebFilt...
public/platform/WebFilterOperations.h:59: virtual size_t size() const  = 0;
uNit: two spaces before =

[ajuma, 2015-06-26 19:09:01]

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

[ajuma, 2015-06-26 19:09:26]

+pdr for public/

https://codereview.chromium.org/1214673005/diff/20001/LayoutTests/css3/filter...
File LayoutTests/css3/filters/filter-effect-removed.html (right):

https://codereview.chromium.org/1214673005/diff/20001/LayoutTests/css3/filter...
LayoutTests/css3/filters/filter-effect-removed.html:2: <svg width="0"
xmlns="http://www.w3.org/2000/svg" width="0" height="0" version="1.1">
On 2015/06/26 17:57:38, Stephen White wrote:
> Nit: width="0" twice here

Done.

https://codereview.chromium.org/1214673005/diff/20001/public/platform/WebFilt...
File public/platform/WebFilterOperations.h (right):

https://codereview.chromium.org/1214673005/diff/20001/public/platform/WebFilt...
public/platform/WebFilterOperations.h:59: virtual size_t size() const  = 0;
On 2015/06/26 17:57:38, Stephen White wrote:
> uNit: two spaces before =

Done.

[pdr., 2015-06-26 19:19:22]

If the filter doesn't display anything, shouldn't
BeginFilterDisplayItem::drawsContent return false so the filter begin/end pair
is removed before ever hitting cc?

[pdr., 2015-06-26 19:21:02]

On 2015/06/26 at 19:19:22, pdr wrote:
> If the filter doesn't display anything, shouldn't
BeginFilterDisplayItem::drawsContent return false so the filter begin/end pair
is removed before ever hitting cc?

A little more context: We have an optimization that removes no-op begin/end
pairs in DisplayItemList::add. When adding an end item, we check if the previous
display item was a begin and if that begin doesn't draw content, we remove the
pair entirely. I think we may be able to use this optimization to work around
this crash too.

[ajuma, 2015-06-26 19:22:43]

On 2015/06/26 19:19:22, pdr wrote:
> If the filter doesn't display anything, shouldn't
> BeginFilterDisplayItem::drawsContent return false so the filter begin/end pair
> is removed before ever hitting cc?

It should, but doesn't because m_imageFilter is stale (since it corresponds to
the stale filter in the layer's FilterEffectBuilder; see the discussion in the
bug for more on how the latter becomes stale).

[ajuma, 2015-06-26 19:27:15]

On 2015/06/26 19:21:02, pdr wrote:
> On 2015/06/26 at 19:19:22, pdr wrote:
> > If the filter doesn't display anything, shouldn't
> BeginFilterDisplayItem::drawsContent return false so the filter begin/end pair
> is removed before ever hitting cc?
> 
> A little more context: We have an optimization that removes no-op begin/end
> pairs in DisplayItemList::add. When adding an end item, we check if the
previous
> display item was a begin and if that begin doesn't draw content, we remove the
> pair entirely. I think we may be able to use this optimization to work around
> this crash too.

That optimization only kicks in when there aren't any other items in between the
begin and end. In this case, we do actually have content in between (that is, we
have the image), so we'll produce display items even for filters that don't draw
their own content (e.g. for a grayscale filter, say).

[pdr., 2015-06-26 19:50:12]

On 2015/06/26 at 19:22:43, ajuma wrote:
> On 2015/06/26 19:19:22, pdr wrote:
> > If the filter doesn't display anything, shouldn't
> > BeginFilterDisplayItem::drawsContent return false so the filter begin/end
pair
> > is removed before ever hitting cc?
> 
> It should, but doesn't because m_imageFilter is stale (since it corresponds to
the stale filter in the layer's FilterEffectBuilder; see the discussion in the
bug for more on how the latter becomes stale).

LGTM

[ajuma, 2015-06-26 20:34:13]

I've replaced WebFilterOperations::size with ::isEmpty, to address a review
comment on the Chromium-side patch.

[ajuma, 2015-06-26 21:47:47]

The CQ bit was checked by ajuma@chromium.org

[ajuma, 2015-06-26 21:47:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
senorblanco@chromium.org, pdr@chromium.org
Link to the patchset: https://codereview.chromium.org/1214673005/#ps60001
(title: "isEmpty instead of size")

[commit-bot: I haz the power, 2015-06-26 21:47:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214673005/60001

[commit-bot: I haz the power, 2015-06-27 00:48:14]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197942