Bug 215143 – Make GlyphBufferAdvance and GlyphBufferOrigin more robust

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Make GlyphBufferAdvance and GlyphBufferOrigin more robust

[Myles C. Maxfield]

Created attachment 405957 [details]
Patch

[Myles C. Maxfield]

Created attachment 405959 [details]
Patch

[Darin Adler]

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.

[Myles C. Maxfield]

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?

[Darin Adler]

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.

[Myles C. Maxfield]

Created attachment 405984 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/66864884>

[Darin Adler]

Comment on attachment 405984 [details]
Patch

This is great!

[Myles C. Maxfield]

Created attachment 408113 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408114 [details]
Patch for committing

[Myles C. Maxfield]

Created attachment 408137 [details]
Patch for committing

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 408150 [details]
Patch for committing

[Darin Adler]

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.

[EWS]

Committed r266688: <https://trac.webkit.org/changeset/266688>

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

[Myles C. Maxfield]

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.

[Darin Adler]

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.