[Webkit-unassigned] [Bug 187204] New: Simplify InlineTextBox::emphasisMarkExistsAndIsAbove() into two methods

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=187204

            Bug ID: 187204
           Summary: Simplify InlineTextBox::emphasisMarkExistsAndIsAbove()
                    into two methods
           Product: WebKit
           Version: WebKit Nightly Build
          Hardware: Unspecified
                OS: Unspecified
            Status: NEW
          Severity: Normal
          Priority: P2
         Component: Layout and Rendering
          Assignee: webkit-unassigned at lists.webkit.org
          Reporter: ddkilzer at webkit.org
                CC: bfulgham at webkit.org, sabouhallawa at apple.com,
                    simon.fraser at apple.com, zalan at apple.com

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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180629/5e54cbff/attachment-0001.html>