Bug 137476 - Use is<>() / downcast<>() for RenderText / RenderTextFragment
Summary: Use is<>() / downcast<>() for RenderText / RenderTextFragment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on: 137424
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-06 20:53 PDT by Chris Dumez
Modified: 2014-10-08 10:19 PDT (History)
8 users (show)

See Also:


Attachments
Patch (138.86 KB, patch)
2014-10-06 21:16 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (139.11 KB, patch)
2014-10-06 21:29 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (139.77 KB, patch)
2014-10-06 21:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (486.50 KB, application/zip)
2014-10-06 22:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (503.41 KB, application/zip)
2014-10-06 23:15 PDT, Build Bot
no flags Details
Patch (140.07 KB, patch)
2014-10-06 23:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (147.04 KB, patch)
2014-10-07 10:39 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-03 for mac-mountainlion (190.42 KB, application/zip)
2014-10-07 11:32 PDT, WebKit Commit Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (259.55 KB, application/zip)
2014-10-07 11:34 PDT, Build Bot
no flags Details
Patch (147.07 KB, patch)
2014-10-07 11:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-06 20:53:26 PDT
Use is<>() / downcast<>() for RenderText / RenderTextFragment
Comment 1 Chris Dumez 2014-10-06 21:16:44 PDT
Created attachment 239381 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-06 21:18:03 PDT
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.
Comment 3 Chris Dumez 2014-10-06 21:29:31 PDT
Created attachment 239382 [details]
Patch
Comment 4 WebKit Commit Bot 2014-10-06 21:31:24 PDT
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.
Comment 5 Chris Dumez 2014-10-06 21:45:26 PDT
Created attachment 239383 [details]
Patch
Comment 6 WebKit Commit Bot 2014-10-06 21:48:29 PDT
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.
Comment 7 Build Bot 2014-10-06 22:56:17 PDT
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
Comment 8 Build Bot 2014-10-06 22:56:21 PDT
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
Comment 9 Build Bot 2014-10-06 23:15:32 PDT
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
Comment 10 Build Bot 2014-10-06 23:15:36 PDT
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
Comment 11 Chris Dumez 2014-10-06 23:34:02 PDT
Created attachment 239397 [details]
Patch
Comment 12 WebKit Commit Bot 2014-10-06 23:37:27 PDT
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.
Comment 13 Darin Adler 2014-10-07 00:11:27 PDT
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.
Comment 14 Chris Dumez 2014-10-07 08:29:38 PDT
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.
Comment 15 Darin Adler 2014-10-07 09:31:53 PDT
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.
Comment 16 Simon Fraser (smfr) 2014-10-07 10:00:05 PDT
> 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
Comment 17 Chris Dumez 2014-10-07 10:15:54 PDT
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.
Comment 18 Chris Dumez 2014-10-07 10:39:17 PDT
Created attachment 239415 [details]
Patch
Comment 19 WebKit Commit Bot 2014-10-07 10:41:27 PDT
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.
Comment 20 WebKit Commit Bot 2014-10-07 11:32:38 PDT
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
Comment 21 WebKit Commit Bot 2014-10-07 11:32:44 PDT
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
Comment 22 Build Bot 2014-10-07 11:34:49 PDT
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.
Comment 23 Build Bot 2014-10-07 11:34:57 PDT
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
Comment 24 Chris Dumez 2014-10-07 11:49:23 PDT
Created attachment 239422 [details]
Patch
Comment 25 WebKit Commit Bot 2014-10-07 11:52:47 PDT
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.
Comment 26 WebKit Commit Bot 2014-10-07 12:33:56 PDT
Comment on attachment 239422 [details]
Patch

Clearing flags on attachment: 239422

Committed r174403: <http://trac.webkit.org/changeset/174403>
Comment 27 WebKit Commit Bot 2014-10-07 12:34:04 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Andy Estes 2014-10-08 10:11:17 PDT
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.
Comment 29 Andy Estes 2014-10-08 10:19:04 PDT
Sorry, commented in the wrong bug. Please ignore.