[webkit-reviews] review granted: [Bug 204953] [LFC][IFC] Paint partial trailing run with hyphen when needed : [Attachment 385028] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has granted zalan <zalan at apple.com>'s request
for review:
Bug 204953: [LFC][IFC] Paint partial trailing run with hyphen when needed
https://bugs.webkit.org/show_bug.cgi?id=204953

Attachment 385028: Patch

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




--- Comment #3 from Antti Koivisto <koivisto at iki.fi> ---
Comment on attachment 385028
  --> https://bugs.webkit.org/attachment.cgi?id=385028
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?