[webkit-reviews] review granted: [Bug 218197] Clean up BoxSide and BorderEdge code : [Attachment 412334] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 218197: Clean up BoxSide and BorderEdge code
https://bugs.webkit.org/show_bug.cgi?id=218197

Attachment 412334: Patch

https://bugs.webkit.org/attachment.cgi?id=412334&action=review




--- Comment #2 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 412334
  --> https://bugs.webkit.org/attachment.cgi?id=412334
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412334&action=review

> Source/WebCore/platform/RectEdges.h:52
> +    T& at(BoxSide side) { return m_sides[static_cast<size_t>(side)]; }

The more traditional thing would be to use operation[] here.

> Source/WebCore/platform/RectEdges.h:89
> +    std::array<T, 4> m_sides { };

I don't think you need the braces at all here. It should default initialize the
members by default.

> Source/WebCore/platform/text/WritingMode.h:131
> +constexpr std::initializer_list<BoxSide> allBoxSides = { BoxSide::Top,
BoxSide::Right, BoxSide::Bottom, BoxSide::Left };

I think this would make more sense as std::array<BoxSide, 4>. Keeping
initializer_list is mostly best used as a parameter to a function.

I also think we should try to replace this, perhaps as a follow up, with
specific iterators on RectEdges.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1799
> +    static constexpr std::initializer_list<BoxSide> paintOrderSides = {
BoxSide::Top, BoxSide::Bottom, BoxSide::Left, BoxSide::Right };

like with the other use, I think this would be better as a std::array.