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.
Created attachment 174995 [details] Patch
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.
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.
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.
(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.
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.
Created attachment 175243 [details] Patch Take two.
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?
+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.
(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.
(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?
Created attachment 175271 [details] Patch
(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.
(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.
Created attachment 175279 [details] Patch
Comment on attachment 175279 [details] Patch Looks great! Thanks for the extra effort. r=me
Comment on attachment 175279 [details] Patch Clearing flags on attachment: 175279 Committed r135390: <http://trac.webkit.org/changeset/135390>
All reviewed patches have been landed. Closing bug.