189077 - Make nsImageFrame smaller

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsImageFrame has 6 PRPackedBool member variables. We can save a little space by
concerting them into a single variable and using its bits as truth values instead.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If you're going to use bits, why not use the 12 high frame state bits (of
mState) that are already there, reserved for implementations of nsIFrame?  See
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/public/nsIFrame.h&rev=3.213&mark=226-228#216

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Use mState

I have some concerns about this, and I have not yet fully investigated what is
happening. Maybe you who have more experience with layout can answer these...

1. nsImageFrame needs to set one bit in the constructor. But it was not
previously doing anything with mState in the constructor, so somebody else does
that. How can I ensure that the bit I set remains? I tested setting the value
of mState to my bit, and that apparently worked but I don't know if there are
subtle errors. If I just tried to |= my bit, mozilla.org did not lay out
correctly and I got assertions.

2. Is my usage of the high bits safe (I am not stomping on anyone's bits and
nobody is stomping on my bits)?

Comment 4

[rbs]

> If I just tried to |= my bit, mozilla.org did not lay out
> correctly and I got assertions.

Weird. Because frames are created with a zeroing |new| and so using "=" and "|="
right at initialization should be the same (unelss frames don't use a zeroing
|new| anymore?).

> How can I ensure that the bit I set remains?
[...]
> 2. Is my usage of the high bits safe (I am not stomping on anyone's bits and
> nobody is stomping on my bits)?

Both questions are the same. If you are inheriting from somebody or somebody is
inheriting from you, then the contract is that you honour the bits owned by
others below you. So, repeating, this means that the inheritance chain is
therefore safe.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Final (?) fix

It seems the broken layout was a fluke, I have not been able to reproduce. This
patch seems to work, and I am just flipping a bit in mState and not stomping
the whole value in the constructor.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 112424 [details] [diff] [review]
Final (?) fix

sr=roc+moz
looks good

You could get Paper (aaronm) to review this if dbaron's too busy.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 112424 [details] [diff] [review]
Final (?) fix

sr=roc+moz
looks good

Comment 8

[ArronM (:paper)]

Comment on attachment 112424 [details] [diff] [review]
Final (?) fix

nothing but smaller goodness here :)

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 112424 [details] [diff] [review]
Final (?) fix

Made nsImageFrame use the frame state bits for its state, which allowed me to
get rid of members that were needlessly bloating the class.

I have run the precheckin tests and have had this change in my tree for a while
and I haven't noticed any problems.

Risks: typo nobody has noticed, or somebody else uses these exact bits in the
frame state that I din't notice. I think the risks are very low.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 112424 [details] [diff] [review]
Final (?) fix

Please hold this cleanup until we open for 1.4alpha. Thanks.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed.

Comment 12

[Boris Zbarsky [:bzbarsky]]

eep. See bug 83774 (nsImageFrame is down to _3_ PRPackedBools there.....)

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

But now we have zero (0) PRPackedBools, so this is smaller. Or am I missing
something?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Hm.... it is indeed 4 bytes smaller.

I'd missed the fact that you moved all that stuff into the frame state instead
of adding another bitfield.  OK.