Bug 210228 – IPC serialization of enums should serialize std::underlying_type instead of uint64_t

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

IPC serialization of enums should serialize std::underlying_type instead of uint64_t

[Alex Christensen]

Created attachment 395888 [details]
Patch

[Alex Christensen]

Created attachment 395895 [details]
Patch

[Alex Christensen]

Created attachment 395904 [details]
Patch

[Darin Adler]

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?

[Alex Christensen]

(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.

[Alex Christensen]

Created attachment 395906 [details]
Patch

[Darin Adler]

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.

[Alex Christensen]

Created attachment 395914 [details]
Patch

[Alex Christensen]

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.

[Alex Christensen]

Committed without adding that assertion to https://trac.webkit.org/changeset/259804/webkit

[Radar WebKit Bug Importer]

<rdar://problem/61520948>

[Darin Adler]

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.

[Ryan Haddad]

Reverted r259804 for reason:

Breaks the watchOS build.

Committed r259817: <https://trac.webkit.org/changeset/259817>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #13)
> Reverted r259804 for reason:
> 
> Breaks the watchOS build.
Build log in radar.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 396036 [details]
patch using uint32_t instead of UInt32

[EWS]

Committed r259845: <https://trac.webkit.org/changeset/259845>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396036 [details].

[Darin Adler]

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

[Alex Christensen]

Why?  Wouldn't that behave the same?  It reads similarly.

[Darin Adler]

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.

[Alex Christensen]

Ah, ok.