Bug 237967 – Clarify code for logical-to-physical mappings, and add physical-to-logical mappings

• Component: CSS
• Status: RESOLVED FIXED

[Oriol Brufau]

The current code for logical-to-physical mappings is not easy to understand.

mapLogicalSideToPhysicalSide relies on casting a LogicalBoxSide enum into a BoxSide enum, and then maybe casting it into an int and back into a BoxSide...
So the code relies on the arbitrary order of the enum values, and it's hard to grasp what it's doing.

mapLogicalCornerToPhysicalCorner just happens to work but again it's quite difficult to follow.
For example, if bug 166941 adds new writing modes, most probably the entire method will need to be changed.

Also, in bug 236199 I will need physical-to-logical mappings, i.e. the inverse of the current functions.
But the way that they are now, writing the inverse code is not straight-forward at all.

So these functions should be refactored to be understandable, and should have inverse functions where it's clear looking at the code that they are really inverse functions.

[Oriol Brufau]

Created attachment 454853 [details]
Patch

[Darin Adler]

Comment on attachment 454853 [details]
Patch

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

Can we make a test suite for these functions using static_assert? It can be in a .cpp file so we don’t run it more than once.

> Source/WebCore/css/makeprop.pl:746
> +    const TextFlow& textflow = makeTextFlow(writingMode, direction);

Can this use auto& or auto?

> Source/WebCore/css/makeprop.pl:781
> +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";

There’s no need to repeat the assertion expression twice. Can just static_assert(expression). Earlier versions of C++ required a string every time, but the latest does not, and there’s no value to repeating the expression in string form.

> Source/WebCore/css/makeprop.pl:785
> +            my $assert =  $logicalToPhysical . "(" . $textFlow . ", " . $physicalToLogical . "(" . $textFlow . ", " . $physicalEnum . ")) == " . $physicalEnum;

Extra space here after "=".

> Source/WebCore/css/makeprop.pl:786
> +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";

Ditto.

> Source/WebCore/platform/text/WritingMode.h:160
>  constexpr inline BoxSide mapLogicalSideToPhysicalSide(TextFlow textflow, LogicalBoxSide logicalSide)

It’s never necessary to write "constexpr inline". Just "constexpr" means the same thing, for a function.

[Oriol Brufau]

(In reply to Darin Adler from comment #2)
> Comment on attachment 454853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=454853&action=review
> 
> Can we make a test suite for these functions using static_assert? It can be
> in a .cpp file so we don’t run it more than once.

I would have written a unit test, but at first I didn't find any existing example.
I guess I have found them in Tools/TestWebKitAPI/Tests/WebCore/, do you mean something like that?

> > Source/WebCore/css/makeprop.pl:781
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> There’s no need to repeat the assertion expression twice. Can just
> static_assert(expression). Earlier versions of C++ required a string every
> time, but the latest does not, and there’s no value to repeating the
> expression in string form.

I did it in order to get a meaningful message if the assert fails.
But it doesn't matter if I write a unit test instead.

[Oriol Brufau]

Created attachment 454923 [details]
Patch

[Oriol Brufau]

(In reply to Darin Adler from comment #2)
> Can we make a test suite for these functions using static_assert? It can be
> in a .cpp file so we don’t run it more than once.

Added Tools/TestWebKitAPI/Tests/WebCore/WritingModeTests.cpp
Not sure if you need to take a look, or if your previous approval still holds.

> > Source/WebCore/css/makeprop.pl:746
> > +    const TextFlow& textflow = makeTextFlow(writingMode, direction);
> 
> Can this use auto& or auto?

Done.

> > Source/WebCore/css/makeprop.pl:781
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> There’s no need to repeat the assertion expression twice. Can just
> static_assert(expression). Earlier versions of C++ required a string every
> time, but the latest does not, and there’s no value to repeating the
> expression in string form.
> 
> > Source/WebCore/css/makeprop.pl:785
> > +            my $assert =  $logicalToPhysical . "(" . $textFlow . ", " . $physicalToLogical . "(" . $textFlow . ", " . $physicalEnum . ")) == " . $physicalEnum;
> 
> Extra space here after "=".
> 
> > Source/WebCore/css/makeprop.pl:786
> > +            print GPERF "static_assert(" . $assert . ", \"" . $assert . "\");\n";
> 
> Ditto.

Removed this code.

> > Source/WebCore/platform/text/WritingMode.h:160
> >  constexpr inline BoxSide mapLogicalSideToPhysicalSide(TextFlow textflow, LogicalBoxSide logicalSide)
> 
> It’s never necessary to write "constexpr inline". Just "constexpr" means the
> same thing, for a function.

Done.

[Darin Adler]

Comment on attachment 454923 [details]
Patch

This is all good. I think at least some of the unit tests could be restructured so they run at compile time rather than runtime since this is largely constexpr code. But that does not have to be done just to land this.

[EWS]

Committed r291407 (248537@main): <https://commits.webkit.org/248537@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 454923 [details].

[Radar WebKit Bug Importer]

<rdar://problem/90431868>