Bug 137476 – Use is<>() / downcast<>() for RenderText / RenderTextFragment

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Chris Dumez]

Use is<>() / downcast<>() for RenderText / RenderTextFragment

[Chris Dumez]

Created attachment 239381 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 44 files


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

[Chris Dumez]

Created attachment 239382 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 44 files


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

[Chris Dumez]

Created attachment 239383 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 45 files


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

[Build Bot]

Comment on attachment 239383 [details]
Patch

Attachment 239383 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6034340492345344

New failing tests:
compositing/repaint/invalidations-on-composited-layers.html

[Build Bot]

Created attachment 239392 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239383 [details]
Patch

Attachment 239383 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5350754302820352

New failing tests:
compositing/repaint/invalidations-on-composited-layers.html

[Build Bot]

Created attachment 239393 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 239397 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 4 in 45 files


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

[Darin Adler]

Comment on attachment 239397 [details]
Patch

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.

[Chris Dumez]

Comment on attachment 239397 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:703
>> +                return String(renderTextFragment.contentString());
> 
> Not sure what the String() here is for. I suggest removing it.

It is because it returns a StringImpl*. We can probably remove the String() through as I don't think that constructor is explicit.

>> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:245
>> +    RenderObject *renderer = core(self)->renderer();
> 
> Should fix the formatting while here, with the * in the wrong place.

It seems the star is consistently on the wrong side in *.mm files so I thought we were using a different coding style there?

>> Source/WebCore/dom/ContainerNode.cpp:852
>> +                point.moveBy(box.location());
> 
> As I’ve said many other places, I do think this reads better without the local.

Yes, I remember and I did it in a few places. I just missed a few cases.

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

Arg. I actually changed that because the style script complained about it. The style script was apparently confused.

[Darin Adler]

Comment on attachment 239397 [details]
Patch

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

>>> Source/WebCore/bindings/objc/DOMUIKitExtensions.mm:245
>>> +    RenderObject *renderer = core(self)->renderer();
>> 
>> Should fix the formatting while here, with the * in the wrong place.
> 
> It seems the star is consistently on the wrong side in *.mm files so I thought we were using a different coding style there?

The rule we follow in new WebKit is to put the star on the right (the “wrong side”) for all Objective-C types, and put the star on the left (the “right side”) for all other types, such as C++ and C types.

This is such a pain that I would suggest that some day we change the rule so that we put the star on the right (the “wrong side”) only for Objective-C types in public headers to match the standard public API style, and put it on the left everywhere else.

[Simon Fraser (smfr)]

> This is such a pain that I would suggest that some day we change the rule so that we put the star on the right (the “wrong side”) only for Objective-C types in public headers to match the standard public API style, and put it on the left everywhere else.

+1

[Chris Dumez]

Comment on attachment 239397 [details]
Patch

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

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

It is actually used outside the loop but I'll make it uninitialized.

[Chris Dumez]

Created attachment 239415 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.h:57:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 46 files


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

[WebKit Commit Bot]

Comment on attachment 239415 [details]
Patch

Rejecting attachment 239415 [details] from commit-queue.

Number of test failures exceeded the failure limit.
Full output: http://webkit-queues.appspot.com/results/5638919764836352

[WebKit Commit Bot]

Created attachment 239418 [details]
Archive of layout-test-results from webkit-cq-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 239415 [details]
Patch

Attachment 239415 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5174414756806656

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 239419 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 239422 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderInline.cpp:574:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:934:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:961:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.cpp:993:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderInline.h:57:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 46 files


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

[WebKit Commit Bot]

Comment on attachment 239422 [details]
Patch

Clearing flags on attachment: 239422

Committed r174403: <http://trac.webkit.org/changeset/174403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Andy Estes]

r174403 caused media/track/track-forced-subtitles-in-band.html to start failing on the bots. I filed https://bugs.webkit.org/show_bug.cgi?id=137525 to track this issue.

[Andy Estes]

Sorry, commented in the wrong bug. Please ignore.