Bug 61495 – Make RenderStyle visuallyOrdered property use an enum instead of a bool.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Make RenderStyle visuallyOrdered property use an enum instead of a bool.

[Luke Macpherson]

Created attachment 94909 [details]
Patch

[WebKit Review Bot]

Attachment 94909 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:202:  _visuallyOrdered is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Luke Macpherson]

The style issue is pre-existing.

[Eric Seidel (no email)]

Comment on attachment 94909 [details]
Patch

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

> Source/WebCore/css/CSSPrimitiveValueMappings.h:2592
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return LogicalOrder;

Don't add a default: so that teh compiler will complain if you forget an enum.  You can always do the "default" behavior after the switch.

> Source/WebCore/rendering/style/RenderStyleConstants.h:431
> +enum ERtlOrdering { LogicalOrder = 0, VisualOrder };

RTL is generally all caps.  So this would be ERTLOrdering.  But I would just drop the E personally. :)  Does this only apply to RTL?  I guess for RTL logical and visual ordering are the same so we don't care?

[Luke Macpherson]

Comment on attachment 94909 [details]
Patch

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

>> Source/WebCore/rendering/style/RenderStyleConstants.h:431
>> +enum ERtlOrdering { LogicalOrder = 0, VisualOrder };
> 
> RTL is generally all caps.  So this would be ERTLOrdering.  But I would just drop the E personally. :)  Does this only apply to RTL?  I guess for RTL logical and visual ordering are the same so we don't care?

This is the set of values that can be used for the webkit-rtl-ordering property. But if you want me to drop the RTL bit I don't mind.

[Luke Macpherson]

Comment on attachment 94909 [details]
Patch

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

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:2592
>> +        return LogicalOrder;
> 
> Don't add a default: so that teh compiler will complain if you forget an enum.  You can always do the "default" behavior after the switch.

You need it here - m_value.ident has hundreds of possible values - it is the superset of all css values.

[Luke Macpherson]

Created attachment 95088 [details]
Patch

[WebKit Review Bot]

Attachment 95088 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:202:  _visuallyOrdered is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 95088 [details]
Patch

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.

[Luke Macpherson]

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

[Luke Macpherson]

Created attachment 96192 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 96192 [details]
Patch

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

Now I have mixed feelings about this again.  I'm amazed that 4 places in our code have diffent names for hte same bool!

Anyway, r=me.  If you are interested in this area of code, obviously someone might want to make all 4 use the same enum/bool/whatever.

> Source/WebCore/css/CSSStyleSelector.cpp:1226
> +    documentStyle->setRTLOrdering(document->visuallyOrdered() ? VisualOrder : LogicalOrder);

Two ways to say the same thing.... Sigh.

> Source/WebCore/rendering/RenderBlock.cpp:6311
> +    bool directionalOverride = style->rtlOrdering() == VisualOrder;

wow.  I love how we have a 3rd confusing way to say the same thing here.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:964
> +            VisualDirectionOverride override = (style()->rtlOrdering() == VisualOrder ? (style()->direction() == LTR ? VisualLeftToRightOverride : VisualRightToLeftOverride) : NoVisualOverride);

What!?  We have a 4th way to express the same concept?

[Eric Seidel (no email)]

I would let mitz have a chance to comment before we mark this cq+.  It's possible he may think this only makes the problem of this bool confusion worse.

[Eric Seidel (no email)]

(In reply to comment #12)
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:964
> > +            VisualDirectionOverride override = (style()->rtlOrdering() == VisualOrder ? (style()->direction() == LTR ? VisualLeftToRightOverride : VisualRightToLeftOverride) : NoVisualOverride);
> 
> What!?  We have a 4th way to express the same concept?

I guess this 4th one is a ternary not just a bool... but I expect CSS has the same thing.  auto, LTR or RTL.  I'm not sure what VisualLeftToRightOverride really means.

[Luke Macpherson]

(In reply to comment #14)
> (In reply to comment #12)
> > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:964
> > > +            VisualDirectionOverride override = (style()->rtlOrdering() == VisualOrder ? (style()->direction() == LTR ? VisualLeftToRightOverride : VisualRightToLeftOverride) : NoVisualOverride);
> > 
> > What!?  We have a 4th way to express the same concept?
> 
> I guess this 4th one is a ternary not just a bool... but I expect CSS has the same thing.  auto, LTR or RTL.  I'm not sure what VisualLeftToRightOverride really means.

(In reply to comment #13)
> I would let mitz have a chance to comment before we mark this cq+.  It's possible he may think this only makes the problem of this bool confusion worse.

Is 4 days enough time?

[WebKit Review Bot]

Comment on attachment 96192 [details]
Patch

Clearing flags on attachment: 96192

Committed r88526: <http://trac.webkit.org/changeset/88526>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.