Bug 177883 – TextDecorationPainter::m_wavyOffset should be a float

• Component: Text
• Status: RESOLVED FIXED

[Daniel Bates]

Currently TextDecorationPainter::m_wavyOffset should be an int. It should be a float. This is consistent with its original type when this code was extracted from InlineTextBox in the patch for bug #152587.

[Daniel Bates]

Created attachment 322687 [details]
Patch

[Build Bot]

Attachment 322687 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/TextDecorationPainter.cpp:246:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 322687 [details]
Patch

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

> Source/WebCore/rendering/TextDecorationPainter.cpp:250
> +    : m_context { context }
> +    , m_decoration { decoration }
> +    , m_wavyOffset { wavyOffsetFromDecoration() }
> +    , m_isPrinting { renderer.document().printing() }
> +    , m_styles { stylesForRenderer(renderer, m_decoration, isFirstLine) }
> +    , m_lineStyle { isFirstLine ? renderer.firstLineStyle() : renderer.style() }

Are we going to do this everywhere? It seems like a lot of churn for zero gain.

> Source/WebCore/rendering/TextDecorationPainter.h:67
>      TextDecoration m_decoration;
> -    int m_wavyOffset { 0 };
> +    float m_wavyOffset;

Looks like these could be re-ordered for better packing (but maybe we never heap-allocate these).

[Daniel Bates]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 322687 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322687&action=review
> 
> > Source/WebCore/rendering/TextDecorationPainter.cpp:250
> > +    : m_context { context }
> > +    , m_decoration { decoration }
> > +    , m_wavyOffset { wavyOffsetFromDecoration() }
> > +    , m_isPrinting { renderer.document().printing() }
> > +    , m_styles { stylesForRenderer(renderer, m_decoration, isFirstLine) }
> > +    , m_lineStyle { isFirstLine ? renderer.firstLineStyle() : renderer.style() }
> 
> Are we going to do this everywhere? 

I hope so. See bug #176058. 

> It seems like a lot of churn for zero gain.
> 

I found this bug by changing to uniform initializer syntax because implicit conversions are disallowed when using this syntax among other benefits. Obviously, catching the implicit conversion from float to int for m_wavyOffset is not a big gain (m_wavyOffset is always equal to 1). I suspect we may find more significant bugs caused by implicit conversion if we used uniform initializer syntax everywhere.

> > Source/WebCore/rendering/TextDecorationPainter.h:67
> >      TextDecoration m_decoration;
> > -    int m_wavyOffset { 0 };
> > +    float m_wavyOffset;
> 
> Looks like these could be re-ordered for better packing (but maybe we never
> heap-allocate these).

Currently TextDecorationPainter is never heap allocated, but nothing is stopping it from being heap allocated. Regardless, we should look to better pack this data structure from a good programming practice perspective.

[Daniel Bates]

Comment on attachment 322687 [details]
Patch

Clearing flags on attachment: 322687

Committed r222862: <http://trac.webkit.org/changeset/222862>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34816545>