Shrink TextRun object size

[Rob Buis]

TextRun does not do a very good job of bit packing. Since most of the time this is an auto var, no big deal, but for SVG it is a member var of SVGTextMetricsBuilder, and thus RenderSVGText. It also is a member var in

[Rob Buis]

Created attachment 140440 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140440 [details]
Patch

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

> Source/WebCore/platform/graphics/TextRun.h:170
> +    ExpansionBehavior m_expansionBehavior : 2;

bitfield enums will fail on MSVC (since they're signed).

> Source/WebCore/platform/graphics/TextRun.h:172
> +    TextDirection m_direction : 1;

Similar problem.

[Eric Seidel (no email)]

Also, if we're going to shrink the size of this, we should keep it small with a COMPILE_ASSERT.

[Rob Buis]

Created attachment 140442 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140442 [details]
Patch

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

> Source/WebCore/platform/graphics/TextRun.h:170
> +    ExpansionBehavior m_expansionBehavior : 2;

Same problem as before.  MSVC will make the enum signed, and thus you'll have one fewer bit than you think you do. :)  I thought we had helper macros for this?

[Rob Buis]

Hi Eric,

(In reply to comment #5)
> (From update of attachment 140442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140442&action=review
> 
> > Source/WebCore/platform/graphics/TextRun.h:170
> > +    ExpansionBehavior m_expansionBehavior : 2;
> 
> Same problem as before.  MSVC will make the enum signed, and thus you'll have one fewer bit than you think you do. :)  I thought we had helper macros for this?

It is an unsigned in disguise!

typedef unsigned ExpansionBehavior;

[Darin Adler]

Comment on attachment 140442 [details]
Patch

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

Can we add some compile time assertions to check if this is packed right, like we normally do in cases like this?

>>> Source/WebCore/platform/graphics/TextRun.h:170
>>> +    ExpansionBehavior m_expansionBehavior : 2;
>> 
>> Same problem as before.  MSVC will make the enum signed, and thus you'll have one fewer bit than you think you do. :)  I thought we had helper macros for this?
> 
> It is an unsigned in disguise!
> 
> typedef unsigned ExpansionBehavior;

Why not be explicit about it, given the likelihood of confusion?

[Rob Buis]

Hi Darin,

(In reply to comment #7)
> (From update of attachment 140442 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140442&action=review
> 
> Can we add some compile time assertions to check if this is packed right, like we normally do in cases like this?

Good idea, did that before landing.

> >>> Source/WebCore/platform/graphics/TextRun.h:170
> >>> +    ExpansionBehavior m_expansionBehavior : 2;
> >> 
> >> Same problem as before.  MSVC will make the enum signed, and thus you'll have one fewer bit than you think you do. :)  I thought we had helper macros for this?
> > 
> > It is an unsigned in disguise!
> > 
> > typedef unsigned ExpansionBehavior;
> 
> Why not be explicit about it, given the likelihood of confusion?

I tried but unfortunately this would mean changing a lot of files that use this ExpansionBehavior. Let me know if you want a follow-up bug for that.
Cheers,

Rob.

[Raphael Kubo da Costa (:rakuco)]

Build fix for r116260 landed in r116262.

[Rob Buis]

(In reply to comment #9)
> Build fix for r116260 landed in r116262.

Thanks!

[Rob Buis]

Landed in r116260.