Bug 109098 - [CSS Filters] Filter outsets clipped on composited layers when filter is applied after first layout
Summary: [CSS Filters] Filter outsets clipped on composited layers when filter is appl...
Status: REOPENED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Max Vujovic
URL:
Keywords:
Depends on: 114179 109330 112606
Blocks: 68469 112830 96580 107708 110362
  Show dependency treegraph
 
Reported: 2013-02-06 15:00 PST by Max Vujovic
Modified: 2014-03-02 09:15 PST (History)
17 users (show)

See Also:


Attachments
Reproduction (408 bytes, text/html)
2013-02-06 15:00 PST, Max Vujovic
no flags Details
Patch (7.86 KB, patch)
2013-02-19 17:10 PST, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Bots (72.50 KB, patch)
2013-02-26 14:29 PST, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Bots (72.49 KB, patch)
2013-03-05 11:20 PST, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Bots (95.05 KB, patch)
2013-03-13 14:31 PDT, Max Vujovic
no flags Details | Formatted Diff | Diff
Patch for Bots (84.52 KB, patch)
2013-03-13 14:53 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Bots (Work in Progress) (128.57 KB, patch)
2013-03-15 15:40 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Review (131.39 KB, patch)
2013-03-19 13:19 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Review (135.63 KB, patch)
2013-03-20 13:13 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Patch for Review (135.42 KB, patch)
2013-03-20 15:19 PDT, Max Vujovic
no flags Details | Formatted Diff | Diff
Patch for Landing (135.73 KB, patch)
2013-04-02 11:34 PDT, Max Vujovic
mvujovic: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64 (597.14 KB, application/zip)
2013-04-02 13:55 PDT, WebKit Review Bot
no flags Details
Patch for Landing (135.73 KB, patch)
2013-04-02 14:08 PDT, Max Vujovic
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Max Vujovic 2013-02-06 15:00:13 PST
Created attachment 186928 [details]
Reproduction

See the attached reproduction. This only reproduces in Chrome, not in Safari.

Possibly related to bug 107708.
Comment 1 Max Vujovic 2013-02-07 15:33:59 PST
Corresponding Chromium bug: http://code.google.com/p/chromium/issues/detail?id=174788

I think I figured out the problem. RenderLayer::styleChanged does not notice a change in filter outsets and does not update the compositing bounds for Chromium. In there, we should call compositor()->setCompositingLayersNeedRebuild() if the outsets changed. I will assign this bug to myself and work on the fix there.
Comment 2 Max Vujovic 2013-02-19 17:10:03 PST
Created attachment 189209 [details]
Patch
Comment 3 WebKit Review Bot 2013-02-19 18:36:31 PST
Comment on attachment 189209 [details]
Patch

Attachment 189209 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16639109

New failing tests:
platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-apply-hw-shadow.html
platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-move-hw-shadow.html
css3/filters/filter-change-repaint-composited.html
Comment 4 Max Vujovic 2013-02-20 11:52:00 PST
(In reply to comment #3)
> (From update of attachment 189209 [details])
> Attachment 189209 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16639109
> 
> New failing tests:
> platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-apply-hw-shadow.html
> platform/chromium/virtual/softwarecompositing/filters/after-first-style-calc-move-hw-shadow.html
> css3/filters/filter-change-repaint-composited.html

filter-change-repaint-composited.html is very similar to the tests I just added. It has an incorrect result checked in, so I will rebaseline its result and remove my tests.
Comment 5 Max Vujovic 2013-02-26 14:29:41 PST
Created attachment 190362 [details]
Patch for Bots
Comment 6 WebKit Review Bot 2013-02-26 15:39:27 PST
Attachment 190362 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/css3/filters/filter-change-repaint-composited.html', u'LayoutTests/platform/chromium-linux/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium-win/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp']" exit_code: 1
LayoutTests/css3/filters/filter-change-repaint-composited-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Max Vujovic 2013-03-05 11:20:16 PST
Created attachment 191520 [details]
Patch for Bots

Let's see if the style-bot likes this patch.
Comment 8 WebKit Review Bot 2013-03-05 11:23:23 PST
Attachment 191520 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/css3/filters/filter-change-repaint-composited.html', u'LayoutTests/platform/chromium-linux/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium-win/css3/filters/filter-change-repaint-composited-expected.png', u'LayoutTests/platform/chromium/TestExpectations', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/RenderLayer.cpp', u'Source/WebCore/rendering/RenderLayerBacking.cpp']" exit_code: 1
LayoutTests/css3/filters/filter-change-repaint-composited-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Alexandru Chiculita 2013-03-05 11:39:29 PST
View in context: https://bugs.webkit.org/attachment.cgi?id=190362&action=review

> Source/WebCore/ChangeLog:16
> +            When we update the graphics layer geometry, we should also call

You should explain the two problems that you fix with this patch: 
1. FilterOutsets are used by some of the compositors to calculate the sizes used by their backing surfaces. And they end up using old values.
2. We need to recompute the overlap map in order to promote the right layers after the outsets have been updated.

> Source/WebCore/rendering/RenderLayer.cpp:5619
> +        compositor()->setCompositingLayersNeedRebuild();

All the branches seem to have a single same line. Can we combine in only one branch?

You also need a test for the case when the layer is promoted to run in the GPU because it overlays some other content, You have two cases:
1) the filter used to be computed in software, so you need to check that it is applied on hardware and it is not applied twice (both software and hardware).
2) sometimes the filter can only be computed in software mode (ie. drop-shadow(..) sepia(..) on Mac), so it will need to preserve the software mode.

> Source/WebCore/rendering/RenderLayerBacking.cpp:584
> +    m_owningLayer->updateOrRemoveFilterEffectRenderer();

It might be better to put this in the updateFilters itself.
Looks like updateFilters is also called from the transitions code (RenderLayerBacking::startTransition). Should you patch that path too?  What happens in that case?

Is there a bug added for computing the overlap maps for filter animations/transitions correctly? I think that still reproduces.
Comment 10 Max Vujovic 2013-03-13 14:27:28 PDT
(In reply to comment #9)
> View in context: https://bugs.webkit.org/attachment.cgi?id=190362&action=review
> 
> > Source/WebCore/ChangeLog:16
> > +            When we update the graphics layer geometry, we should also call
> 
> You should explain the two problems that you fix with this patch: 
> 1. FilterOutsets are used by some of the compositors to calculate the sizes used by their backing surfaces. And they end up using old values.
> 2. We need to recompute the overlap map in order to promote the right layers after the outsets have been updated.

Yup, that's a great suggestion.

> 
> > Source/WebCore/rendering/RenderLayer.cpp:5619
> > +        compositor()->setCompositingLayersNeedRebuild();
> 
> All the branches seem to have a single same line. Can we combine in only one branch?

Sure, I'll do that in my final patch.

> 
> You also need a test for the case when the layer is promoted to run in the GPU because it overlays some other content, You have two cases:
> 1) the filter used to be computed in software, so you need to check that it is applied on hardware and it is not applied twice (both software and hardware).
> 2) sometimes the filter can only be computed in software mode (ie. drop-shadow(..) sepia(..) on Mac), so it will need to preserve the software mode.

Yes, these are good tests. I'll make sure I have them in my final patch.

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:584
> > +    m_owningLayer->updateOrRemoveFilterEffectRenderer();
> 
> It might be better to put this in the updateFilters itself.

Yeah, that would be better.

> Looks like updateFilters is also called from the transitions code (RenderLayerBacking::startTransition). Should you patch that path too?  What happens in that case?

The beginning or end of a transition can cause a filter render mode change (sw->hw painted filters or vice-versa). I realized we also need to call setContentsNeedDisplay() whenever the filter render mode changes. In a fix I'm working on, updateFilters calls setContentsNeedDisplay() in this situation. 

> 
> Is there a bug added for computing the overlap maps for filter animations/transitions correctly? I think that still reproduces.

I'll have to try that case out. I haven't been able to locate a bug for overlap map + filter animation, but I thought you filed one some time ago?

Locally, I've been seeing issues with filter outsets being clipped during animation. These issues are unrelated to overlap testing. I'm working on automating and expanding a test suite I've built locally that reproduces these issues. I've fixed the issue locally by adding a little more to this fix. I'll upload the tests in a patch when I have them automated.
Comment 11 Max Vujovic 2013-03-13 14:31:35 PDT
Created attachment 192993 [details]
Patch for Bots

Trying out some additions to my fix on EWS to see if they break existing tests.
Comment 12 Max Vujovic 2013-03-13 14:53:50 PDT
Created attachment 193000 [details]
Patch for Bots
Comment 13 Simon Fraser (smfr) 2013-03-13 15:30:34 PDT
Comment on attachment 193000 [details]
Patch for Bots

The [Chromium] on the title seems wrong given that the patch touches cross-platform code.
Comment 14 Max Vujovic 2013-03-13 15:35:57 PDT
(In reply to comment #13)
> (From update of attachment 193000 [details])
> The [Chromium] on the title seems wrong given that the patch touches cross-platform code.
Comment 15 Max Vujovic 2013-03-13 15:37:02 PDT
(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 193000 [details] [details])
> > The [Chromium] on the title seems wrong given that the patch touches cross-platform code.

Yup- updated.
Comment 16 Build Bot 2013-03-14 01:18:41 PDT
Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17140125

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
Comment 17 Build Bot 2013-03-14 02:38:04 PDT
Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16997782

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
Comment 18 Build Bot 2013-03-14 08:10:33 PDT
Comment on attachment 193000 [details]
Patch for Bots

Attachment 193000 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17120126

New failing tests:
compositing/filters/sw-shadow-overlaps-hw-layer.html
compositing/filters/sw-shadow-overlaps-hw-shadow.html
compositing/filters/sw-layer-overlaps-hw-shadow.html
Comment 19 Max Vujovic 2013-03-15 15:40:00 PDT
Created attachment 193385 [details]
Patch for Bots (Work in Progress)

This patch is still a work in progress. I've added new tests to it, and I've added some ChangeLog comments (in case someone wants a preview explanation of the changes).

This patch makes layer bounds smaller on Safari when using composited blurs and drop-shadows. When Safari's CoreAnimation compositor paints filters, it computes filter outsets
internally, so I don't include them in the composited layer bounds anymore.

Currently, other platforms' compositors still need filter outsets included in their composited layer bounds. Thus, I've added different expectations for Chromium. I'm expecting failures from the other platforms that enable filters.

On a separate note, the conditions for calling setCompositingLayersNeedRebuild() in RenderLayer::styleChanged are getting even harder to read with this patch, as Alex noticed. I think the conditions should be factored into functions like:
- needsCompositingLayersRebuiltForClip
- needsCompositingLayersRebuiltForOverflow
- needsCompositingLayersRebuiltForFilters

I'll create a separate bug to do that refactor so this patch doesn't touch too much.
Comment 20 Max Vujovic 2013-03-19 13:19:56 PDT
Created attachment 193909 [details]
Patch for Review

Updated the patch after the refactor in bug 112606. Added a few more comments in the ChangeLog entries.
Comment 21 Alexandru Chiculita 2013-03-19 14:11:15 PDT
Comment on attachment 193909 [details]
Patch for Review

View in context: https://bugs.webkit.org/attachment.cgi?id=193909&action=review

Looks good Max! Thanks for working on this.
I did a quick review on the code. A couple of nits on reorganizing the code.

> Source/WTF/wtf/Platform.h:928
> +/* FIXME: Once all platforms' compositors compute their own filter outsets, we should remove this define. */

Let's add a bug number to track this new flag. Eventually all platforms would need to do this and the flag would need to be removed.

> Source/WebCore/rendering/RenderLayer.cpp:6009
> +    if (oldStyle)
> +        oldOutsets = oldStyle->filterOutsets();

You should return false for oldStyle == 0, so maybe you want to promote that to the first if condition.

> Source/WebCore/rendering/RenderLayer.cpp:6012
> +    return (oldOutsets != newOutsets) || (didChangeFilterPaintMode && !newOutsets.isZero());

You could break this into 2 statements and add more comments about the cases that you handle.

> Source/WebCore/rendering/RenderLayer.cpp:6069
>      updateOrRemoveFilterClients();

These functions are expensive, maybe we could enclose them all in a boolean that checks if we had filters or going to have filters.

> Source/WebCore/rendering/RenderLayer.cpp:6076
> +    bool didChangeFilterPaintMode = false;
> +    bool isRunningAcceleratedFilterAnimation = renderer()->animation()->isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyWebkitFilter);
> +    if (isComposited() && !isRunningAcceleratedFilterAnimation)
> +        didChangeFilterPaintMode = backing()->updateFilters(renderer()->style());
> +#endif
> +    updateOrRemoveFilterEffectRenderer();

These lines could go into a separate function.

> Source/WebCore/rendering/RenderLayer.cpp:6086
> +        || needsCompositingLayersRebuiltForFilters(oldStyle, newStyle, isRunningAcceleratedFilterAnimation, didChangeFilterPaintMode)

You could use the same boolean here to guard against getting in filters code.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:751
> +    // If the compositor computes its own filter outsets, don't include them in the composited bounds.

Let's add a bug number to track this new flag. Eventually all platforms would need to do this.
Comment 22 Max Vujovic 2013-03-19 14:14:24 PDT
Comment on attachment 193909 [details]
Patch for Review

Removing r? to address Alex's suggestions.
Comment 23 EFL EWS Bot 2013-03-19 15:21:36 PDT
Comment on attachment 193909 [details]
Patch for Review

Attachment 193909 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17122732
Comment 24 Max Vujovic 2013-03-20 13:13:33 PDT
Created attachment 194103 [details]
Patch for Review

New patch to address Alex's suggestions. Also, added TestExpectations entries for Qt, since Qt will need rebaselines for the new and updated tests.
Comment 25 Max Vujovic 2013-03-20 13:27:34 PDT
Comment on attachment 193909 [details]
Patch for Review

Thanks for the suggestions, Alex! I've taken another pass at the code and tried to factor it better. I've also added the ideas from the ChangeLog as comments where appropriate.

View in context: https://bugs.webkit.org/attachment.cgi?id=193909&action=review

>> Source/WTF/wtf/Platform.h:928
>> +/* FIXME: Once all platforms' compositors compute their own filter outsets, we should remove this define. */
> 
> Let's add a bug number to track this new flag. Eventually all platforms would need to do this and the flag would need to be removed.

Good idea. Added bug 112830.

>> Source/WebCore/rendering/RenderLayer.cpp:6009
>> +        oldOutsets = oldStyle->filterOutsets();
> 
> You should return false for oldStyle == 0, so maybe you want to promote that to the first if condition.

I've restructured this code quite a bit. I ended up combining the !oldStyle check with the (oldOutsets != newOutsets) check. It seems to make more sense there.

>> Source/WebCore/rendering/RenderLayer.cpp:6012
>> +    return (oldOutsets != newOutsets) || (didChangeFilterPaintMode && !newOutsets.isZero());
> 
> You could break this into 2 statements and add more comments about the cases that you handle.

Done.

>> Source/WebCore/rendering/RenderLayer.cpp:6069
>>      updateOrRemoveFilterClients();
> 
> These functions are expensive, maybe we could enclose them all in a boolean that checks if we had filters or going to have filters.

Right. I've added a hasOrHadFilters(oldStyle, newStyle) helper function to help us exit early. I tried writing it with the boolean, but it didn't look as good.

>> Source/WebCore/rendering/RenderLayer.cpp:6076
>> +    updateOrRemoveFilterEffectRenderer();
> 
> These lines could go into a separate function.

Done: RenderLayer::updateFilters.

>> Source/WebCore/rendering/RenderLayer.cpp:6086
>> +        || needsCompositingLayersRebuiltForFilters(oldStyle, newStyle, isRunningAcceleratedFilterAnimation, didChangeFilterPaintMode)
> 
> You could use the same boolean here to guard against getting in filters code.

Now I call hasOrHadFilters in that function and exit early if it's false.
Comment 26 Max Vujovic 2013-03-20 13:38:27 PDT
Comment on attachment 194103 [details]
Patch for Review

Canceling review. I realized some of combinations of my #if ENABLE(CSS_FILTERS) / #if USE(ACCELERATED_COMPOSITING) blocks won't work.
Comment 27 EFL EWS Bot 2013-03-20 14:28:25 PDT
Comment on attachment 194103 [details]
Patch for Review

Attachment 194103 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17161899
Comment 28 Stephen White 2013-03-20 14:37:16 PDT
Comment on attachment 194103 [details]
Patch for Review

Sorry I'm late to this patch.

If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non-automatic bounds, as I understand it.

(Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)
Comment 29 Max Vujovic 2013-03-20 15:05:47 PDT
(In reply to comment #28)
> (From update of attachment 194103 [details])
> Sorry I'm late to this patch.

No worries. Thanks for looking! :)

> 
> If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  

Computing filter outsets in the Chromium compositor was the next problem Alex and I wanted to tackle after this patch. This patch is intended to set up that work. The idea is, after Chromium can compute its own filter outsets, we can add Chromium to the HAVE(COMPOSITOR_FILTER_OUTSETS) flag. Chromium is in the majority right now- all of the compositors besides CoreAnimation (e.g. Qt) can't compute their own filter outsets yet.

Luckily, the codepaths aren't too different. I use the flag in two places. For platforms with compositor filter outsets:
(1) Avoid including filter outsets in the composited layer bounds.
(2) Invalidate the compositing layer tree if compositor-painted filters fall back to software painting, since WebKit will be drawing the filters and needs a bigger backing. (Also, vice-versa.)

> It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non automatic bounds, as I understand it.
> 
> (Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)

Chrome is the majority :). This patch tries to fix a couple of things that are pretty coupled:
(1) Filter outset changes go unnoticed by WebKit
(2) We always include filter outsets in the composited layer bounds.
(3) When filters with outsets fall back to software painting, we don't update the layer's compositing bounds.
Comment 30 Stephen White 2013-03-20 15:10:02 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 194103 [details] [details])
> > Sorry I'm late to this patch.
> 
> No worries. Thanks for looking! :)
> 
> > 
> > If the problem is that the Chromium compositor doesn't compute filter outsets internally, that's something that could be fixed in the Chrome compositor, if it would make the Safari and Chrome codepaths closer.  
> 
> Computing filter outsets in the Chromium compositor was the next problem Alex and I wanted to tackle after this patch. This patch is intended to set up that work. The idea is, after Chromium can compute its own filter outsets, we can add Chromium to the HAVE(COMPOSITOR_FILTER_OUTSETS) flag. Chromium is in the majority right now- all of the compositors besides CoreAnimation (e.g. Qt) can't compute their own filter outsets yet.
> 
> Luckily, the codepaths aren't too different. I use the flag in two places. For platforms with compositor filter outsets:
> (1) Avoid including filter outsets in the composited layer bounds.
> (2) Invalidate the compositing layer tree if compositor-painted filters fall back to software painting, since WebKit will be drawing the filters and needs a bigger backing. (Also, vice-versa.)
> 
> > It's something we've talked about doing for Skia (independently of Chrome).  Presumably, we still need to have a way to override the filter outsets in Skia, since SVG filters on SVG content for example have non automatic bounds, as I understand it.
> > 
> > (Note that I'm not against this patch as-is, just that it seems unfortunate to diverge if Chrome is the outlier.)
> 
> Chrome is the majority :). This patch tries to fix a couple of things that are pretty coupled:
> (1) Filter outset changes go unnoticed by WebKit
> (2) We always include filter outsets in the composited layer bounds.
> (3) When filters with outsets fall back to software painting, we don't update the layer's compositing bounds.

Sounds great.  Thanks for the explanation.
Comment 31 Max Vujovic 2013-03-20 15:19:15 PDT
Created attachment 194131 [details]
Patch for Review

Fixed up the #if / #endif issues. Reworded the comments in the tests to clarify when we're testing something Safari specific (e.g. Safari falls back to painting filters in software when drop-shadow is not the last filter in the chain).
Comment 32 Max Vujovic 2013-03-20 17:15:02 PDT
CC-ing Dean because he might find this patch interesting.
Comment 33 Max Vujovic 2013-04-02 11:34:16 PDT
Created attachment 196194 [details]
Patch for Landing

Rebased patch for landing. Running it through EWS.
Comment 34 WebKit Review Bot 2013-04-02 13:55:20 PDT
Comment on attachment 196194 [details]
Patch for Landing

Attachment 196194 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17376264

New failing tests:
css3/filters/composited-layer-child-bounds-after-composited-to-sw-shadow-change.html
css3/filters/composited-layer-promotion-after-outset-overlap-change-using-composited-shadow.html
css3/filters/composited-layer-promotion-after-outset-overlap-change-using-sw-shadow.html
Comment 35 WebKit Review Bot 2013-04-02 13:55:25 PDT
Created attachment 196219 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 36 Max Vujovic 2013-04-02 14:08:38 PDT
Created attachment 196221 [details]
Patch for Landing

Updated Chromium expectations for new tests. Since my last update, DRT has started printing "(contentsOpaque 1)" in some of the test results.
Comment 37 Max Vujovic 2013-04-02 14:59:58 PDT
Comment on attachment 196221 [details]
Patch for Landing

Chromium EWS looks good. Setting cq+.
Comment 38 WebKit Review Bot 2013-04-02 15:30:16 PDT
Comment on attachment 196221 [details]
Patch for Landing

Clearing flags on attachment: 196221

Committed r147502: <http://trac.webkit.org/changeset/147502>
Comment 39 WebKit Review Bot 2013-04-02 15:30:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Simon Fraser (smfr) 2013-04-05 14:58:18 PDT
This broke animations of CAFilters on Mac.
Comment 41 Max Vujovic 2013-04-05 15:21:38 PDT
(In reply to comment #40)
> This broke animations of CAFilters on Mac.

Linking to bug 114067 for more details.
Comment 42 Max Vujovic 2013-04-08 11:19:25 PDT
Reopening this bug because its changeset was reverted in bug 114067.

I believe the next steps for this bug are to automate and check in the regression test that smfr provided. See bug 114179.
Comment 43 Csaba Osztrogonác 2014-02-13 04:16:02 PST
Comment on attachment 194131 [details]
Patch for Review

Cleared Dean Jackson's review+ from obsolete attachment 194131 [details] so that this bug does not appear in http://webkit.org/pending-commit.