Closed Bug 1320014 Opened 3 years ago Closed 3 years ago

Clean up definition and macros related to mozilla::css::Corner

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

(15 files)

59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
59 bytes, text/x-review-board-request
mats
: review+
Details
+++ This bug was initially created as a clone of Bug #1317588 +++

The code related to mozilla::Side was cleaned up in bug 1317588. This bug is for cleaning up Corner.

1. Move all the definitions and macros related to Corner to 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 to be type safe by using constexpr functions.
4. Unify mozilla::css::Corner and RectCorner.
5. Move Corner to namespace mozilla instead of mozilla::css.
Mats, sorry to throw so many review requests at once. I did all the work I can think of in comment 0, and each patch should do exactly one thing. Please let me know if I miss anything related that might need some clean-up. Thanks.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f1e5ff64647fb2ed90aefae598681c96e0a060e
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Comment on attachment 8824048 [details]
Bug 1320014 Part 1 - Move Corner's definition and NS_FOR_CSS_CORNERS to gfx/2d/Types.h.

https://reviewboard.mozilla.org/r/102570/#review102918
Attachment #8824048 - Flags: review?(mats) → review+
Comment on attachment 8824049 [details]
Bug 1320014 Part 2 - Rename Corner to LogicalCorner, and move it to nsStyleCoord.h.

https://reviewboard.mozilla.org/r/102572/#review102926

We define LogicalAxis, LogicalEdge and LogicalSide enums in nsStyleCoord.h.
The LogicalCorner enum seems to belong to that set of definitions so perhaps
we should move it there?  Or maybe next to LogicalSideBits in WritingModes.h?
nsCellMap.h just seems like an odd place to define this. (not your fault
of course)
Attachment #8824049 - Flags: review?(mats) → review+
Comment on attachment 8824050 [details]
Bug 1320014 Part 3 - Move Corner from namespace mozilla::css into mozilla.

https://reviewboard.mozilla.org/r/102574/#review102932
Attachment #8824050 - Flags: review?(mats) → review+
Comment on attachment 8824051 [details]
Bug 1320014 Part 4 - Remove eNumCorners and rewrite NS_FOR_CSS_CORNERS.

https://reviewboard.mozilla.org/r/102576/#review102936
Attachment #8824051 - Flags: review?(mats) → review+
Comment on attachment 8824052 [details]
Bug 1320014 Part 5 - Remove NS_FOR_CSS_CORNERS.

https://reviewboard.mozilla.org/r/102578/#review102940
Attachment #8824052 - Flags: review?(mats) → review+
Comment on attachment 8824053 [details]
Bug 1320014 Part 6 - Remove #define NS_CORNER_[TOP_LEFT/TOP_RIGHT/BOTTOM_RIGHT/BOTTOM_LEFT]

https://reviewboard.mozilla.org/r/102580/#review102942
Attachment #8824053 - Flags: review?(mats) → review+
Comment on attachment 8824054 [details]
Bug 1320014 Part 7 - Convert half corner indices #define to an enum.

https://reviewboard.mozilla.org/r/102582/#review102946
Attachment #8824054 - Flags: review?(mats) → review+
Comment on attachment 8824055 [details]
Bug 1320014 Part 8 - Move NS_FOR_CSS_HALF_CORNERS to Types.h and rewrite it.

https://reviewboard.mozilla.org/r/102584/#review102948
Attachment #8824055 - Flags: review?(mats) → review+
Comment on attachment 8824056 [details]
Bug 1320014 Part 9 - Convert NS_HALF_CORNER_IS_X to a constexpr function.

https://reviewboard.mozilla.org/r/102586/#review102954
Attachment #8824056 - Flags: review?(mats) → review+
Comment on attachment 8824057 [details]
Bug 1320014 Part 10 - Convert NS_HALF_TO_FULL_CORNER to a constexpr function.

https://reviewboard.mozilla.org/r/102588/#review102958

Please update the comment in the last chunk.
Attachment #8824057 - Flags: review?(mats) → review+
Comment on attachment 8824058 [details]
Bug 1320014 Part 11 - Convert NS_FULL_TO_HALF_CORNER to a constexpr function.

https://reviewboard.mozilla.org/r/102590/#review102962

Please update the comment in the last chunk.
Attachment #8824058 - Flags: review?(mats) → review+
Comment on attachment 8824059 [details]
Bug 1320014 Part 12 - Convert NS_SIDE_IS_VERTICAL to a constexpr function.

https://reviewboard.mozilla.org/r/102592/#review102966
Attachment #8824059 - Flags: review?(mats) → review+
Comment on attachment 8824060 [details]
Bug 1320014 Part 13 - Convert NS_SIDE_TO_FULL_CORNER to a constexpr function.

https://reviewboard.mozilla.org/r/102594/#review102968

Please update the comment in the last chunk.

The "aIsSecond" param is a bit obscure.  Perhaps we should document it with an example:
@param aIsSecond when true, return the clockwise second of the two corners associated with aSide, for example with aSide = eSideBottom the result is eCornerBottomRight when aIsSecond is false and eCornerBottomLeft aIsSecond is true.
Attachment #8824060 - Flags: review?(mats) → review+
Comment on attachment 8824061 [details]
Bug 1320014 Part 14 - Convert NS_SIDE_TO_HALF_CORNER to a constexpr function.

https://reviewboard.mozilla.org/r/102596/#review102970

Ditto for aIsSecond.  I guess you can just use a "@param aIsSecond see above" here.

aIsParallel is a bit obscure too.  Perhaps document it with something like:
@param aIsParallel return the half-corner that is parallel with aSide when aIsParallel is true, for example with aSide=eSideTop, aIsSecond=true the result is eCornerTopRightX when aIsParallel is true and eCornerTopRightY when aIsParallel is false (because "X" is parallel with eSideTop (and eSideBottom), similarly "Y" is parallel with eSideLeft/eSideRight)

(And please double-check my logic above in case I've misunderstood...)
Attachment #8824061 - Flags: review?(mats) → review+
Comment on attachment 8824062 [details]
Bug 1320014 Part 15 - Replace RectCorner with Corner.

https://reviewboard.mozilla.org/r/102598/#review102998

::: gfx/2d/PathHelpers.h:228
(Diff revision 1)
>      }
>      return true;
>    }
>  
>    bool AreRadiiSame() const {
> -    for (size_t i = 1; i < RectCorner::Count; i++) {
> +    NS_FOR_CSS_FULL_CORNERS(i) {

I guess you're aware that this is a change although checking that radii[0] == radii[0] shouldn't affect the result.  Hopefully the compiler can optimize that check away.  But, perhaps this method would be clearer as:

  return TopRight()    == TopLeft() &&
         BottomRight() == TopLeft() &&
         BottomLeft()  == TopLeft();

I think I'd slightly prefer that, fwiw, but the loop is fine too if you want to keep it.   (Ditto for operator==)
Attachment #8824062 - Flags: review?(mats) → review+
Comment on attachment 8824049 [details]
Bug 1320014 Part 2 - Rename Corner to LogicalCorner, and move it to nsStyleCoord.h.

https://reviewboard.mozilla.org/r/102572/#review102926

> We define LogicalAxis, LogicalEdge and LogicalSide enums in nsStyleCoord.h.
> The LogicalCorner enum seems to belong to that set of definitions so perhaps
> we should move it there? 

Sounds good. I'll move LogicalCorner to nsStyleCoord.h. Also, I'll add eLogicalCorner prefix to all the enum values to match the coding standard.
Comment on attachment 8824061 [details]
Bug 1320014 Part 14 - Convert NS_SIDE_TO_HALF_CORNER to a constexpr function.

https://reviewboard.mozilla.org/r/102596/#review102970

> (And please double-check my logic above in case I've misunderstood...)

Your logic and the exmaple look good to me. I'll add them in my next patchset. Also, I'll add a note that those `static_assert` checks in nsStyleCoord.cpp could also serve as usage exmaples.
Comment on attachment 8824062 [details]
Bug 1320014 Part 15 - Replace RectCorner with Corner.

https://reviewboard.mozilla.org/r/102598/#review102998

> I guess you're aware that this is a change although checking that radii[0] == radii[0] shouldn't affect the result.  Hopefully the compiler can optimize that check away.  But, perhaps this method would be clearer as:
> 
>   return TopRight()    == TopLeft() &&
>          BottomRight() == TopLeft() &&
>          BottomLeft()  == TopLeft();
> 
> I think I'd slightly prefer that, fwiw, but the loop is fine too if you want to keep it.   (Ditto for operator==)

I must admit I didn't aware of this for-loops starting from 1. Thank you for catching this. And I agree it's more readable to explicitly write down all the comparisions. I'll rewrite this as well as operator== in my next patchset.
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2cb81f3fc29e
Part 1 - Move Corner's definition and NS_FOR_CSS_CORNERS to gfx/2d/Types.h. r=mats
https://hg.mozilla.org/integration/autoland/rev/0682966014dd
Part 2 - Rename Corner to LogicalCorner, and move it to nsStyleCoord.h. r=mats
https://hg.mozilla.org/integration/autoland/rev/9a201f9c2374
Part 3 - Move Corner from namespace mozilla::css into mozilla. r=mats
https://hg.mozilla.org/integration/autoland/rev/2177bf76842a
Part 4 - Remove eNumCorners and rewrite NS_FOR_CSS_CORNERS. r=mats
https://hg.mozilla.org/integration/autoland/rev/6b29fba60795
Part 5 - Remove NS_FOR_CSS_CORNERS. r=mats
https://hg.mozilla.org/integration/autoland/rev/b1b0ea869038
Part 6 - Remove #define NS_CORNER_[TOP_LEFT/TOP_RIGHT/BOTTOM_RIGHT/BOTTOM_LEFT] r=mats
https://hg.mozilla.org/integration/autoland/rev/cf81b45536f3
Part 7 - Convert half corner indices #define to an enum. r=mats
https://hg.mozilla.org/integration/autoland/rev/7b303c2f6979
Part 8 - Move NS_FOR_CSS_HALF_CORNERS to Types.h and rewrite it. r=mats
https://hg.mozilla.org/integration/autoland/rev/4dc7949d79de
Part 9 - Convert NS_HALF_CORNER_IS_X to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/83333bd36d76
Part 10 - Convert NS_HALF_TO_FULL_CORNER to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/919cd7ac0dc3
Part 11 - Convert NS_FULL_TO_HALF_CORNER to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/1c3a30388a86
Part 12 - Convert NS_SIDE_IS_VERTICAL to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/3e3ca0d6683a
Part 13 - Convert NS_SIDE_TO_FULL_CORNER to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/af9e85ac33da
Part 14 - Convert NS_SIDE_TO_HALF_CORNER to a constexpr function. r=mats
https://hg.mozilla.org/integration/autoland/rev/d6bf8872b3c3
Part 15 - Replace RectCorner with Corner. r=mats
Depends on: 1387158
You need to log in before you can comment on or make changes to this bug.