[webkit-reviews] review canceled: [Bug 222532] Fix inspector viewing of canvas save/restore stack, and tighten and simplify CanvasRenderingContext2DBase : [Attachment 421782] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has canceled Darin Adler <darin at apple.com>'s
request for review:
Bug 222532: Fix inspector viewing of canvas save/restore stack, and tighten and
simplify CanvasRenderingContext2DBase
https://bugs.webkit.org/show_bug.cgi?id=222532

Attachment 421782: Patch

https://bugs.webkit.org/attachment.cgi?id=421782&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 421782
  --> https://bugs.webkit.org/attachment.cgi?id=421782
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:278
>> +}
> 
> I am not sure if this is safe to do since the
OffscreenCanvasRenderingContext2D inherits from this class and is used on a
non-main thread.

Your broader point about this class is definitely correct and does call into
question the design pattern I used here.

For the actual safety of the code change, the only caller of this function is
CanvasRenderingContext2D::setFont, which uses it to call setOneFamily, which
uses an AtomString. For safety, we could move this function into
CanvasRenderingContext2D and instead export the const char* that it uses to
initialize the AtomString, which I suppose is how it was working before, except
that it was making a new AtomString every time, so I would want to add a global
there for efficiency.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:282
> +    static NeverDestroyed<String> font = DefaultFont;

Using the same global String across threads is also unsafe, not just
AtomString. So I think your comment about the function above is actually
correctly pointing out a serious problem with this one! Creating a new String
every time we make a CanvasRenderingContext2DBase::State is inefficient but I
guess it’s required to keep things thread-safe! That is frustrating.

>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:417
>> +FloatRect Path::strokeBoundingRect(const
Optional<Function<void(GraphicsContext&)>>& strokeStyleApplier) const
> 
> WTF::Function has null state (though it is debatable whether that is a good
thing). Can that be used rather than the Optional?

Yes, I will switch to that.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:-54
>> -class BoundingRectStrokeStyleApplier final : public StrokeStyleApplier {
> 
> I wonder if we have other one off delegate patterns like this that can be
replaced with function objects?

I am certain that we do.