[webkit-reviews] review canceled: [Bug 96979] Update RenderText to use String instead of UChar* for text : [Attachment 167142] Patch with 8 bit TextRun creation wrapped in new feature macro - ENABLE_CAN_USE_8BIT_TEXTRUN

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has canceled Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 96979: Update RenderText to use String instead of UChar* for text
https://bugs.webkit.org/show_bug.cgi?id=96979

Attachment 167142: Patch with 8 bit TextRun creation wrapped in new feature
macro - ENABLE_CAN_USE_8BIT_TEXTRUN
https://bugs.webkit.org/attachment.cgi?id=167142&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
Made the changes suggested by Geoff.

(In reply to comment #12)
> (From update of attachment 167142 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=167142&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +	     now written as rt->getChar(n).
> 
> I would have called this characterAt()

Done!

> > Source/WebCore/ChangeLog:18
> > +	     for PLATFORM(MAC).  Other platform can update this setting in
Platform.h when their platform specific use of
> 
> s/use/uses/

Changed the sentence to: Other platform can update this setting in Platform.h
when their platform specific code is updated to TextRun's with 8 bit data.

> > Source/WebCore/rendering/InlineTextBox.cpp:862
> > +	 if (string.length() != static_cast<unsigned>(length) || m_start)
> > +	     string = string.substringSharingImpl(m_start, length);
> 
> Is there a reason why you added ASSERT(static_cast<unsigned>(m_start +
length) <= string.length()) above in paint() but not here in paintSelection()?

Added the second ASSERT.

> > Source/WebCore/rendering/InlineTextBox.h:110
> > +	 TextRun constructTextRun(RenderStyle*, const Font&, String, int,
BufferForAppendingHyphen* = 0) const;
> 
> I think you should have left the parameter name maximumLength here, since
it’s not implied by the type.

Added back parameter names.

> > Source/WebCore/rendering/RenderBlock.cpp:7555
> > +{
> > +	 ASSERT(style);
> > +
> > +	 TextDirection textDirection = LTR;
> > +	 bool directionalOverride = style->rtlOrdering() == VisualOrder;
> > +
> > +	 TextRun run(characters, length, 0, 0, expansion, textDirection,
directionalOverride);
> > +	 if (textRunNeedsRenderingContext(font))
> > +	    
run.setRenderingContext(SVGTextRunRenderingContext::create(context));
> > +
> > +	 return run;
> > +}
> 
> Couldn’t this have been written simply as a call to the version that takes
TextRunFlags, passing the DefaultTextRunFlags?

Passing the extra parameter cost performance.

> > Source/WebCore/rendering/RenderBlock.cpp:7619
> > +	 return constructTextRunInternal(context, font, string.characters(),
length, style, expansion, flags);
> 
> Why not characters16()?

Because characters() will up convert an 8 bit string if necessary.  This allows
an 8 bit string to create a 16 bit text run.

> > Source/WebCore/rendering/RenderBlock.h:272
> > +	 static TextRun constructTextRun(RenderObject* context, const Font&,
const RenderText*, unsigned, unsigned, RenderStyle*,
> 
> You should name the unsigned parameters here.

Done.

> > Source/WebCore/rendering/RenderCombineText.cpp:74
> > +void RenderCombineText::stringToRender(int start, String& string, int&
length) const
> 
> We normally use “get” in the name of functions that use out parameters like
this. How about getStringToRender()?

Done.

> > Source/WebCore/rendering/RenderCombineText.h:35
> > +	 void stringToRender(int, String&, int&) const;
> 
> I’d probably keep the parameter name here.

Done.

Note that this patch will fail the style-bot due to the parameter names in the
.h files.