[webkit-reviews] review canceled: [Bug 84479] [chromium] Some background filters require inflating damage on the surface behind them : [Attachment 138921] 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 138921: Patch
https://bugs.webkit.org/attachment.cgi?id=138921&action=review

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


>> Source/WebCore/platform/graphics/chromium/cc/CCDamageTracker.cpp:65
>> +	IntRect pixelOutsets(-left, -top, left + right, top + bottom);
> 
> I feel like this is quite misleading.  It encodes and obscures the real
meaning of left, right, bottom top.   Those values are actually "deltas" of
position, while an IntRect should always represent an actual rect with position
and size.
> 
> I think it's much clearer to just skip creating the IntRect and use left,
right, top, bottom directly in the move/expand operations immediately below. 
=)

Oh sure, this was an artifact of old code where I was trying to merge pixel
outsets. Done.

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:417
>> +	// Setting the update rect should cause the corresponding damage to the

> 
> I know this is the World's Most Trivial Nit, but would it be OK if we labeled
each scenario with something like "CASE 1:"  "CASE 2:"	 etc.?
> Personally I find those visual cues really help make the code quite a bit
more readable.

done.

>> Source/WebKit/chromium/tests/CCDamageTrackerTest.cpp:419
>> +	// Since the damage extends to the right/bottom outside of the blured
> 
> s/blured/blurred

done.