Bug 82767 – Add a compile assert for the size of InlineFlowBox

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Add a compile assert for the size of InlineFlowBox

[Ryosuke Niwa]

Created attachment 134856 [details]
Patch

[Tony Chang]

Comment on attachment 134856 [details]
Patch

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

> Source/WebCore/rendering/InlineFlowBox.cpp:50
> +    uint32_t bitfields : 24;

Can we drop the : 24?  It's always going to round up to the packing size, right?

> Source/WebCore/rendering/InlineFlowBox.h:315
> +    unsigned m_hasAnnotationsBefore : 1;
> +    unsigned m_hasAnnotationsAfter : 1;

Should we add getters and setters for these (I guess to RootInlineBox)?

[Ryosuke Niwa]

Comment on attachment 134856 [details]
Patch

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

>> Source/WebCore/rendering/InlineFlowBox.h:315
>> +    unsigned m_hasAnnotationsAfter : 1;
> 
> Should we add getters and setters for these (I guess to RootInlineBox)?

Maybe in the future. As is, these member variables are only used by RootInlineBox.
It'll be nice if we had some macro or WTF template that enforces such a restriction.

[Ojan Vafai]

Comment on attachment 134856 [details]
Patch

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

>>> Source/WebCore/rendering/InlineFlowBox.h:315
>>> +    unsigned m_hasAnnotationsAfter : 1;
>> 
>> Should we add getters and setters for these (I guess to RootInlineBox)?
> 
> Maybe in the future. As is, these member variables are only used by RootInlineBox.
> It'll be nice if we had some macro or WTF template that enforces such a restriction.

Not adding getters/setters seems like it's asking for bugs down the road. Can you just make these private and manually add protected getters/setters? For something simple like this, I don't think we need complicated macros/templates.

[WebKit Review Bot]

Comment on attachment 134856 [details]
Patch

Clearing flags on attachment: 134856

Committed r112725: <http://trac.webkit.org/changeset/112725>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

(In reply to comment #4)
> Not adding getters/setters seems like it's asking for bugs down the road. Can you just make these private and manually add protected getters/setters? For something simple like this, I don't think we need complicated macros/templates.

I wouldn't think so since these member variables are only used in InlineFlowBox and RootInlineBox but I wouldn't object to making such a change.