[webkit-reviews] review denied: [Bug 61495] Make RenderStyle visuallyOrdered property use an enum instead of a bool. : [Attachment 95088] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Luke Macpherson
<macpherson at chromium.org>'s request for review:
Bug 61495: Make RenderStyle visuallyOrdered property use an enum instead of a
bool.
https://bugs.webkit.org/show_bug.cgi?id=61495

Attachment 95088: Patch
https://bugs.webkit.org/attachment.cgi?id=95088&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95088&action=review

> Source/WebCore/rendering/style/RenderStyle.h:362
> +    Order visuallyOrdered() const { return
static_cast<Order>(inherited_flags._visuallyOrdered); }
> +    void setVisuallyOrdered(Order o) { inherited_flags._visuallyOrdered = o;
}

The naming gets confused here.	The current names (mostly) look like they
take/return bools.  Technically it should be isVisuallyOrdered(), but some old
parts of the code don't follow the "is" style.	If we're changing thsi to an
enum, it makes better sense to name it something like rtlOrdering().  Which I
would argue is still better than the old code because it's not immediately
clear that the opposite of visual ordering is logical ordering.

If it's not too much work, I would rename this to rtlOrdering and
setRTLOrdering and we can be done. :)

As is, I'm not sure the bool -> enum conversion is worth it in this case, as it
doesnt' really add clarity.  The clarity comes when replacing callsites which
pass "true" or "false" to functions which take more than one argument.	In this
case, isFoo and setFoo are pretty clear when passed true/false.