Bug 187204 – Refactor InlineTextBox::emphasisMarkExistsAndIsAbove()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[David Kilzer (:ddkilzer)]

In Bug 186968 Comment #14, Said About-Hallawa wrote:

Comment on attachment 343545 [details]
Patch v3

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

> Source/WebCore/rendering/InlineTextBox.cpp:361
> -bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, bool& above) const
> +bool InlineTextBox::emphasisMarkExistsAndIsAbove(const RenderStyle& style, std::optional<bool>& above) const


Here is a simpler approach. I think this function can be split into two functions:

bool emphasisMarkExists(const RenderStyle& style)
bool emphasisMarkIsAbove(const RenderStyle& style)

I checked the code and I found all the callers to this function use the boolean 'above' only if emphasisMarkExistsAndIsAbove() returns true. So I do not think there a need to have the two functions combined into one function.

> Source/WebCore/rendering/InlineTextBox.cpp:1008
> +    std::optional<bool> emphasisMarkAbove;
>      bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove);
>      const AtomicString& emphasisMark = hasTextEmphasis ? lineStyle.textEmphasisMarkString() : nullAtom();
>      if (!emphasisMark.isEmpty())
> -        emphasisMarkOffset = emphasisMarkAbove ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);
> +        emphasisMarkOffset = (emphasisMarkAbove && *emphasisMarkAbove) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);


This can be done using two functions like this:

const AtomicString& emphasisMark = emphasisMarkExists(lineStyle) ? lineStyle.textEmphasisMarkString() : nullAtom();
if (!emphasisMark.isEmpty())
    emphasisMarkOffset = emphasisMarkIsAbove(lineStyle) ? -font.fontMetrics().ascent() - font.emphasisMarkDescent(emphasisMark) : font.fontMetrics().descent() + font.emphasisMarkAscent(emphasisMark);

[David Kilzer (:ddkilzer)]

After giving it a try, this makes the code much more readable:

-    bool emphasisMarkExistsAndIsAbove(const RenderStyle&, std::optional<bool>& isAbove) const;
+    bool emphasisMarkExists(const RenderStyle&) const;
+    bool emphasisMarkIsAbove(const RenderStyle&) const;

I also tried doing this:

-    bool emphasisMarkExistsAndIsAbove(const RenderStyle&) const;
+    std::optional<bool> emphasisMarkExistsAndIsAbove(const RenderStyle&) const;

However, naming the variable that was the result of that method was hard, and the code that called the method read pretty poorly.  It was also more difficult to reason about how the code in the original method should be broken up.

[David Kilzer (:ddkilzer)]

Created attachment 343991 [details]
Patch v1

[David Kilzer (:ddkilzer)]

Comment on attachment 343991 [details]
Patch v1

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

> Source/WebCore/rendering/InlineFlowBox.cpp:1577
> +            auto& textBox = downcast<InlineTextBox>(*child);
>              const RenderStyle& childLineStyle = child->lineStyle();

If you want me to change the second line to something like this, let me know:

            auto& childLineStyle = textBox->lineStyle();

[EWS Watchlist]

Comment on attachment 343991 [details]
Patch v1

Attachment 343991 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8393577

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344008 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[David Kilzer (:ddkilzer)]

Comment on attachment 343991 [details]
Patch v1

Windows crash is unrelated.

[Said Abou-Hallawa]

Comment on attachment 343991 [details]
Patch v1

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

> Source/WebCore/rendering/InlineTextBox.cpp:390
> +    ASSERT(emphasisMarkExists(style));

Since we are calling emphasisMarkExists() in the ASSERT statement and since it has the same assertions below 

ASSERT(!((emphasisPosition & TextEmphasisPosition::Over) && (emphasisPosition & TextEmphasisPosition::Under)));
ASSERT(!((emphasisPosition & TextEmphasisPosition::Left) && (emphasisPosition & TextEmphasisPosition::Right)));

I would suggest removing them from here and have them only in emphasisMarkExists().

> Source/WebCore/rendering/InlineTextBox.cpp:392
> +    auto emphasisPosition = style.textEmphasisPosition();

I think it will help if you make these definitions:

    static const OptionSet<TextEmphasisPosition> emphasisPositionHorizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right };
    auto emphasisPositionHorizontalValue = emphasisPosition & emphasisPositionHorizontalMask

> Source/WebCore/rendering/InlineTextBox.cpp:394
> +    ASSERT(!((emphasisPosition & TextEmphasisPosition::Left) && (emphasisPosition & TextEmphasisPosition::Right)));

So this assertion can now be simplified like this:
    ASSERT(emphasisPositionHorizontalValue != emphasisPositionHorizontalMask);

> Source/WebCore/rendering/InlineTextBox.cpp:396
> +    if (emphasisPositionHasNeitherLeftNorRight(emphasisPosition))

And this can be:
    if (!emphasisPositionHorizontalValue)

And I think you can get rid of emphasisPositionHasNeitherLeftNorRight() in this case.

> Source/WebCore/rendering/InlineTextBox.cpp:402
> +    return emphasisPosition.contains(TextEmphasisPosition::Right);

And this can be:
    return emphasisPositionHorizontalValue == TextEmphasisPosition::Right;

> Source/WebCore/rendering/InlineTextBox.cpp:1025
> -    bool hasTextEmphasis = emphasisMarkExistsAndIsAbove(lineStyle, emphasisMarkAbove);
> +    bool hasTextEmphasis = emphasisMarkExists(lineStyle);

Maybe you can get rid of the local variable hasTextEmphasis since it is referenced only once.

[David Kilzer (:ddkilzer)]

Comment on attachment 343991 [details]
Patch v1

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

>> Source/WebCore/rendering/InlineFlowBox.cpp:1577
>>              const RenderStyle& childLineStyle = child->lineStyle();
> 
> If you want me to change the second line to something like this, let me know:
> 
>             auto& childLineStyle = textBox->lineStyle();

Will change this here...

> Source/WebCore/rendering/InlineFlowBox.cpp:1625
>              const RenderStyle& childLineStyle = child->lineStyle();

...and here.

[David Kilzer (:ddkilzer)]

Created attachment 344024 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 344026 [details]
Patch v3

[zalan]

Comment on attachment 344026 [details]
Patch v3

The change makes the code more error prone since now we can (accidentally) call emphasisMarkIsAbove() even when there's no emphasis. it asserts in debug, but in release the result looks undefined to me. Currently all the call sites ensure that this won't ever happen, but nothing prevents me from adding a new call to emphasisMarkIsAbove() without checking emphasisMarkExists() first (and unless we already have a regression test case that would exercise this new codepath with the condition of not having emphasis mark, the code would just be landed and produce undefined behavior in release environment).
If this cleanup is believed to be an improvement, I think we should at least add emphasisMarkExists() check in emphasisMarkIsAbove() and turn the return bool into a 3state (maybe optional).

[EWS Watchlist]

Comment on attachment 344026 [details]
Patch v3

Attachment 344026 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8399569

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
http/tests/security/canvas-remote-read-remote-video-redirect.html
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344036 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #1)
> After giving it a try, this makes the code much more readable:
> 
> -    bool emphasisMarkExistsAndIsAbove(const RenderStyle&,
> std::optional<bool>& isAbove) const;
> +    bool emphasisMarkExists(const RenderStyle&) const;
> +    bool emphasisMarkIsAbove(const RenderStyle&) const;
> 
> I also tried doing this:
> 
> -    bool emphasisMarkExistsAndIsAbove(const RenderStyle&) const;
> +    std::optional<bool> emphasisMarkExistsAndIsAbove(const RenderStyle&)
> const;
> 
> However, naming the variable that was the result of that method was hard,
> and the code that called the method read pretty poorly.  It was also more
> difficult to reason about how the code in the original method should be
> broken up.

After Zalan's Comment #11, I went back and retried the second suggestion above.

This time, it made much more sense than splitting the function into two, and I still think the code is more readable than before.

[David Kilzer (:ddkilzer)]

Created attachment 344047 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Comment on attachment 344047 [details]
Patch v4

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

> Source/WebCore/rendering/InlineTextBox.cpp:369
> +    std::optional<bool> isAbove;

This can probably just be a `bool` since it's always set in the code path below.  Opinions?

[Darin Adler]

Comment on attachment 344047 [details]
Patch v4

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

> Source/WebCore/ChangeLog:15
> +        Refactor emphasisMarkExistsAndIsAbove() to return a
> +        std::optional<bool> instead of returning a bool and taking a
> +        std::optional<bool> argument.  The state returned is now:
> +        - std::nullopt => emphasis mark doesn't exist or is suppressed.
> +        - false => emphasis mark exists and is not suppressed, but is not above.
> +        - true => emphasis mark exists and is not suppressed, and is above.

Great economy in expressing only the three states we need to here. I’m not totally on board with representing these two booleans as std::optional<bool>. The idea that the optional is what expresses "exists and is not suppressed" and the bool is what expresses "is above or not" is a pretty subtle one. I think a good solution would be to use an enum for above/below and have this be an optional enum instead of an optional boolean. I suspect that if we did that the function might even be able to just be named emphasisMark, and not have the "exists" and "is above" in the name.

But the enum would be slightly awkward when trying to compare with an isFlippedLinesWritingMode boolean so it wouldn’t be a lot better.

> Source/WebCore/rendering/InlineTextBox.cpp:362
> +    static NeverDestroyed<OptionSet<TextEmphasisPosition>> emphasisPositionHorizontalMask(std::initializer_list<TextEmphasisPosition>({ TextEmphasisPosition::Left, TextEmphasisPosition::Right }));

This is just a constant integer value. There is no need to store it in a static. Even if we did store it in a static, there is no need to use NeverDestroyed, since there is no non-trivial destructor. And the explicit std::initializer_list here is particularly unfortunate. I am not sure why it’s needed, but probably due to the unnecessary NeverDestroyed. We should just write this:

    OptionSet<TextEmphasisPosition> horizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right };

Or maybe add a "const" if we think that will help clarify or even possibly generate slightly better code.

I think the shorter name I am suggesting here is OK given how short this function is. No need to keep saying "emphasis" over and over again in the names of local variables. And maybe no need to say position either. But tastes may differ and you might want a longer name. I think we should take the word "emphasis" out of all the names of the local variables in this function.

>> Source/WebCore/rendering/InlineTextBox.cpp:369
>> +    std::optional<bool> isAbove;
> 
> This can probably just be a `bool` since it's always set in the code path below.  Opinions?

Yes, I agree.

[David Kilzer (:ddkilzer)]

Comment on attachment 344047 [details]
Patch v4

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

>> Source/WebCore/ChangeLog:15
>> +        - true => emphasis mark exists and is not suppressed, and is above.
> 
> Great economy in expressing only the three states we need to here. I’m not totally on board with representing these two booleans as std::optional<bool>. The idea that the optional is what expresses "exists and is not suppressed" and the bool is what expresses "is above or not" is a pretty subtle one. I think a good solution would be to use an enum for above/below and have this be an optional enum instead of an optional boolean. I suspect that if we did that the function might even be able to just be named emphasisMark, and not have the "exists" and "is above" in the name.
> 
> But the enum would be slightly awkward when trying to compare with an isFlippedLinesWritingMode boolean so it wouldn’t be a lot better.

I'm going to keep the std::optional<bool> for this patch since it makes the comparison with isFlippedLinesWritingMode easier.

>> Source/WebCore/rendering/InlineTextBox.cpp:362
>> +    static NeverDestroyed<OptionSet<TextEmphasisPosition>> emphasisPositionHorizontalMask(std::initializer_list<TextEmphasisPosition>({ TextEmphasisPosition::Left, TextEmphasisPosition::Right }));
> 
> This is just a constant integer value. There is no need to store it in a static. Even if we did store it in a static, there is no need to use NeverDestroyed, since there is no non-trivial destructor. And the explicit std::initializer_list here is particularly unfortunate. I am not sure why it’s needed, but probably due to the unnecessary NeverDestroyed. We should just write this:
> 
>     OptionSet<TextEmphasisPosition> horizontalMask { TextEmphasisPosition::Left, TextEmphasisPosition::Right };
> 
> Or maybe add a "const" if we think that will help clarify or even possibly generate slightly better code.
> 
> I think the shorter name I am suggesting here is OK given how short this function is. No need to keep saying "emphasis" over and over again in the names of local variables. And maybe no need to say position either. But tastes may differ and you might want a longer name. I think we should take the word "emphasis" out of all the names of the local variables in this function.

Will fix before landing.

>>> Source/WebCore/rendering/InlineTextBox.cpp:369
>>> +    std::optional<bool> isAbove;
>> 
>> This can probably just be a `bool` since it's always set in the code path below.  Opinions?
> 
> Yes, I agree.

Will fix before landing.

[David Kilzer (:ddkilzer)]

Committed r233441: <https://trac.webkit.org/changeset/233441>

[Radar WebKit Bug Importer]

<rdar://problem/41744398>