1317588 - Clean up definition and macros related to mozilla::Side

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

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

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 1 - Move NS_FOR_CSS_SIDES and operator++ to gfx/2d/Types.h.

Make them live with the definition of enum Side.

Review commit: https://reviewboard.mozilla.org/r/94322/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94322/

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 2 - Remove mozilla::css::Side typedef.

This patch is written with the help of the following script.

function rename() {
find .\
     -type f\
     ! -path "./obj*"\
     ! -path "./.git"\
     ! -path "./.hg"\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -e "s/$1/$2/g" "{}" \;
}

rename "css::Side" "Side"

Review commit: https://reviewboard.mozilla.org/r/94324/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94324/

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

Per bug 1299379 comment 10, a limit value was not added to Side due to
compiler warnings. However, the number of switch-cases isn't too many to
fix. Hence this patch.

I don't bother #define NS_SIDE_COUNT for eSideCount to make those
switch-cases look better since all the NS_SIDE_* will be removed in next
patch.

Review commit: https://reviewboard.mozilla.org/r/94326/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94326/

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT.

This patch was written with the help of the following script. Also, manually
add mozilla qualifier to the enum values in nsStyleCoord.h and gfxRect.h to
make it build.

function rename() {
find .\
     -type f\
     ! -path "./obj*"\
     ! -path "./.git"\
     ! -path "./.hg"\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -e "s/$1/$2/g" "{}" \;
}

rename "NS_SIDE_TOP" "eSideTop"
rename "NS_SIDE_RIGHT" "eSideRight"
rename "NS_SIDE_BOTTOM" "eSideBottom"
rename "NS_SIDE_LEFT" "eSideLeft"

Review commit: https://reviewboard.mozilla.org/r/94328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94328/

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

The postfix operator++ was actually incorrectly implemented as a prefix
version. Since it's only used in NS_FOR_CSS_SIDES, let's changed it to
prefix version and gain some performance improvement.

Review commit: https://reviewboard.mozilla.org/r/94330/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94330/

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Attachment: Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

Those bits are already defined as enum in gfx/2d/Types.h.

Review commit: https://reviewboard.mozilla.org/r/94332/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/94332/

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

The patchset for cleaning mozilla::Side up seems big enough. I'll do mozilla::css::Corner in another bug.

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812659 [details]
Bug 1317588 Part 1 - Move NS_FOR_CSS_SIDES and operator++ to gfx/2d/Types.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94322/diff/1-2/

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812660 [details]
Bug 1317588 Part 2 - Remove mozilla::css::Side typedef.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94324/diff/1-2/

Comment 10

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812661 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94326/diff/1-2/

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812662 [details]
Bug 1317588 Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94328/diff/1-2/

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812663 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94330/diff/1-2/

Comment 13

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812664 [details]
Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94332/diff/1-2/

Comment 14

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812664 [details]
Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94332/diff/2-3/

Comment 15

[Mats Palmgren (inactive)]

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.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

(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 17

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812659 [details]
Bug 1317588 Part 1 - Move NS_FOR_CSS_SIDES and operator++ to gfx/2d/Types.h.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94322/diff/2-3/

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812660 [details]
Bug 1317588 Part 2 - Remove mozilla::css::Side typedef.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94324/diff/2-3/

Comment 19

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812662 [details]
Bug 1317588 Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94328/diff/2-3/

Comment 20

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812664 [details]
Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94332/diff/3-4/

Comment 21

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8812661 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94326/diff/2-3/

Comment 22

[Mats Palmgren (inactive)]

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

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8812660 [details]
Bug 1317588 Part 2 - Remove mozilla::css::Side typedef.

https://reviewboard.mozilla.org/r/94324/#review95424

Comment 24

[Mats Palmgren (inactive)]

Comment on attachment 8812662 [details]
Bug 1317588 Part 3 - Remove #define NS_SIDE_TOP/RIGHT/BOTTOM/LEFT.

https://reviewboard.mozilla.org/r/94328/#review95426

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 8812664 [details]
Bug 1317588 Part 4 - Remove side bits defined in BorderConsts.h

https://reviewboard.mozilla.org/r/94332/#review95428

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8812661 [details]
Bug 1317588 Part 5 - Change Side's operator++ from postfix to prefix.

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
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

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aceab4673889
https://hg.mozilla.org/mozilla-central/rev/60506dc6f798
https://hg.mozilla.org/mozilla-central/rev/1710e2f930db
https://hg.mozilla.org/mozilla-central/rev/88f2c7f0a24a
https://hg.mozilla.org/mozilla-central/rev/0c03c2ddda3a