Bug 210228 - IPC serialization of enums should serialize std::underlying_type instead of uint64_t
Summary: IPC serialization of enums should serialize std::underlying_type instead of u...
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: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-08 17:16 PDT by Alex Christensen
Modified: 2020-04-10 12:22 PDT (History)
22 users (show)

See Also:


Attachments
Patch (20.37 KB, patch)
2020-04-08 17:17 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (21.70 KB, patch)
2020-04-08 18:07 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.58 KB, patch)
2020-04-08 20:15 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.76 KB, patch)
2020-04-08 20:45 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (28.10 KB, patch)
2020-04-08 23:16 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
patch using uint32_t instead of UInt32 (28.02 KB, patch)
2020-04-09 20:00 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2020-04-08 17:16:21 PDT
IPC serialization of enums should serialize std::underlying_type instead of uint64_t
Comment 1 Alex Christensen 2020-04-08 17:17:57 PDT
Created attachment 395888 [details]
Patch
Comment 2 Alex Christensen 2020-04-08 18:07:36 PDT
Created attachment 395895 [details]
Patch
Comment 3 Alex Christensen 2020-04-08 20:15:20 PDT
Created attachment 395904 [details]
Patch
Comment 4 Darin Adler 2020-04-08 20:36:22 PDT
Comment on attachment 395904 [details]
Patch

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

> Source/WebCore/platform/graphics/ca/PlatformCALayer.cpp:135
> +        graphicsContext.setTextDrawingMode(TextDrawingModeFlags(TextDrawingMode::Fill) | TextDrawingMode::Stroke);

I suggest initializer list syntax here: { TextDrawingMode::Fill, TextDrawingMode::Stroke }.

> Source/WebCore/platform/graphics/cairo/CairoOperations.cpp:318
> +    if (!(textDrawingMode & TextDrawingMode::Fill) || shadow.type() == ShadowBlur::NoShadow)

I suggest using contains here.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:-1763
> -    switch (mode) {

Not new, but a strange omission that this function does nothing if neither Fill or Stroke is set. Doesn’t set a mode at all. Not even kCGTextInvisible. Nor assert.

> Source/WebCore/rendering/TextPaintStyle.cpp:188
> +        newMode = newMode | TextDrawingMode::Letterpress;

I suggest add() here.

> Source/WebCore/rendering/TextPaintStyle.cpp:193
> +        newMode = newMode | TextDrawingMode::Stroke;

Ditto.

> Source/WebKit/Platform/IPC/Decoder.h:108
> -    template<typename E>
> -    auto decode(E& e) -> std::enable_if_t<std::is_enum<E>::value, bool>
> +    template<typename E, typename = std::enable_if_t<std::is_enum<E>::value>>
> +    bool decode(E& e)

Why this change?

> Source/WebKit/Platform/IPC/Decoder.h:120
> -    template<typename E, std::enable_if_t<std::is_enum<E>::value>* = nullptr>
> +    template<typename E, typename = std::enable_if_t<std::is_enum<E>::value>>

Why this change?

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:576
> +    encoder << static_cast<UInt32>(encoding);

Why UInt32 instead of uint32_t?
Comment 5 Alex Christensen 2020-04-08 20:44:40 PDT
(In reply to Darin Adler from comment #4)
> > Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:-1763
> > -    switch (mode) {
> 
> Not new, but a strange omission that this function does nothing if neither
> Fill or Stroke is set. Doesn’t set a mode at all. Not even kCGTextInvisible.
> Nor assert.
I'd rather not make a change like this in an IPC patch.  I put a FIXME comment.

> > Source/WebKit/Platform/IPC/Decoder.h:120
> > -    template<typename E, std::enable_if_t<std::is_enum<E>::value>* = nullptr>
> > +    template<typename E, typename = std::enable_if_t<std::is_enum<E>::value>>
> 
> Why this change?
This is a little nicer syntax.  It makes it clearer that we're not using a pointer for sfinae, we're using a typename.

> > Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:576
> > +    encoder << static_cast<UInt32>(encoding);
> 
> Why UInt32 instead of uint32_t?
CFStringEncoding is typedef'ed to UInt32, and since this is in ArgumentCodersCF, I figured it was a good idea to use the same CF type.
I'm also adding a FIXME comment to add parameter validation.  I wasn't confident enough to do it in this patch.
Comment 6 Alex Christensen 2020-04-08 20:45:18 PDT
Created attachment 395906 [details]
Patch
Comment 7 Darin Adler 2020-04-08 21:38:28 PDT
Comment on attachment 395906 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1772
> +    // FIXME: Should we use kCGTextInvisible if neither fill nor stroke is true?

Or assert this never gets called with that kind of mode?

> Source/WebCore/rendering/TextPaintStyle.cpp:204
> +    if (mode & TextDrawingMode::Fill && (fillColor != context.fillColor()))

Should eventually change this to contains and remove the extra parentheses here too. Sorry, missed it last time.

> Source/WebCore/rendering/TextPainter.cpp:167
> -                m_context.setTextDrawingMode(textDrawingMode & ~TextModeStroke);
> +                textDrawingMode.remove(TextDrawingMode::Stroke);
> +                m_context.setTextDrawingMode(textDrawingMode);
>                  paintTextWithShadows(shadowToUse, shadowColorFilter, *m_font, textRun, boxRect, textOrigin, startOffset, endOffset, nullAtom(), 0, false);
>                  shadowToUse = nullptr;
>                  m_context.setTextDrawingMode(textDrawingMode);

Don’t land it like this! Just noticed that this messes up the save/restore here. The second call to setTextDrawingMode is supposed to restore it to its old value.

> Source/WebCore/rendering/TextPainter.cpp:174
> -                m_context.setTextDrawingMode(textDrawingMode & ~TextModeFill);
> +                textDrawingMode.remove(TextDrawingMode::Fill);
> +                m_context.setTextDrawingMode(textDrawingMode);
>                  paintTextWithShadows(shadowToUse, shadowColorFilter, *m_font, textRun, boxRect, textOrigin, startOffset, endOffset, nullAtom(), 0, paintStyle.strokeWidth > 0);
>                  shadowToUse = nullptr;
>                  m_context.setTextDrawingMode(textDrawingMode);

Same error here.
Comment 8 Alex Christensen 2020-04-08 23:16:37 PDT
Created attachment 395914 [details]
Patch
Comment 9 Alex Christensen 2020-04-09 09:13:14 PDT
Comment on attachment 395914 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1773
> +        ASSERT_NOT_REACHED();

It turns out this is reached.
Comment 10 Alex Christensen 2020-04-09 09:18:54 PDT
Committed without adding that assertion to https://trac.webkit.org/changeset/259804/webkit
Comment 11 Radar WebKit Bug Importer 2020-04-09 09:19:18 PDT
<rdar://problem/61520948>
Comment 12 Darin Adler 2020-04-09 11:16:00 PDT
Comment on attachment 395914 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1773
>> +        ASSERT_NOT_REACHED();
> 
> It turns out this is reached.

An interesting mystery, why this is OK. It means in that case that we just leave the text drawing mode as-is. I wonder why that’s good.
Comment 13 Ryan Haddad 2020-04-09 13:00:47 PDT
Reverted r259804 for reason:

Breaks the watchOS build.

Committed r259817: <https://trac.webkit.org/changeset/259817>
Comment 14 Ryan Haddad 2020-04-09 13:01:29 PDT
(In reply to Ryan Haddad from comment #13)
> Reverted r259804 for reason:
> 
> Breaks the watchOS build.
Build log in radar.
Comment 15 Alex Christensen 2020-04-09 17:07:16 PDT
Comment on attachment 395914 [details]
Patch

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

> Source/WebKit/Shared/cf/ArgumentCodersCF.cpp:576
> +    encoder << static_cast<UInt32>(encoding);

watchOS didn't like this.  UInt32 is unsigned long on 32-bit platforms and unsigned int on 64-bit platforms, and there's no unsigned long encoder.  I think I'll just cast this to a uint32_t.
Comment 16 Alex Christensen 2020-04-09 20:00:34 PDT
Created attachment 396036 [details]
patch using uint32_t instead of UInt32
Comment 17 EWS 2020-04-09 21:54:00 PDT
Committed r259845: <https://trac.webkit.org/changeset/259845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396036 [details].
Comment 18 Darin Adler 2020-04-10 09:16:53 PDT
Comment on attachment 396036 [details]
patch using uint32_t instead of UInt32

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

> Source/WebCore/rendering/TextPaintStyle.cpp:207
> +    if (mode & TextDrawingMode::Stroke) {

Should switch this one to contains
Comment 19 Alex Christensen 2020-04-10 11:39:23 PDT
Why?  Wouldn't that behave the same?  It reads similarly.
Comment 20 Darin Adler 2020-04-10 12:14:27 PDT
It’s arbitrarily different than the one two lines above (and many others in the patch). No strong feeling on which style, but consistency is nice.
Comment 21 Alex Christensen 2020-04-10 12:22:59 PDT
Ah, ok.