Bug 60255

Created attachment 92385 [details] Patch

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

Created attachment 92386 [details] Patch v2 Missing include in SVGTextMetrics, not sure why it builds here..

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.

(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.

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.

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

(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.

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

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.

Created attachment 92935 [details] Patch v3 Less intrusive approach, w/o adding a TextRunFactory. I hope this is better now.

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

(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?

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?

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. :)

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.

(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!

(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...

Landed in r86739.