[webkit-reviews] review granted: [Bug 137476] Use is<>() / downcast<>() for RenderText / RenderTextFragment : [Attachment 239397] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 137476: Use is<>() / downcast<>() for RenderText / RenderTextFragment
https://bugs.webkit.org/show_bug.cgi?id=137476

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=239397&action=review


> Source/WebCore/WebCore.exp.in:1631
> +__ZNK7WebCore12RenderInline16linesBoundingBoxEv

ChangeLog doesn’t explain this change.

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:5416
> +		BCEA4878097D93020094C9E4 /* RenderInline.h in Headers */ = {isa
= PBXBuildFile; fileRef = BCEA4839097D93020094C9E4 /* RenderInline.h */;
settings = {ATTRIBUTES = (Private, ); }; };

ChangeLog doesn’t explain this change.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:703
> +		   return String(renderTextFragment.contentString());

Not sure what the String() here is for. I suggest removing it.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:706
> +	       return String(renderTextObject.text());

I’m not sure that the explicit String() is needed here. I suggest removing it.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3446
>      while (renderer && !renderer->isText())

Missed updating this one.

> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:245
> +    RenderObject *renderer = core(self)->renderer();

Should fix the formatting while here, with the * in the wrong place.

> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:248
> +	   RenderText &renderText = downcast<RenderText>(*renderer);
> +	   return renderText.style().computedLineHeight();

Should either fix the formatting of RenderText& or get rid of the local
variable and do this all in one line.

> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:319
> +		   RenderText &t = downcast<RenderText>(inlineBox->renderer());

> +		   if (t.textNode())
> +		       return kit(t.textNode());

Would be nice to fix the formatting or RenderText& and one-character variable
name while here.

> Source/WebCore/dom/ContainerNode.cpp:850
> +	       if (is<RenderText>(*o) &&
downcast<RenderText>(*o).firstTextBox()) {
> +		   point.move(downcast<RenderText>(*o).linesBoundingBox().x(),
downcast<RenderText>(*o).firstTextBox()->root().lineTop());
> +	       } else if (is<RenderBox>(*o)) {

One line body doesn’t need an if.

> Source/WebCore/dom/ContainerNode.cpp:852
> +		   RenderBox& box = downcast<RenderBox>(*o);
> +		   point.moveBy(box.location());

As I’ve said many other places, I do think this reads better without the local.


> Source/WebCore/dom/ContainerNode.cpp:906
> +		   RenderBox& box = downcast<RenderBox>(*o);
> +		   point.moveBy(box.frameRect().maxXMaxYCorner());

As I’ve said many other places, I do think this reads better without the local.


> Source/WebCore/rendering/InlineIterator.h:86
>      inline bool atTextParagraphSeparator()

Might be nice to remove this unneeded and unhelpful "inline" while you are
here.

> Source/WebCore/rendering/InlineIterator.h:89
> +	   return is<RenderText>(m_renderer) && m_renderer->preservesNewline()
&& downcast<RenderText>(*m_renderer).textLength()
> +	       && downcast<RenderText>(*m_renderer).characterAt(m_pos) == '\n';


I don’t think the check that textLength() is non-zero here is valuable, since
characterAt already checks m_pos against the length. We should remove that.

> Source/WebCore/rendering/RenderInline.h:56
> +    virtual IntRect borderBoundingBox() const override final {

This is not the correct formatting for WebKit coding style. The brace goes on
the next line as it did in the code before it was moved.

ChangeLog doesn’t explain this change.

> Source/WebCore/rendering/RenderLineBoxList.cpp:339
> +    RenderObject* current = nullptr;

Not helpful to initialize this here since it’s overwritten immediately
afterward. But also, it’s not used outside the loop, so it should be defined
inside the for statement.

> Source/WebCore/rendering/RenderText.cpp:965
> +static inline bool isInlineFlowOrEmptyText(const RenderObject* renderer)

Since this dereferences the pointer immediately without checking, it should be
changed to take a reference instead of a pointer.

> Source/WebCore/rendering/TextAutoSizing.cpp:104
> +    RenderText& renderText = downcast<RenderText>(*node->renderer());
> +    renderText.setCandidateComputedTextSize(size);

Might read better in a single line without the local.

> Source/WebCore/rendering/TextAutosizer.cpp:498
> +	   if (!skipLocalText && is<RenderText>(*descendant)) {
> +	       textWidth +=
downcast<RenderText>(*descendant).renderedTextLength() *
descendant->style()->specifiedFontSize();
>	   } else if (isAutosizingContainer(descendant)) {

WebKit coding style doesn't put braces on a single line like this.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:920
> +    RenderText& nextText = downcast<RenderText>(next);

I don’t see why this cast is safe. The caller seems to check only that it’s
either text or an empty inline. What guarantees it’s text here!?

I was going to suggest we put the typecast closer to the type check, in the
callers rather than in this function, which would make the problem clearer, i
think.

> Source/WebCore/rendering/line/BreakingContextInlineHeaders.h:922
> +    if (!nextText.textLength())
>	   return false;

This special case for a length of zero isn’t needed, because character will do
that check anyway.