[webkit-reviews] review granted: [Bug 180239] [CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation : [Attachment 328443] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Zan Dobersek
<zan at falconsigh.net>'s request for review:
Bug 180239: [CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo
implementation
https://bugs.webkit.org/show_bug.cgi?id=180239

Attachment 328443: Patch

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




--- Comment #8 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 328443
  --> https://bugs.webkit.org/attachment.cgi?id=328443
Patch

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

I appreciate the high-quality commit message. As usual.

> Source/WebCore/platform/graphics/nicosia/NicosiaBuffer.cpp:48
> +    if (!tryFastZeroedMalloc(checkedArea.unsafeGet()).getValue(bufferData))
> +	   return;

Should it really be fallible? If rendering is going to be broken, wouldn't it
be better to crash?

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContext.h:45
> +    static void paint(Buffer& buffer, const T& functor)

Consider naming it paintFunctor, that might be slightly more clear.

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.cpp:46
> +    // Balanced by the deref in the s_bufferKey user data destroy callback.
> +    buffer.ref();

You can't use a Ref<Buffer> for this because you're not allowed to capture
anything in the lambda, right? OK then.

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.cpp:60
> +#ifndef NDEBUG

#if !ASSERT_DISABLED

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.cpp:81
> +    m_graphicsContext = nullptr;
> +    m_platformContext = nullptr;
> +    m_cr = nullptr;
> +    m_surface = nullptr;

I like that you ensured they're destroyed in the reverse order that they're
declared in the class.

I'm tempted to suggest putting this entire block in an #if !ASSERT_DISABLED
statement, for clarity. Up to you. The risk is that adding more objects to the
class could then result in a different order of destruction depending on if
assertions are enabled, so probably not worth changing....

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.h:56
> +    RefPtr<cairo_t> m_cr;

m_cr is not a very good name... maybe m_cairo? Not that that's much better....