Closed Bug 524025 Opened 10 years ago Closed 7 years ago

Remove LL_ constants from nsLineLayout


(Core :: Layout, defect)

Not set





(Reporter: roc, Assigned: lcamacho)


(Whiteboard: [good first bug])


(1 file, 4 obsolete files)

nsLineLayout should just use a bunch of named bitfields like PRPackedBool mHasBullet:1 instead of these annoying #defined constants.
Whiteboard: [good first bug]
Attached patch Patch (v1) (obsolete) — Splinter Review
Is this old bug request still alive? I'm not sure of the benefit of the change, but the attached patch might help... let me know?
Assignee: nobody → markcapella
Attachment #603838 - Flags: feedback?(roc)
Comment on attachment 603838 [details] [diff] [review]
Patch (v1)

So the idea was not to just move the constants to be static members.  It was to not have constants at all, but instead have member variables that each occupy one bit.  So that ugly code like:
  SetFlag(LL_GOTLINEBOX, true);
can just be written as:
  mGotLineBox = true;
and so that:
can just be written as:
Attachment #603838 - Flags: feedback?(roc) → feedback-
Ah... I just realized this was a Mac OS/X bug. I work in a WIN environment. I could try this as a pure coding exercise, but then have no way to test for embarassing mistakes...  I may have taken up your time needlessly.
Assignee: markcapella → nobody
Attached patch patchV1 (obsolete) — Splinter Review
Attachment #663815 - Flags: review?(dbaron)
Did you try compiling this?  I wouldn't have expected the way you initialize the members within the class declaration to compile.

You also don't need the "last flag" anymore; that was just to show which bits were available.

And you want to indicate fields widths (that is, ": 1") for each of the members where they're declared.  I *think* using field widths with bool is acceptable (though it wasn't with PRBool, since PRBool was signed).
OS: Mac OS X → All
Hardware: x86 → All
Also, I would expect the patch to remove the GetFlag and SetFlag methods and the mFlags member.
Attachment #663815 - Flags: review?(dbaron) → review-
Attached patch patchV2 (obsolete) — Splinter Review
Attachment #663815 - Attachment is obsolete: true
Attachment #664304 - Flags: review?(dbaron)
This mostly looks good, except for two things:

 (1) Instead of making an InitFlags(), could you just initialize them in the constructor?  (Member-initializer syntax would probably be preferable, but assignment is ok too.)

 (2) Could you declare them where mFlags was declared, rather than where the LL_* constants were declared?  (That's where the other member variables are.)
Attached patch patchV3 (obsolete) — Splinter Review
Attachment #664304 - Attachment is obsolete: true
Attachment #664304 - Flags: review?(dbaron)
Attachment #664758 - Flags: review?(dbaron)
Comment on attachment 664758 [details] [diff] [review]

r=dbaron with two corrections:

 (1) move the declarations in nsLineLayout.h down to closer to where mFlags was, but actually slightly above:  put them between mTrimmableWidth and mSpanDepth.  This is for two reasons:
    (a) initialization happens in the order the variables are declared, not the order the initializers are written (in the constructor), but gcc warns when the initializers are written in a different order than the order in which they'll happen.  So the order should match the initializers.
    (b) it will lead to a smaller size structure on 64-bit platforms, since on 64-bit platforms putting these variables between the beginning of the structure and a pointer will be putting them in a space that has to be 64-bits (for alignment) 

 (2) Sorry I didn't notice this before, but it would be better to capitalize "OK" and "BR" slightly differently, as:

One other comment is that the description part of the commit message:
Remove LL_ constants from nsLineLayout
should probably say:
Replace LL_* constants in nsLineLayout with separate booleans with field widths.

If you post a revised patch with those changes, feel free to add the checkin-needed keyword to the bug.
Attachment #664758 - Flags: review?(dbaron) → review+
Attachment #664758 - Attachment is obsolete: true
Assignee: nobody → leonard.camacho
Keywords: checkin-needed
Attachment #603838 - Attachment is obsolete: true
Pushed to Try for a sanity check. I'll land it if it's green.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.