[webkit-reviews] review granted: [Bug 60255] Refactor TextRun creation : [Attachment 92935] Patch v3

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has granted Nikolas Zimmermann
<zimmermann at kde.org>'s request for review:
Bug 60255: Refactor TextRun creation
https://bugs.webkit.org/show_bug.cgi?id=60255

Attachment 92935: Patch v3
https://bugs.webkit.org/attachment.cgi?id=92935&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=92935&action=review

I'm not against a Ctor with 8 arguments, and I think that it makes sense, if
the object really needs all 8 arguments to interact in a sane way instead of
calling 8 setters afterwards (just as an example). But your general concept
looks sane, even if we may have different opinions about the ctor. Please
remove the comments about the Ctor with 8 arguments. This can be addressed in a
later patch. Instead concentrate your descriptions about the other benefits.
Layering violation, and the fact that you centralized some logic. r=me with
comments.

> Source/WebCore/ChangeLog:9
> +	   The long-term goal is to remove the ugly eight parameters
catch-it-all TextRun constructor, and

Please remove this sentence or make it as proposal, not as future plan.

> Source/WebCore/rendering/RenderBlock.cpp:6338
> +    // FIXME: Remove TextRuns all-in-one-constructor and use explicit
setters here.

Wouldn't add a fixme here.

> Source/WebCore/rendering/RenderTextControl.cpp:545
> +    return
style()->font().width(constructTextRunAllowTrailingExpansion(String(&ch, 1),
style()));

Why do you create a String here? Can't you use the
constructTextRunAllowTrailingExpansion with UChar* and length?