Bug 102694 – [Skia] Encapsulate access to PlatformContextSkia's SkCanvas

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Florin Malita]

PlatformContextSkia's underlying SkCanvas is accessible via canvas() and there are 90+ call sites that use it directly.

This bug focuses on refactoring and adding the SkCanvas wrapper methods necessary to reduce the number of direct canvas users to a manageable level, in order to facilitate https://bugs.webkit.org/show_bug.cgi?id=102272.

[Florin Malita]

Created attachment 174995 [details]
Patch

[Stephen White]

Comment on attachment 174995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174995&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:206
> +    SkDevice* getTopDevice(bool updateMatrixClip = false) const;

It looks like this parameter isn't used by any of the current callers.  Could we eliminate it?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:214
> +    bool clipPath(const SkPath&, SkRegion::Op = SkRegion::kIntersect_Op, bool = false);
> +    bool clipRect(const SkRect&, SkRegion::Op = SkRegion::kIntersect_Op, bool = false);

Prefer a new enum here, instead of a bool.  This would make the call sites self-documenting.  See #10 in the WebKit coding style doc.

[Dana Jansens]

Comment on attachment 174995 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=174995&action=review

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:298
> +    m_canvas->writePixels(bitmap, x, y, config8888);

This should call didDrawRect, passing in the bitmap as well, with an SkPaint with Xfermode set to kSrc_Mode.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:354
> +    m_canvas->drawBitmap(bitmap, left, top, paint);

This should call didDrawRect, passing in the bitmap as well.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:360
> +    m_canvas->drawBitmapRect(bitmap, isrc, dst, paint);

This should call didDrawRect, passing in the bitmap as well.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:396
> +    m_canvas->drawIRect(rect, paint);

Have this call didDrawRect() also.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:402
> +    m_canvas->drawPosText(text, byteLength, pos, paint);

This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:408
> +    m_canvas->drawPosTextH(text, byteLength, xpos, constY, paint);

This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:414
> +    m_canvas->drawTextOnPath(text, byteLength, path, matrix, paint);

This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.

[Dana Jansens]

Can we add unit tests for all the new draw methods to Source/WebKit/chromium/tests/PlatformContextSkiaTest.cpp to make sure they are recording draws.

[Florin Malita]

(In reply to comment #2)
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:206
> > +    SkDevice* getTopDevice(bool updateMatrixClip = false) const;
> 
> It looks like this parameter isn't used by any of the current callers.  Could we eliminate it?

Sure, will do.

> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:214
> > +    bool clipPath(const SkPath&, SkRegion::Op = SkRegion::kIntersect_Op, bool = false);
> > +    bool clipRect(const SkRect&, SkRegion::Op = SkRegion::kIntersect_Op, bool = false);
> 
> Prefer a new enum here, instead of a bool.  This would make the call sites self-documenting.  See #10 in the WebKit coding style doc.

Will do.

[Florin Malita]

Sounds good Dana, but could we do this in a follow-up patch? I'd rather keep this one refactoring-only since it's grown quite a bit.

(In reply to comment #3) 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:402
> > +    m_canvas->drawPosText(text, byteLength, pos, paint);
> 
> This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:408
> > +    m_canvas->drawPosTextH(text, byteLength, xpos, constY, paint);
> 
> This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:414
> > +    m_canvas->drawTextOnPath(text, byteLength, path, matrix, paint);
> 
> This should call didDrawBounded with the bounds of the text, or didDrawUnbounded if the bounds of the text aren't known.

There doesn't seem to be a good way for getting the positioned text bounds, but maybe we can add something to Skia to avoid didDrawUnbounded.

[Florin Malita]

Created attachment 175243 [details]
Patch

Take two.

[Stephen White]

Comment on attachment 175243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175243&action=review

> Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:54
> -    return canvasForContext(context)->getTopDevice()->accessBitmap(false);
> +    return context.platformContext()->getTopDevice()->accessBitmap(false);

Not essential for this patch, but it looks like the only use we make of the device is to access the backing store, or to test whether it's vectorial.

I wonder if we should provide an "accessBitmap()" (or "accessBackingStore()") and "isVector()" on PlatformContextSkia, so that we can avoid exposing the SkDevice?

(As a side note, I think all places where we are calling getDevice()->accessBitmap() could equally (and more correctly?) be calling getTopDevice()->accessBitmap(), but they're semantically equivalent because we're not in the middle of a saveLayer().  Mike can correct me if I'm wrong.  Also not new to this patch.)

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:48
> +enum AntiAliasingMode { BW, AntiAliased };

BW is not a clear antonym of AntiAliased.  How about NonAntiAliased?

[Mike Reed]

+1 for hiding access to SkDevice.... our longer term goal is to remove *all* references to SkDevice, and add the necessary APIs on SkCanvas. Not required, but doing that encapsulation now on PlatformContextSkia would be a nice step forward.

[Florin Malita]

(In reply to comment #8)
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.h:48
> > +enum AntiAliasingMode { BW, AntiAliased };
> 
> BW is not a clear antonym of AntiAliased.  How about NonAntiAliased?

Sounds good.


> > Source/WebCore/platform/graphics/chromium/TransparencyWin.cpp:54
> > -    return canvasForContext(context)->getTopDevice()->accessBitmap(false);
> > +    return context.platformContext()->getTopDevice()->accessBitmap(false);
> 
> Not essential for this patch, but it looks like the only use we make of the device is to access the backing store, or to test whether it's vectorial.
> 
> I wonder if we should provide an "accessBitmap()" (or "accessBackingStore()") and "isVector()" on PlatformContextSkia, so that we can avoid exposing the SkDevice?

(In reply to comment #9)
> +1 for hiding access to SkDevice.... our longer term goal is to remove *all* references to SkDevice, and add the necessary APIs on SkCanvas. Not required, but doing that encapsulation now on PlatformContextSkia would be a nice step forward.

Alright, I'll give that a shot.

[Florin Malita]

(In reply to comment #8)
> I wonder if we should provide an "accessBitmap()" (or "accessBackingStore()") and "isVector()" on PlatformContextSkia, so that we can avoid exposing the SkDevice?

I just noticed there's already a bitmap() method which provides read-only access to the base device bitmap. So maybe add a "layerBitmap()" with an access mode flag to cover the remaining users?

[Florin Malita]

Created attachment 175271 [details]
Patch

[Stephen White]

(In reply to comment #11)
> (In reply to comment #8)
> > I wonder if we should provide an "accessBitmap()" (or "accessBackingStore()") and "isVector()" on PlatformContextSkia, so that we can avoid exposing the SkDevice?
> 
> I just noticed there's already a bitmap() method which provides read-only access to the base device bitmap. So maybe add a "layerBitmap()" with an access mode flag to cover the remaining users?

Sounds good.

I've always found it kind of unfortunate that bitmap() returns a const SkBitmap*, but that's a refactoring for another day.  I'd have your layerBitmap() return a const SkBitmap&, as accessBitmap() does.

[Florin Malita]

(In reply to comment #13)
> (In reply to comment #11)
> > (In reply to comment #8)
> > > I wonder if we should provide an "accessBitmap()" (or "accessBackingStore()") and "isVector()" on PlatformContextSkia, so that we can avoid exposing the SkDevice?
> > 
> > I just noticed there's already a bitmap() method which provides read-only access to the base device bitmap. So maybe add a "layerBitmap()" with an access mode flag to cover the remaining users?
> 
> Sounds good.
> 
> I've always found it kind of unfortunate that bitmap() returns a const SkBitmap*, but that's a refactoring for another day.  I'd have your layerBitmap() return a const SkBitmap&, as accessBitmap() does.

Thanks, I'll update the patch then.

[Florin Malita]

Created attachment 175279 [details]
Patch

[Stephen White]

Comment on attachment 175279 [details]
Patch

Looks great!  Thanks for the extra effort.  r=me

[WebKit Review Bot]

Comment on attachment 175279 [details]
Patch

Clearing flags on attachment: 175279

Committed r135390: <http://trac.webkit.org/changeset/135390>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.