[webkit-reviews] review canceled: [Bug 95121] [skia] Drawing of sub-rectangle in a bitmap is mis-aligned : [Attachment 164062] Patch

[bugzilla-daemon at webkit.org]

Hin-Chung Lam <hclam at google.com> has canceled Hin-Chung Lam
<hclam at google.com>'s request for review:
Bug 95121: [skia] Drawing of sub-rectangle in a bitmap is mis-aligned
https://bugs.webkit.org/show_bug.cgi?id=95121

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

------- Additional Comments from Hin-Chung Lam <hclam at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=164062&action=review


Taking this off review since we're holding this until Skia's API change took
place.

>> Source/WebCore/ChangeLog:31
>> +	    fragments.
> 
> Is this still the case, now that we're keying off the scaled subrect size?

This is still the case. We don't cache the scaled full image but scaled
fragment. This fragment can be used to cover a smaller scale request because
scale factor is now consistent.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:229
>> +	enclosingSrcRect.intersect(bitmapRect); // Clip to bitmap rectangle.
> 
> Still wondering if we need to check the return value of intersect() here, or
if it's handled by the caller?

This is handled by caller. Line 415 checks to see of either of the output
rectangles are empty.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:288
>> +	enclosingScaledSrcRect->intersect(SkIRect::MakeSize(scaledImageSize));
> 
> As above:  do we need to check the return value of intersect() here, or is an
empty rect ok?

It is okay to be empty. We just get an empty image below.

>> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:347
>> +	canvas.clipRect(destRectVisibleSubset);
> 
> Still concerned about clipping performance here.  Mike:  should I worry about
this?  Is Skia fast enough at a simple clipped rect that this is not a concern?
 If not, Alpha, is there a way we can skip clipping when unnecessary?

Looks like Mike's change is going to land soon. I can hold this off until then.
We don't nee to clip with the new API.