Bug 60255 – Refactor TextRun creation

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

[Nikolas Zimmermann]

TextRun has an ugly constructor, that takes 8 (!) arguments.
As first step I'm creating a TextRunFactory.h in rendering/ with helper functions to create TextRuns.
As a next step we can remove the TextRun catch-it-all constructor, and replace it with explicit setters/getters.
TextRunFactory will also play a central role in fixing bug 60524.

This whole work blocks bug 59085, my attempt to integrate SVG Fonts within the GlyphPage concept.

[Nikolas Zimmermann]

Created attachment 92385 [details]
Patch

[Early Warning System Bot]

Attachment 92385 [details] did not build on qt:
Build output: http://queues.webkit.org/results/8551816

[Nikolas Zimmermann]

Created attachment 92386 [details]
Patch v2

Missing include in SVGTextMetrics, not sure why it builds here..

[Dirk Schulze]

Comment on attachment 92385 [details]
Patch

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

Just some comments. LGTM in general. Waiting for your new patch to give r=me.

> Source/WebCore/rendering/InlineTextBox.cpp:1241
> +    RenderStyle* styleToUse = text->style(m_firstLine);

Can you assert this please?

> Source/WebCore/rendering/RenderFileUploadControl.cpp:277
> +        float charWidth = style()->font().width(createTextRunAllowingTrailingExpansion(String(&ch, 1), style()));

This function calls style() multiple times, can you move this to a local var please? (and assert it)

> Source/WebCore/rendering/TextRunFactory.cpp:55
> +    ASSERT(style);

you're asserting it in createTextRunForInlineTextBox(box, style, textRenderer->characters() + box->start(), box->len(), charactersWithHyphen); no need to do it twice, whats your opinion?

Comment on attachment 92386 [details]
Patch v2

This patch makes the code bigger (by about 100 lines, if I counted correctly) and more complicated; and turns several private members into public ones for no good reason. It appears that whatever benefit there is in this change, it can be achieved by adding or modifying TextRun constructors and factoring code into protected member functions in [SVG]InlineTextBox. Finally, the prefix “create” is conventionally used for functions that transfer ownership of a heap-allocated object, which is not the case here.

[Nikolas Zimmermann]

(In reply to comment #5)
> (From update of attachment 92386 [details])
> This patch makes the code bigger (by about 100 lines, if I counted correctly) and more complicated; and turns several private members into public ones for no good reason.
More complicated? Are you joking? :-)
The current TextRun creation is very fragile, when it comes to directionalOverride/direction handling, hence the several FIXMEs above the callsites that use TextRun (should this always be LTR?).

> It appears that whatever benefit there is in this change, it can be achieved by adding or modifying TextRun constructors and factoring code into protected member functions in [SVG]InlineTextBox. Finally, the prefix “create” is conventionally used for functions that transfer ownership of a heap-allocated object, which is not the case here.
Ok, I could use "constructTextRun", as it was done in SVGInlineTextBox before.

[Nikolas Zimmermann]

Examples:
TextRun textRun(string, length, false, 0, 0, TextRun::AllowTrailingExpansion, style()->direction(), style()->unicodeBidi() == Override);

TextRun(str.characters(), str.length(), false, 0, 0, TextRun::AllowTrailingExpansion, LTR, style->visuallyOrdered()), IntPoint(m_x + tx, m_y + ty + style->fontMetrics().ascent()

TextRun textRun(characters, length, textRenderer()->allowTabs(), textPos(), m_expansion, expansionBehavior(), direction(), m_dirOverride || styleToUse->visuallyOrdered());

I'm especially concerned regarding the direction, and the direction override handling, which is completly inconsistent atm. It's still inconsistent with this patch, as I didn't add "RespectDirection | RespectDirectionOverride" flags everywhere, but for sure this is easier now, than before.

[WebKit Review Bot]

Attachment 92385 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8554830

[Nikolas Zimmermann]

(In reply to comment #5)
> (From update of attachment 92386 [details])
> This patch makes the code bigger (by about 100 lines, if I counted correctly) and more complicated; and turns several private members into public ones for no good reason. It appears that whatever benefit there is in this change, it can be achieved by adding or modifying TextRun constructors and factoring code into protected member functions in [SVG]InlineTextBox. Finally, the prefix “create” is conventionally used for functions that transfer ownership of a heap-allocated object, which is not the case here.

Another note: I can't pass a RenderStyle* object to TextRun, which would extract the direction / directionalOverride settings, as that would be a layering violation. But still I'd like a central place which does that, instead of relying on the fact, that each call site of TextRun knows how to find out the direction, and/or wheter it's overriden (InlineBoxes need to check m_dirOverride || style->visuallyOrdered(), other places only need the style->visuallyOrdered() checks, etc.)

This patch was meant as starting point for more refactorizations, that I have in mind, all preparing for the master patch at bug https://bugs.webkit.org/show_bug.cgi?id=59085.

[WebKit Review Bot]

Attachment 92385 [details] did not pass chromium-ews:
Output: http://queues.webkit.org/results/8551840

[Collabora GTK+ EWS bot]

Attachment 92385 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/8554913

(In reply to comment #7)
> Examples:
> TextRun textRun(string, length, false, 0, 0, TextRun::AllowTrailingExpansion, style()->direction(), style()->unicodeBidi() == Override);
> 
> TextRun(str.characters(), str.length(), false, 0, 0, TextRun::AllowTrailingExpansion, LTR, style->visuallyOrdered()), IntPoint(m_x + tx, m_y + ty + style->fontMetrics().ascent()
> 
> TextRun textRun(characters, length, textRenderer()->allowTabs(), textPos(), m_expansion, expansionBehavior(), direction(), m_dirOverride || styleToUse->visuallyOrdered());
> 
> I'm especially concerned regarding the direction, and the direction override handling, which is completly inconsistent atm. It's still inconsistent with this patch, as I didn't add "RespectDirection | RespectDirectionOverride" flags everywhere, but for sure this is easier now, than before.

You’ve identified some real problems with the code and it’s good that you want to solve them. I rejected your proposed solution because it adds code and complexity and makes internal notions public. I suggested an alternative way of addressing the same problems, which is more in line with how we deal with such issues in the rendering code, and does not have the downsides I just pointed out.

[Nikolas Zimmermann]

Created attachment 92935 [details]
Patch v3

Less intrusive approach, w/o adding a TextRunFactory. I hope this is better now.

[Eric Seidel (no email)]

I'm sure Levi and Ryosuke will have opinions here.

[Nikolas Zimmermann]

(In reply to comment #12)
> (In reply to comment #7)
> > Examples:
> > TextRun textRun(string, length, false, 0, 0, TextRun::AllowTrailingExpansion, style()->direction(), style()->unicodeBidi() == Override);
> > 
> > TextRun(str.characters(), str.length(), false, 0, 0, TextRun::AllowTrailingExpansion, LTR, style->visuallyOrdered()), IntPoint(m_x + tx, m_y + ty + style->fontMetrics().ascent()
> > 
> > TextRun textRun(characters, length, textRenderer()->allowTabs(), textPos(), m_expansion, expansionBehavior(), direction(), m_dirOverride || styleToUse->visuallyOrdered());
> > 
> > I'm especially concerned regarding the direction, and the direction override handling, which is completly inconsistent atm. It's still inconsistent with this patch, as I didn't add "RespectDirection | RespectDirectionOverride" flags everywhere, but for sure this is easier now, than before.
> 
> You’ve identified some real problems with the code and it’s good that you want to solve them. I rejected your proposed solution because it adds code and complexity and makes internal notions public. I suggested an alternative way of addressing the same problems, which is more in line with how we deal with such issues in the rendering code, and does not have the downsides I just pointed out.

Dan, it's been 10+ days since your last reaction. You're ignoring private mails as well. Shall I find another reviewer?

[Dirk Schulze]

Comment on attachment 92935 [details]
Patch v3

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?

[Eric Seidel (no email)]

Comment on attachment 92935 [details]
Patch v3

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

Despite mitz being AWOL, this is definitely code that I would want hyatt or mitz to chime in on.  I understand what you're doing is very just moving stuff into create functions.  But I odn't really understand the "allowTrailingExpansion" flag, or why that justifies being a separate constructor?

> Source/WebCore/rendering/InlineTextBox.cpp:1292
> +    return constructTextRun(style, textRenderer->characters() + start(), len(), charactersWithHyphen);

What about direction() here?

> Source/WebCore/rendering/RenderFileUploadControl.cpp:280
> +        float charWidth = style->font().width(constructTextRunAllowTrailingExpansion(String(&ch, 1), style));

It seems like single-char text runs used for char measuring are a special case and should be treated that way. :)

[Eric Seidel (no email)]

Comment on attachment 92935 [details]
Patch v3

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

> Source/WebCore/rendering/EllipsisBox.cpp:57
> +    context->drawText(style->font(), RenderBlock::constructTextRunAllowTrailingExpansion(m_str, style), IntPoint(m_x + tx, m_y + ty + style->fontMetrics().ascent()));

Why don't we just split off a separte constructor instead of making a construct function?  I guess the construct function allows us to easily use members of the RenderBlock?  I don't see this function using any though...

> Source/WebCore/rendering/InlineTextBox.cpp:1076
> +    TextRun run = constructTextRun(style);

This function on the other hand, does make a lot of sense.  This allows us to share repeated calls to textRenderer(), textPos(), expansionBehavior(), etc. and so having a helper function for text run consturction on InlineBox sounds like a good idea.

> Source/WebCore/rendering/RenderBlock.cpp:6339
> +    return TextRun(characters, length, false, 0, 0, TextRun::AllowTrailingExpansion, textDirection, directionalOverride);

This feels like a separtae TextRun constructor, since you always pass false, 0, 0 here.

[Nikolas Zimmermann]

(In reply to comment #17)
> (From update of attachment 92935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92935&action=review
> 
> Despite mitz being AWOL, this is definitely code that I would want hyatt or mitz to chime in on.  I understand what you're doing is very just moving stuff into create functions.  But I odn't really understand the "allowTrailingExpansion" flag, or why that justifies being a separate constructor?
I guess you meant a seperate function?

> 
> > Source/WebCore/rendering/InlineTextBox.cpp:1292
> > +    return constructTextRun(style, textRenderer->characters() + start(), len(), charactersWithHyphen);
> 
> What about direction() here?
This is handled by constructTextRun, that's being called?

> 
> > Source/WebCore/rendering/RenderFileUploadControl.cpp:280
> > +        float charWidth = style->font().width(constructTextRunAllowTrailingExpansion(String(&ch, 1), style));
> 
> It seems like single-char text runs used for char measuring are a special case and should be treated that way. :)
We can for sure do more refactoring here, and I plan to do that!

[Nikolas Zimmermann]

(In reply to comment #18)
> (From update of attachment 92935 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92935&action=review
> 
> > Source/WebCore/rendering/EllipsisBox.cpp:57
> > +    context->drawText(style->font(), RenderBlock::constructTextRunAllowTrailingExpansion(m_str, style), IntPoint(m_x + tx, m_y + ty + style->fontMetrics().ascent()));
> 
> Why don't we just split off a separte constructor instead of making a construct function?  I guess the construct function allows us to easily use members of the RenderBlock?  I don't see this function using any though...
The introduction of these functions has a differnet purpose. I want to kill the RenderObject layering violation in TextRun, this is just the preparation patch. I think the next patch will make it much clearer.

> 
> > Source/WebCore/rendering/InlineTextBox.cpp:1076
> > +    TextRun run = constructTextRun(style);
> 
> This function on the other hand, does make a lot of sense.  This allows us to share repeated calls to textRenderer(), textPos(), expansionBehavior(), etc. and so having a helper function for text run consturction on InlineBox sounds like a good idea.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:6339
> > +    return TextRun(characters, length, false, 0, 0, TextRun::AllowTrailingExpansion, textDirection, directionalOverride);
> 
> This feels like a separtae TextRun constructor, since you always pass false, 0, 0 here.
Definately, I delayed this for a further patch...

[Nikolas Zimmermann]

Landed in r86739.