[Webkit-unassigned] [Bug 61495] Make RenderStyle visuallyOrdered property use an enum instead of a bool.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61495





--- Comment #10 from Luke Macpherson <macpherson at chromium.org>  2011-06-06 17:46:49 PST ---
(In reply to comment #9)
> (From update of attachment 95088 [details])
> 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. :)

Ok, I'll update that and get back to you shortly.

> 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.

Pragmatically it is necessary because you can overload the cast to an enum in a sensible way, but you can't do that with a boolean because a CSSPrimitiveValue could represent a number of different boolean types.  So I need an enum simply to allow the cast to be overloaded sensibly.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.