IPC serialization of enums should serialize std::underlying_type instead of uint64_t
Created attachment 395888 [details] Patch
Created attachment 395895 [details] Patch
Created attachment 395904 [details] Patch
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?
(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.
Created attachment 395906 [details] Patch
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.
Created attachment 395914 [details] Patch
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.
Committed without adding that assertion to https://trac.webkit.org/changeset/259804/webkit
<rdar://problem/61520948>
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.
Reverted r259804 for reason: Breaks the watchOS build. Committed r259817: <https://trac.webkit.org/changeset/259817>
(In reply to Ryan Haddad from comment #13) > Reverted r259804 for reason: > > Breaks the watchOS build. Build log in radar.
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.
Created attachment 396036 [details] patch using uint32_t instead of UInt32
Committed r259845: <https://trac.webkit.org/changeset/259845> All reviewed patches have been landed. Closing bug and clearing flags on attachment 396036 [details].
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
Why? Wouldn't that behave the same? It reads similarly.
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.
Ah, ok.