[webkit-reviews] review granted: [Bug 187204] Refactor InlineTextBox::emphasisMarkExistsAndIsAbove() : [Attachment 344047] Patch v4

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 187204: Refactor InlineTextBox::emphasisMarkExistsAndIsAbove()
https://bugs.webkit.org/show_bug.cgi?id=187204

Attachment 344047: Patch v4

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




--- Comment #17 from Darin Adler <darin at apple.com> ---
Comment on attachment 344047
  --> https://bugs.webkit.org/attachment.cgi?id=344047
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.