Bug 143399 – Code clean up for extracting information from the mix of WritingMode and TextDirection

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Said Abou-Hallawa]

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.

[Said Abou-Hallawa]

Created attachment 250122 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252909 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 252913 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252914 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Said Abou-Hallawa]

Created attachment 252924 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252926 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252927 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252930 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252933 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252966 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 252967 [details]
Patch

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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

[Dave Hyatt]

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.

[Said Abou-Hallawa]

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

[Said Abou-Hallawa]

Created attachment 253855 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

Created attachment 253857 [details]
Patch

[WebKit Commit Bot]

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.

[Said Abou-Hallawa]

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

[WebKit Commit Bot]

Comment on attachment 253857 [details]
Patch

Clearing flags on attachment: 253857

Committed r184962: <http://trac.webkit.org/changeset/184962>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/21150357>