Bug 204953 – [LFC][IFC] Paint partial trailing run with hyphen when needed

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

[alan]

ssia

[Radar WebKit Bug Importer]

<rdar://problem/57705169>

[alan]

Created attachment 385028 [details]
Patch

[Antti Koivisto]

Comment on attachment 385028 [details]
Patch

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

> Source/WebCore/layout/displaytree/DisplayRun.h:48
> -        TextContext(unsigned position, unsigned length, const String&, Optional<ExpansionContext> = { });
> +        TextContext(unsigned position, unsigned length, const String&, bool needsHyphen = false, Optional<ExpansionContext> = { });

I can't find where you are using the optional constructor parameters. They seem to be set via the setters. Can you remove them?

> Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:514
> +        for (auto index = inlineContent.runs.size(); index--;) {
> +            auto& displayRun = inlineContent.runs[index];
> +            if (!displayRun.textContext())
> +                continue;
> +            // This has to be a run on this new line.
> +            ASSERT(!previousRunCount || index > previousRunCount - 1);

Is the assert important? This would be nicer with range-for.

> Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:519
> +            break;

Not a fan of for loops that don't loop over the body.

> Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp:227
> -        TextRun textRun { textContext.content(), logicalLeft, horizontalExpansion, behavior };
> +        String textWithHyphen;
> +        if (textContext.needsHyphen())
> +            textWithHyphen = makeString(textContext.content(), style.hyphenString());
> +        TextRun textRun { !textWithHyphen.isEmpty() ? textWithHyphen : textContext.content(), logicalLeft, horizontalExpansion, behavior };

Isn't there are FIXME here you remove?

[Antti Koivisto]

> Is the assert important? This would be nicer with range-for.

that is

for (auto& run : WTF::makeReversedRange(inlineContent.runs))
    ....

[alan]

Committed r253214: <https://trac.webkit.org/changeset/253214>

[alan]

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 385028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385028&action=review
> 
> > Source/WebCore/layout/displaytree/DisplayRun.h:48
> > -        TextContext(unsigned position, unsigned length, const String&, Optional<ExpansionContext> = { });
> > +        TextContext(unsigned position, unsigned length, const String&, bool needsHyphen = false, Optional<ExpansionContext> = { });
> 
> I can't find where you are using the optional constructor parameters. They
> seem to be set via the setters. Can you remove them?
Done.

> 
> > Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:514
> > +        for (auto index = inlineContent.runs.size(); index--;) {
> > +            auto& displayRun = inlineContent.runs[index];
> > +            if (!displayRun.textContext())
> > +                continue;
> > +            // This has to be a run on this new line.
> > +            ASSERT(!previousRunCount || index > previousRunCount - 1);
> 
> Is the assert important? This would be nicer with range-for.
> 
> > Source/WebCore/layout/inlineformatting/InlineFormattingContext.cpp:519
> > +            break;
> 
> Not a fan of for loops that don't loop over the body.
Not a good practice indeed. I ended up doing it differently by tracking the index of the last text item instead.

> 
> > Source/WebCore/layout/integration/LayoutIntegrationLineLayout.cpp:227
> > -        TextRun textRun { textContext.content(), logicalLeft, horizontalExpansion, behavior };
> > +        String textWithHyphen;
> > +        if (textContext.needsHyphen())
> > +            textWithHyphen = makeString(textContext.content(), style.hyphenString());
> > +        TextRun textRun { !textWithHyphen.isEmpty() ? textWithHyphen : textContext.content(), logicalLeft, horizontalExpansion, behavior };
> 
> Isn't there are FIXME here you remove?
Oh right! Removed.