[webkit-reviews] review requested: [Bug 90092] [chromium] ContentLayerPainter should generate opaque rects in content space : [Attachment 149966] Patch

[bugzilla-daemon at webkit.org]

vollick at chromium.org has asked	for review:
Bug 90092: [chromium] ContentLayerPainter should generate opaque rects in
content space
https://bugs.webkit.org/show_bug.cgi?id=90092

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

------- Additional Comments from vollick at chromium.org
Fixed the layout test. We needed to grab the transform from the canvas _before_
painting (just like was being done in the
OpaqueRectTrackingContentLayerDelegate
previously).

(In reply to comment #2)
> (From update of attachment 149787 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=149787&action=review
>
> > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:75
> > -	 m_painter->paint(canvas, scaledContentRect, resultingOpaqueRect);
> > +	 m_painter->paint(canvas, scaledContentRect, contentsScale,
resultingOpaqueRect);
>
> As a sanity-preserving constraint, if it's possible, I'd like any paint(rect,
&resultingOpaque) to return resultingOpaque in the same space as rect.
sgtm.
>
> So, with that in mind, what about leaving the signature of
LayerPainterChromium the same and just transforming the resultingOpaqueRect
here from layer space back to content space? CanvasLayerTextureUpdater is the
one that's introducing this extra scale, so it kind of makes sense to me that
it should be the one to undo it before returning the result.
Done.
>
> (Also, for clarity, can you maybe rename scaledContentRect and
resultingOpaqueRect to include the name of the space they're in in the variable
name, like layerRect and resultingOpaqueLayerRect?)
Done.
>
> >
Source/WebCore/platform/graphics/chromium/OpaqueRectTrackingContentLayerDelegat
e.cpp:-59
> > -	 // Record transform prior to painting, as all opaque tracking will be
> > -	 // relative to this current value.
> > -	 AffineTransform canvasToContentTransform = context.getCTM().inverse();
> > -
>
> I don't follow these changes to OpaqueRectTrackingContentLayerDelegate? Are
these tested?
They weren't -- I'd missed this -- but the unit test now uses the ORTCLD.