There should be eight TextFlows. A TextFlow consists of two directions. The first one controls the line growing direction and the second one controls the block growing direction. The possible TextFlows are: TextFlowEastSouth TextFlowWestSouth TextFlowEastNorth TextFlowWestNorth TextFlowSouthEast TextFlowSouthWest TextFlowNorthEast TextFlowNorthWest We need to extract the following information easily from the TextFlow -- isTextFlowReversed() // RTL -- isTextFlowFlipped() // the opposite direction to normal -- isTextFlowVertical() // The line grows vertically I came across the function resolveToPhysicalProperty() and I found out it is needlessly complicated and hard to read. I think mixing the WritingMode and the TextDirection into TextFlow and choosing their values carefully to ease implementing the above functions can make things clearer.
Created attachment 250122 [details] Patch
Attachment 250122 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/text/WritingMode.h:41: One space before end of line comments [whitespace/comments] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252909 [details] Patch
Attachment 252909 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 252909 [details] Patch Need to remove LengthBox.cpp from more build files. Just do a grep to see where it’s mentioned.
Created attachment 252913 [details] Patch
Attachment 252913 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252914 [details] Patch
Attachment 252914 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 252914 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252914&action=review > Source/WebCore/platform/LengthBox.h:37 > + : m_sides { type, type, type, type } C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\LengthBox.h(38): error C2536: 'WebCore::LengthBox::WebCore::LengthBox::m_sides' : cannot specify explicit initializer for arrays (C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources\StyleBuilder.cpp) [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore.vcxproj] C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\LengthBox.h(87) : see declaration of 'WebCore::LengthBox::m_sides' Windows compiler doesn’t like this. Maybe std::array will help. > Source/WebCore/platform/LengthBox.h:87 > + Length m_sides[4]; If we used std::array here instead, then operator== above would not have to list all four sides. It could just say return m_sides == other.m_sides instead. > Source/WebCore/platform/text/TextFlags.h:31 > +enum TextDirection { LTR, RTL }; Logical change. But seems slightly dangerous to change this unless we audit every single call site that uses TextDirection. Some of them might be doing if (x). > Source/WebCore/platform/text/WritingMode.h:44 > +#define MAKE_TEXT_FLOW(writingMode, direction) ((writingMode) << 1 | (direction)) Seems like we could use a constexpr function some day, maybe only in the future. I suggest also #undef MAKE_TEXT_FLOW after we are done with it.
Created attachment 252924 [details] Patch
Attachment 252924 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252926 [details] Patch
Attachment 252926 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252927 [details] Patch
Attachment 252927 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252930 [details] Patch
Attachment 252930 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252933 [details] Patch
Attachment 252933 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252966 [details] Patch
Attachment 252966 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 252967 [details] Patch
Attachment 252967 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/TextFlags.h:31: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 252967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252967&action=review > Source/WebCore/platform/text/WritingMode.h:49 > +enum TextFlow { > + TextFlowEastSouth = MAKE_TEXT_FLOW(TopToBottomWritingMode, LTR), enum class would avoid the repetition here: enum class TextFlow { EastSouth, .... But I would prefer something like BlockEastInlineSouth (if that's accurate).
Comment on attachment 252967 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252967&action=review > Source/WebCore/platform/text/WritingMode.h:56 > + TextFlowWestSouth = MAKE_TEXT_FLOW(TopToBottomWritingMode, RTL), > + TextFlowEastNorth = MAKE_TEXT_FLOW(BottomToTopWritingMode, LTR), > + TextFlowWestNorth = MAKE_TEXT_FLOW(BottomToTopWritingMode, RTL), > + TextFlowSouthEast = MAKE_TEXT_FLOW(LeftToRightWritingMode, LTR), > + TextFlowSouthWest = MAKE_TEXT_FLOW(LeftToRightWritingMode, RTL), > + TextFlowNorthEast = MAKE_TEXT_FLOW(RightToLeftWritingMode, LTR), > + TextFlowNorthWest = MAKE_TEXT_FLOW(RightToLeftWritingMode, RTL) I think using cardinal directions is weird. In my opinion, a better convention would be to just use the terminology from Internet Explorer's original writing mode definitions, e.g., tb-rl tb-lr etc.
(In reply to comment #26) > Comment on attachment 252967 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252967&action=review > > > Source/WebCore/platform/text/WritingMode.h:56 > > + TextFlowWestSouth = MAKE_TEXT_FLOW(TopToBottomWritingMode, RTL), > > + TextFlowEastNorth = MAKE_TEXT_FLOW(BottomToTopWritingMode, LTR), > > + TextFlowWestNorth = MAKE_TEXT_FLOW(BottomToTopWritingMode, RTL), > > + TextFlowSouthEast = MAKE_TEXT_FLOW(LeftToRightWritingMode, LTR), > > + TextFlowSouthWest = MAKE_TEXT_FLOW(LeftToRightWritingMode, RTL), > > + TextFlowNorthEast = MAKE_TEXT_FLOW(RightToLeftWritingMode, LTR), > > + TextFlowNorthWest = MAKE_TEXT_FLOW(RightToLeftWritingMode, RTL) > > I think using cardinal directions is weird. In my opinion, a better > convention would be to just use the terminology from Internet Explorer's > original writing mode definitions, e.g., > > tb-rl > tb-lr > > etc. The problem with this naming is the writing_mode is physical direction but the text_direction is logical direction relative to the writing mode. For example Internet Explorer's naming in these cases lr-lr == TextFlowSouthEast lr-rl == TextFlowSouthWest rl-lr == TextFlowNorthEast rl-rl == TextFlowNorthWest do not indicate easily where the text grows. But cardinal directions give you that easily. The first one TextFlowSouthEast says: 1. The line grows from South-to-North 2. The paragraph grows from East-to-West But with "lr-lr": 1. The first "lr" says the paragraph grows from East-to-West 2. For the second "lr", I have to remember that the text is vertical so the left now means the bottom so I know that the line grows from South-to-North.
Created attachment 253855 [details] Patch
Attachment 253855 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/WritingMode.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 15 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 253857 [details] Patch
Attachment 253857 [details] did not pass style-queue: ERROR: Source/WebCore/platform/text/WritingMode.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #10) > Comment on attachment 252914 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=252914&action=review > > > Source/WebCore/platform/LengthBox.h:37 > > + : m_sides { type, type, type, type } > > C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\LengthBox.h(38): > error C2536: 'WebCore::LengthBox::WebCore::LengthBox::m_sides' : cannot > specify explicit initializer for arrays > (C: > \cygwin\home\buildbot\WebKit\WebKitBuild\Release\obj32\WebCore\DerivedSources > \StyleBuilder.cpp) > [C:\cygwin\home\buildbot\WebKit\Source\WebCore\WebCore.vcxproj\WebCore. > vcxproj] > > C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\LengthBox.h(87) : see > declaration of 'WebCore::LengthBox::m_sides' > > Windows compiler doesn’t like this. Maybe std::array will help. > This has been fixed and submitted in http://trac.webkit.org/changeset/184895. > > Source/WebCore/platform/LengthBox.h:87 > > + Length m_sides[4]; > > If we used std::array here instead, then operator== above would not have to > list all four sides. It could just say return m_sides == other.m_sides > instead. > This has been fixed and submitted in http://trac.webkit.org/changeset/184895. > > Source/WebCore/platform/text/TextFlags.h:31 > > +enum TextDirection { LTR, RTL }; > > Logical change. But seems slightly dangerous to change this unless we audit > every single call site that uses TextDirection. Some of them might be doing > if (x). > I checked all the places which use TextDirection and fortunately I did not find condition like "if (textDirection)" or "if (!textDirection)". So changing the order of LTR and RTL should be safe. > > Source/WebCore/platform/text/WritingMode.h:44 > > +#define MAKE_TEXT_FLOW(writingMode, direction) ((writingMode) << 1 | (direction)) > > Seems like we could use a constexpr function some day, maybe only in the > future. I suggest also #undef MAKE_TEXT_FLOW after we are done with it. I tried using constexpr and its seems to work except on Windows. So I gave up and I left the macro and the function as they are but I added the #undef as suggested.
Comment on attachment 253857 [details] Patch Clearing flags on attachment: 253857 Committed r184962: <http://trac.webkit.org/changeset/184962>
All reviewed patches have been landed. Closing bug.
<rdar://problem/21150357>