[webkit-reviews] review canceled: [Bug 84479] [chromium] Some background filters require inflating damage on the surface behind them : [Attachment 138420] Patch

[bugzilla-daemon at webkit.org]

Dana Jansens <danakj at chromium.org> has canceled Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 84479: [chromium] Some background filters require inflating damage on the
surface behind them
https://bugs.webkit.org/show_bug.cgi?id=84479

Attachment 138420: Patch
https://bugs.webkit.org/attachment.cgi?id=138420&action=review

------- Additional Comments from Dana Jansens <danakj at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=138420&action=review


>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:180
>> +	    // Visit layers in front-to-back order.
> 
> To simplify this, would it be possible to process layers in back-to-front
order and then immediately expand the damage rect when we come upon a
contributing surface with background filters that move pixels rather than
having to call this function again with an offset?

Yes! I think that is a great idea.

>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:196
>> +		damageRect.unite(damageRectBelowFilters);
> 
> Is it easy to clip the damage here to the layer size? If it's not a big deal,
you should to avoid a bigger scissor than needed.  Even if occlusion tracking
is more loose, I think there's still a win by being as conservative as possible
with our damage rects.

Hm.... damageRect.unite(union(damageRectBelowFilters,
intersection(expandedDamageRectBelowFilters, layerRect))) !

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:416
>> +	expectedDamageRect.expand(outsetLeft + outsetRight, outsetTop +
outsetBottom);
> 
> This is kind of circular, in that you're just testing if filter.getOutsets is
used.  Do you think it'd be worth baking literal outsets into this test or
asserting that the outsets are what you expect they are?

Well, I think that is testing FilterOperations, which isn't the goal here. It
does seem a bit circular.. but its verifying that the damage matches what the
FilterOperations says it should do. It seems scoped correctly here to me. The
outsets as they are I think are also larger than what they need to be, and
would be testing an implementation details of FilterOperations that isn't
important here?