Bug 215143

Created attachment 405957 [details] Patch

Created attachment 405959 [details] Patch

Comment on attachment 405959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405959&action=review Thanks for considering my suggestion. > Source/WebCore/platform/graphics/GlyphBuffer.h:90 > + m_origins.append(createGlyphBufferOrigin()); m_origins.append({ }); > Source/WebCore/platform/graphics/GlyphBuffer.h:110 > + m_origins.insertVector(location, Vector<GlyphBufferOrigin>(length, createGlyphBufferOrigin())); Can just use { } here. > Source/WebCore/platform/graphics/GlyphBuffer.h:153 > + m_advances[i] = createGlyphBufferAdvance(FloatSize( Maybe overload makeGlyphBufferAdvance so we don’t need to cast to FloatSize here? And/or just use { } instead of FloatSize(). > Source/WebCore/platform/graphics/GlyphBufferMembers.h:72 > +inline GlyphBufferAdvance createGlyphBufferAdvance(const FloatSize& = FloatSize()); > +inline FloatSize floatSizeFromGlyphBufferAdvance(const GlyphBufferAdvance&); > +inline void setGlyphBufferAdvanceWidth(GlyphBufferAdvance&, float); > +inline void setGlyphBufferAdvanceHeight(GlyphBufferAdvance&, float); > +inline float glyphBufferAdvanceWidth(const GlyphBufferAdvance&); > +inline float glyphBufferAdvanceHeight(const GlyphBufferAdvance&); > +inline GlyphBufferOrigin createGlyphBufferOrigin(const FloatPoint& = FloatPoint()); > +inline FloatPoint floatPointFromGlyphBufferOrigin(const GlyphBufferOrigin&); > +inline void setGlyphBufferOriginX(GlyphBufferOrigin&, float); > +inline void setGlyphBufferOriginY(GlyphBufferOrigin&, float); > +inline float glyphBufferOriginX(const GlyphBufferOrigin&); > +inline float glyphBufferOriginY(const GlyphBufferOrigin&); In WebKit, I generally suggest reserving the word create for things that are heap-allocated and use make for things that are not. Also, because C++ has overloading, a lot of these functions could have shorter names. I suggest these names: makeGlyphBufferAdvance size setWidth setHeight width height makeGlyphBufferOrigin point setX setY x y It’s possible these names are *too* short, but I think we should try to get closer to these names. Code won’t be readable if the names are too long.

Comment on attachment 405959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405959&action=review >> Source/WebCore/platform/graphics/GlyphBuffer.h:90 >> + m_origins.append(createGlyphBufferOrigin()); > > m_origins.append({ }); Will this actually zero-fill a CGPoint struct?

Comment on attachment 405959 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=405959&action=review >>> Source/WebCore/platform/graphics/GlyphBuffer.h:90 >>> + m_origins.append(createGlyphBufferOrigin()); >> >> m_origins.append({ }); > > Will this actually zero-fill a CGPoint struct? Yes. Roughly speaking, there are only a few ways to *not* initialize scalars, and the mostly get zero-initialized. In the specific case of CGPoint and { }, this technically is aggregate initialization. Since the number of initializer clauses (0) is less than the number of members (2) the remaining members are value-initialized, which for scalars means they are zero-initialized.

Created attachment 405984 [details] Patch

<rdar://problem/66864884>

Comment on attachment 405984 [details] Patch This is great!

Created attachment 408113 [details] Patch for committing

Created attachment 408114 [details] Patch for committing

Created attachment 408137 [details] Patch for committing

I see what's happening. I need to initialize m_initialAdvance to (0, 0).

Created attachment 408150 [details] Patch for committing

Comment on attachment 408150 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=408150&action=review > Source/WebCore/platform/graphics/GlyphBuffer.h:216 > + GlyphBufferAdvance m_initialAdvance { makeGlyphBufferAdvance() }; I would have expected that just empty braces, { }, would do the trick here; surprised that a call to makeGlyphBufferAdvance() is needed.

Committed r266688: <https://trac.webkit.org/changeset/266688> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408150 [details].

Comment on attachment 408150 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=408150&action=review >> Source/WebCore/platform/graphics/GlyphBuffer.h:216 >> + GlyphBufferAdvance m_initialAdvance { makeGlyphBufferAdvance() }; > > I would have expected that just empty braces, { }, would do the trick here; surprised that a call to makeGlyphBufferAdvance() is needed. Me too! But this looks like the reason why the obsoleted patches were red.

Comment on attachment 408150 [details] Patch for committing View in context: https://bugs.webkit.org/attachment.cgi?id=408150&action=review >>> Source/WebCore/platform/graphics/GlyphBuffer.h:216 >>> + GlyphBufferAdvance m_initialAdvance { makeGlyphBufferAdvance() }; >> >> I would have expected that just empty braces, { }, would do the trick here; surprised that a call to makeGlyphBufferAdvance() is needed. > > Me too! But this looks like the reason why the obsoleted patches were red. In those obsoleted patches there were no braces at all. That would leave it uninitialized. My point is that you do need the braces but don’t need the function call.