Bug 180239 – [CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

[CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation

[Zan Dobersek]

Created attachment 328078 [details]
Patch

[Zan Dobersek]

Created attachment 328226 [details]
Patch

[EWS Watchlist]

Comment on attachment 328226 [details]
Patch

Attachment 328226 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/5465541

New failing tests:
js/mozilla/eval/exhaustive-fun-normalcaller-direct-normalcode.html

[EWS Watchlist]

Created attachment 328227 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Michael Catanzaro]

Comment on attachment 328226 [details]
Patch

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

> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.h:49
> +    PaintingContextCairo(Buffer&);

Mark it as explicit.

[Zan Dobersek]

Created attachment 328443 [details]
Patch

Still up for review.

[Alex Christensen]

Why was the name Nicosia chosen?

[Michael Catanzaro]

Comment on attachment 328443 [details]
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....

[Michael Catanzaro]

(In reply to Alex Christensen from comment #7)
> Why was the name Nicosia chosen?

I'll let Zan answer this.

But it's not finalized... naming things is hard.

[Zan Dobersek]

Comment on attachment 328443 [details]
Patch

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

>> Source/WebCore/platform/graphics/nicosia/NicosiaBuffer.cpp:48
>> +        return;
> 
> Should it really be fallible? If rendering is going to be broken, wouldn't it be better to crash?

To be decided. It's possible to avoid painting if the necessary memory isn't available for allocation, and it would avoid crashes in memory-intensive scenarios, but in exchange for incomplete display of the Web content.

>> Source/WebCore/platform/graphics/nicosia/NicosiaPaintingContextCairo.cpp:46
>> +    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.

It's not possible to capture anything in the lambda because then that lambda can't be converted to the cairo_destroy_func_t type, which is just a function pointer.

[Zan Dobersek]

(In reply to Alex Christensen from comment #7)
> Why was the name Nicosia chosen?

Nicosia's the capital of Cyprus, so it's a play on Cairo being the capital of Egypt.

It doesn't play that well because Cairo is a whole 2D graphics library, and this is just an abstraction layer. So it could probably use a better name.

[Zan Dobersek]

Created attachment 328562 [details]
Patch for landing

[Zan Dobersek]

Comment on attachment 328562 [details]
Patch for landing

Clearing flags on attachment: 328562

Committed r225573: <https://trac.webkit.org/changeset/225573>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35895996>

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #10)
> To be decided. It's possible to avoid painting if the necessary memory isn't
> available for allocation, and it would avoid crashes in memory-intensive
> scenarios, but in exchange for incomplete display of the Web content.

Keep in mind that GLib (g_malloc) does not support fallible allocations. If we fail an allocation here, WebKit will be crashing sooner rather than later regardless.