[webkit-reviews] review canceled: [Bug 83608] [chromium] Refactor opaque content transform out of Skia context : [Attachment 136519] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has canceled Adrienne Walker
<enne at google.com>'s request for review:
Bug 83608: [chromium] Refactor opaque content transform out of Skia context
https://bugs.webkit.org/show_bug.cgi?id=83608

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

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=136519&action=review


>>
Source/WebCore/platform/graphics/chromium/BitmapCanvasLayerTextureUpdater.cpp:9
5
>> +	IntRect opaqueRect;
> 
> how would you feel about calling this resultingOpaqueRect also?

Wouldn't that conflict with the existing resultingOpaqueRect parameter?

>> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:72
>> +	    if (transform.preservesAxisAlignment()) {
> 
> Why do you check this? If anything it seems like it would make a good assert?

> 
> The transform should only be the translate/scale set in this function.

I was mimicking the removal of the code on OpaqueRegionSkia.cpp:294.

>>
Source/WebCore/platform/graphics/chromium/SkPictureCanvasLayerTextureUpdater.cp
p:63
>> +	IntRect opaqueRect;
> 
> resultingOpaqueRect also maybe?

Same issue? I could make resultingOpaqueRect not optional, if that would make
things clearer.

>> Source/WebCore/platform/graphics/skia/OpaqueRegionSkia.cpp:-256
>> -
> 
> This needs to stay. This is moving things to the SkCanvas's device space. We
track all paints in this space. The transform to layer space is just the one
below that you removed. Do the PlatformContextSkiaTest unit tests pass with
this change..?

Oops! Nice catch.  I think I was being overzealous after removing the one on
line 294.

>> Source/WebKit/chromium/tests/LayerTextureUpdaterTest.cpp:262
>> +	IntRect scaledRect(5, 10, 24, 37);
> 
> I'm confused by this.. the rect moves left/up but is half the size?

Ack! Now that I look at this, it's totally wrong.  If I draw a canvas has a
given opaque rect, then the resulting final opaque rect is content scale *
canvas opaque rect, and not canvas opaque rect / content scale.