[CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation
Created attachment 328078 [details] Patch
Created attachment 328226 [details] Patch
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
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
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.
Created attachment 328443 [details] Patch Still up for review.
Why was the name Nicosia chosen?
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....
(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.
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.
(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.
Created attachment 328562 [details] Patch for landing
Comment on attachment 328562 [details] Patch for landing Clearing flags on attachment: 328562 Committed r225573: <https://trac.webkit.org/changeset/225573>
All reviewed patches have been landed. Closing bug.
<rdar://problem/35895996>
(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.