Make RenderStyle visuallyOrdered property use an enum instead of a bool.
Created attachment 94909 [details] Patch
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.
The style issue is pre-existing.
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?
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.
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.
Created attachment 95088 [details] Patch
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.
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.
(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.
Created attachment 96192 [details] Patch
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?
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.
(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 #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?
Comment on attachment 96192 [details] Patch Clearing flags on attachment: 96192 Committed r88526: <http://trac.webkit.org/changeset/88526>
All reviewed patches have been landed. Closing bug.