[webkit-reviews] review granted: [Bug 137512] Use is<>() / downcast<>() for RenderBlock objects : [Attachment 239458] 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 137512: Use is<>() / downcast<>() for RenderBlock objects
https://bugs.webkit.org/show_bug.cgi?id=137512

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

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


> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:200
>  static inline RenderObject* lastChildConsideringContinuation(RenderObject*
renderer)

This function dereferences the renderer, so it should eventually change to take
a RenderObject&.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:207
> +    if (!is<RenderInline>(*current) && !is<RenderBlock>(*current))
>	   return renderer;

Really seems like this code should just use renderer, not current. In fact, I
see you did that for the endOfContinuations function below.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:209
> +    while (current) {

Would be nicer as a for loop.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:274
> +    RenderObject* current = &renderer;
> +    while (current) {

This should be a for loop.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:293
> +    while (currentContainer) {

Should be a for loop.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:296
> +	       current = currentContainer->firstChild();
> +	       while (current) {

Should be a for loop.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:340
> +	   RenderElement* firstParent =
startOfContinuations(*m_renderer->firstChildSlow())->parent();

Might even consider auto* instead of RenderElement* so we get the most specific
type (which, I suppose, is RenderElement).

> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:213
> +	   && (is<RenderBlock>(*renderer) &&
downcast<RenderBlock>(*renderer).inlineElementContinuation() == nil)

The use of nil here is incorrect, since inlineElementContinuation() does not
return an Objective-C object pointer. This should be ! just as with any other
WebCore code, or nullptr if for some reason we need to use ==.

> Source/WebCore/rendering/RenderBlock.cpp:338
> +    while (current) {

Should be a for loop.

> Source/WebCore/rendering/RenderBlock.cpp:448
> +    while (current && current->isDescendantOf(fromBlock) && current !=
fromBlock) {

Would be nicer logically to make this a for loop, but might be hard to fit it
all on one line.

> Source/WebCore/rendering/RenderBlock.cpp:449
> +	   RenderBlock& currentBlock = downcast<RenderBlock>(*current);

Not clear to me why this is a safe cast.

> Source/WebCore/rendering/RenderBlock.cpp:790
> +	   RenderBlock* nextBlock = downcast<RenderBlock>(next);
> +	   RenderBlock* prevBlock = downcast<RenderBlock>(prev);

Should be references, since we null check both of these in the if just above.

> Source/WebCore/rendering/RenderBlock.cpp:3045
> +	   firstLineBlock = downcast<RenderBlock>(parentBlock);

Could use *parentBlock since we know it’s not null. But then we’d have to do &
outside the downcast.

> Source/WebCore/rendering/RenderBlock.cpp:3129
> +	   firstLetterBlock = downcast<RenderBlock>(parentBlock);

Could use *parentBlock since we know it’s not null. But then we’d have to do &
outside the downcast.

> Source/WebCore/rendering/RenderInline.cpp:469
> +    RenderBlock* post =
downcast<RenderBlock>(pre->createAnonymousBoxWithSameTypeAs(block));

Since createAnonymousBoxWithSameTypeAs won’t return null, this should be a
reference.

> Source/WebCore/rendering/RenderInline.cpp:856
> +    RenderBlock* containingBlock = this->containingBlock();

Code below seems to assume this is non-null. So I think we should use a
reference.

> Source/WebCore/rendering/RenderObject.cpp:723
> +    auto renderer = parent();

I think the parent should be named parent, not renderer, since this is also a
renderer.

> Source/WebCore/rendering/RenderObject.cpp:1712
> +    current = parent()->firstChild();
> +    while (current) {

This should be a for loop.

> Source/WebCore/rendering/RenderRuby.cpp:78
> +    return isRubyBeforeBlock(child) ? downcast<RenderBlock>(child) :
nullptr;

Should probably use &downcast<RenderBlock>(*child) since we checked for null
already.

> Source/WebCore/rendering/RenderRuby.cpp:84
> +    return isRubyAfterBlock(child) ? downcast<RenderBlock>(child) : nullptr;


Should probably use &downcast<RenderBlock>(*child) since we checked for null
already.

> Source/WebCore/rendering/RenderRubyBase.cpp:95
> +	       toBlock = downcast<RenderBlock>(lastChild);

Should probably use &downcast<RenderBlock>(*lastChild) since we checked for
null already.

> Source/WebCore/rendering/RenderRubyBase.cpp:122
> +	   RenderBlock* anonBlockHere = downcast<RenderBlock>(firstChildHere);
> +	   RenderBlock* anonBlockThere = downcast<RenderBlock>(lastChildThere);


Ditto.

> Source/WebCore/rendering/RenderThemeMac.mm:1882
>      while (renderBox != plugInRenderer) {

Should be a for loop.

> Source/WebCore/rendering/TextAutosizer.cpp:129
> +    RenderBlock* container = is<RenderBlock>(*layoutRoot) ?
downcast<RenderBlock>(layoutRoot) : layoutRoot->containingBlock();

Could be &downcast<RenderBlock>(&layoutRoot).

> Source/WebCore/rendering/TextAutosizer.cpp:214
> +	       RenderBlock* descendantBlock =
downcast<RenderBlock>(descendant);

Could be &downcast<RenderBlock>(*descendant)

> Source/WebCore/rendering/TextAutosizer.cpp:499
> +	       RenderBlock* descendantBlock =
downcast<RenderBlock>(descendant);

Ditto.

> Source/WebCore/rendering/TextAutosizer.cpp:546
> +	   return downcast<RenderBlock>(firstNode);

Ditto.