Bug 137512 – Use is<>() / downcast<>() for RenderBlock objects

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Chris Dumez]

Use is<>() / downcast<>() for RenderBlock objects

[Chris Dumez]

Created attachment 239455 [details]
Patch

[Chris Dumez]

Created attachment 239458 [details]
Patch

[Darin Adler]

Comment on attachment 239458 [details]
Patch

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.

[Chris Dumez]

Comment on attachment 239458 [details]
Patch

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

>> Source/WebCore/rendering/RenderBlock.cpp:449
>> +        RenderBlock& currentBlock = downcast<RenderBlock>(*current);
> 
> Not clear to me why this is a safe cast.

Hmm. me neither.

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

Could you clarify why this would be better? I would understand for is<>() because it would avoid the null check, but why is it better for downcast<>()?

[Chris Dumez]

Created attachment 239488 [details]
Patch

[WebKit Commit Bot]

Attachment 239488 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:445:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 239458 [details]
Patch

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

>>> 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.
> 
> Could you clarify why this would be better? I would understand for is<>() because it would avoid the null check, but why is it better for downcast<>()?

Maybe it’s not. I think I was confused because in the general case, a typecast might have to do extra work to properly handle a null pointer, but as I realized when you were talking to me about this before, that’s only when multiple inheritance is involved.

[Chris Dumez]

Created attachment 239496 [details]
Patch

[WebKit Commit Bot]

Attachment 239496 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderBlock.cpp:445:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 239458 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239458&action=review
> 
> >> Source/WebCore/rendering/RenderBlock.cpp:449
> >> +        RenderBlock& currentBlock = downcast<RenderBlock>(*current);
> > 
> > Not clear to me why this is a safe cast.
> 
> Hmm. me neither.

dhyatt found out this is actually dead code so I'll just remove the method before landing.

[Chris Dumez]

Created attachment 239499 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 239499 [details]
Patch

Clearing flags on attachment: 239499

Committed r174480: <http://trac.webkit.org/changeset/174480>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.