Bug 81263

Created attachment 137638 [details] Patch V1

Comment on attachment 137638 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=137638&action=review Just one small question on naming, and some typos. > Source/WebCore/ChangeLog:10 > + Even if the element is completely offscreen, but it's shadow is in the viewport, we can still compute the source typo it's -> its > Source/WebCore/ChangeLog:14 > + No new tests, this change doesn't have visible results and the functionality should > + already be covered by previous filter tests. We couldn't have a repaint test specifically for this? I'm not worried if not. > Source/WebCore/rendering/FilterEffectRenderer.cpp:370 > + // Note that the outsets are reveresed here because we are going backwards -> we have the dirty rect and Typo - reveresed -> reversed. > Source/WebCore/rendering/FilterEffectRenderer.cpp:379 > +bool FilterEffectRendererHelper::prepareFilterEffect(RenderLayer* renderLayer, const LayoutRect& filterBoxRect, const LayoutRect& dirtyRect, const LayoutRect& layerRepaintRect) Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed?

(In reply to comment #2) Thanks for the review. > Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed? It's not calling prepare anymore, but it's preparing the "repaint" rectangle and updating the backing store size. I actually think that we should rename the other functions: FilterEffectRenderer::updateBackingStore -> updateBackingStoreRect FilterEffectRenderer:: prepare -> allocateBackingStoreIfNeeded FilterEffectRenderer:: prepare should not be required to call clearIntermediateResults() anymore, because we do that when we finish in FilterEffectRendererHelper::applyFilterEffect, so next time it should already be cleaned up. What do you think?

(In reply to comment #3) > (In reply to comment #2) > Thanks for the review. > > > Unless I'm missing something, but this method no longer calls prepare(), so maybe it should be renamed? > > It's not calling prepare anymore, but it's preparing the "repaint" rectangle and updating the backing store size. > > I actually think that we should rename the other functions: > > FilterEffectRenderer::updateBackingStore -> updateBackingStoreRect > FilterEffectRenderer:: prepare -> allocateBackingStoreIfNeeded > > FilterEffectRenderer:: prepare should not be required to call clearIntermediateResults() anymore, because we do that when we finish in FilterEffectRendererHelper::applyFilterEffect, so next time it should already be cleaned up. > > What do you think? Sounds good.

(In reply to comment #2) > (From update of attachment 137638 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137638&action=review > > > Source/WebCore/ChangeLog:14 > > + No new tests, this change doesn't have visible results and the functionality should > > + already be covered by previous filter tests. > > We couldn't have a repaint test specifically for this? I'm not worried if not. There's already a test that makes sure that memory is not exploding for offscreen buffers. Also, blur repaints are already covered. I'm not sure what else could we do for it. I think we need something in DRT to dump the intermediate filter bitmaps. I will add a bug for that http://svn.webkit.org/repository/webkit/trunk/LayoutTests/css3/filters/offscreen-filters-memory-usage.html

Landed in http://trac.webkit.org/changeset/114529 .

(In reply to comment #6) > Landed in http://trac.webkit.org/changeset/114529 . Build fix for Qt was landed in http://trac.webkit.org/changeset/114534

Created attachment 137736 [details] QT Fix I've already committed this, unreviewed. QT build is also using FilterEffectRenderer::prepare and I've updated it to the new name.