Use is<>() / downcast<>() for RenderBlock objects
Created attachment 239455 [details] Patch
Created attachment 239458 [details] Patch
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.
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<>()?
Created attachment 239488 [details] Patch
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.
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.
Created attachment 239496 [details] Patch
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.
(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.
Created attachment 239499 [details] Patch
Comment on attachment 239499 [details] Patch Clearing flags on attachment: 239499 Committed r174480: <http://trac.webkit.org/changeset/174480>
All reviewed patches have been landed. Closing bug.