Closed Bug 1317588 Opened 3 years ago Closed 3 years ago

Clean up definition and macros related to mozilla::Side

Categories

(Core :: Layout, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

I notice that our code handling Corner and Side are scattering across multiple files, which is hard to understand.  Some (incomplete) observations. 

1. mozilla::css::Corner and NS_FOR_CSS_CORNERS are in gfx/thebes/gfxRect.h [1], and yet two similar macros NS_FOR_CSS_FULL_CORNERS and NS_FOR_CSS_HALF_CORNERS are in layout/style/nsStyleConsts.h [2]. Also, RectCorner which is similar to mozilla::css::Corner is in gfx/2d/Types.h [3]. 
2. mozilla::Side is in gfx/2d/Types.h [4], and NS_FOR_CSS_SIDES is in layout/style/nsStyleConsts.h [5].

I'm thinking about:
1. Move all the above definitions and macros to live in one place such as gfx/2d/Types.h.
2. Use enum to define half corners such as NS_CORNER_TOP_LEFT_X.
3. Refine macros such as NS_HALF_TO_FULL_CORNER [6] to be type safe by using constexpr functions.
4. Unify mozilla::css::Corner and RectCorner.
5. Move Corner and Side to namespace mozilla instead of mozilla::css. I notice bug 1028460 move Side from namespace mozilla::css to mozilla, but then reintroduce it to mozilla::css in [7]. I'm not sure if whether we need the extra mozilla::css namespace. Perhaps mozilla is sufficient.
6. Move all the validation code of the macros [8] to gtest to reduce libxul size.

Any feedback or suggestion is welcome if there's something else we should do or something above that doesn't make sense.

[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/thebes/gfxRect.h#32
[2] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/style/nsStyleConsts.h#41,53
[3] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/2d/Types.h#374
[4] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/gfx/2d/Types.h#386-405
[5] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/style/nsStyleConsts.h#20,27
[6] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/style/nsStyleConsts.h#59-67
[7] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/style/nsStyleConsts.h#20
[8] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/layout/style/nsStyleCoord.cpp#331-421
Priority: -- → P3
The patchset for cleaning mozilla::Side up seems big enough. I'll do mozilla::css::Corner in another bug.
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Summary: Clean up definition and macros related to mozilla::css::Corner and mozilla::Side → Clean up definition and macros related to mozilla::Side
Attachment #8812663 - Attachment is obsolete: true
Attachment #8812663 - Flags: review?(mats)
Comment on attachment 8812661 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

https://reviewboard.mozilla.org/r/94326/#review95032

I'd rather not introduce the eSideCount value and force everyone to handle
that value in their switch-statements.  I agree the macro is a bit ugly,
but I prefer that over the visible ugliness of having to handle eSideCount
(which should never occur) in the switches to avoid a compile warning.
Attachment #8812661 - Flags: review?(mats) → review-
(In reply to Mats Palmgren (:mats) from comment #15)
> I'd rather not introduce the eSideCount value and force everyone to handle
> that value in their switch-statements.  I agree the macro is a bit ugly,
> but I prefer that over the visible ugliness of having to handle eSideCount
> (which should never occur) in the switches to avoid a compile warning.

That makes sense. I'll rebase with the patch introducing eSideCount removed. Please review other parts.
Comment on attachment 8812659 [details]
Bug 1317588 Part 1 - Move NS_FOR_CSS_SIDES and operator++ to gfx/2d/Types.h.

https://reviewboard.mozilla.org/r/94322/#review95422
Attachment #8812659 - Flags: review?(mats) → review+
Comment on attachment 8812660 [details]
Bug 1317588 Part 2 - Remove mozilla::css::Side typedef.

https://reviewboard.mozilla.org/r/94324/#review95424
Attachment #8812660 - Flags: review?(mats) → review+
Comment on attachment 8812662 [details]
Bug 1317588 Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT.

https://reviewboard.mozilla.org/r/94328/#review95426
Attachment #8812662 - Flags: review?(mats) → review+
Comment on attachment 8812664 [details]
Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

https://reviewboard.mozilla.org/r/94332/#review95428
Attachment #8812664 - Flags: review?(mats) → review+
Comment on attachment 8812661 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

https://reviewboard.mozilla.org/r/94326/#review95432
Attachment #8812661 - Flags: review?(mats) → review+
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aceab4673889
Part 1 - Move NS_FOR_CSS_SIDES and operator++ to gfx/2d/Types.h. r=mats
https://hg.mozilla.org/integration/autoland/rev/60506dc6f798
Part 2 - Remove mozilla::css::Side typedef. r=mats
https://hg.mozilla.org/integration/autoland/rev/1710e2f930db
Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT. r=mats
https://hg.mozilla.org/integration/autoland/rev/88f2c7f0a24a
Part 4 - Remove side bits defined in BorderConsts.h r=mats
https://hg.mozilla.org/integration/autoland/rev/0c03c2ddda3a
Part 5 - Change Side's operator++ from postfix to prefix. r=mats
Blocks: 1320014
You need to log in before you can comment on or make changes to this bug.