Bug 180239 - [CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation
Summary: [CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-01 01:13 PST by Zan Dobersek
Modified: 2017-12-06 16:41 PST (History)
5 users (show)

See Also:


Attachments
Patch (25.86 KB, patch)
2017-12-01 02:00 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch (25.32 KB, patch)
2017-12-02 02:07 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.12 MB, application/zip)
2017-12-02 03:30 PST, Build Bot
no flags Details
Patch (25.32 KB, patch)
2017-12-05 00:10 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff
Patch for landing (25.45 KB, patch)
2017-12-06 03:26 PST, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2017-12-01 01:13:28 PST
[CoordGraphics] Introduce Nicosia::PaintingContext, add Cairo implementation
Comment 1 Zan Dobersek 2017-12-01 02:00:19 PST
Created attachment 328078 [details]
Patch
Comment 2 Zan Dobersek 2017-12-02 02:07:55 PST
Created attachment 328226 [details]
Patch
Comment 3 Build Bot 2017-12-02 03:30:56 PST
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
Comment 4 Build Bot 2017-12-02 03:30:58 PST
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 5 Michael Catanzaro 2017-12-04 04:50:55 PST
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.
Comment 6 Zan Dobersek 2017-12-05 00:10:15 PST
Created attachment 328443 [details]
Patch

Still up for review.
Comment 7 Alex Christensen 2017-12-05 15:40:41 PST
Why was the name Nicosia chosen?
Comment 8 Michael Catanzaro 2017-12-05 17:21:12 PST
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....
Comment 9 Michael Catanzaro 2017-12-05 17:22:08 PST
(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 10 Zan Dobersek 2017-12-06 03:19:35 PST
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.
Comment 11 Zan Dobersek 2017-12-06 03:26:10 PST
(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.
Comment 12 Zan Dobersek 2017-12-06 03:26:29 PST
Created attachment 328562 [details]
Patch for landing
Comment 13 Zan Dobersek 2017-12-06 03:46:53 PST
Comment on attachment 328562 [details]
Patch for landing

Clearing flags on attachment: 328562

Committed r225573: <https://trac.webkit.org/changeset/225573>
Comment 14 Zan Dobersek 2017-12-06 03:46:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2017-12-06 16:26:49 PST
<rdar://problem/35895996>
Comment 16 Michael Catanzaro 2017-12-06 16:41:12 PST
(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.