| Summary: | Use is<>() / downcast<>() for RenderText / RenderTextFragment | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> |
| Component: | Layout and Rendering | Assignee: | Chris Dumez <cdumez> |
| Status: | RESOLVED FIXED | ||
| Severity: | Normal | CC: | aestes, benjamin, buildbot, commit-queue, darin, kling, rniwa, simon.fraser |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 137424 | ||
| Bug Blocks: | |||
| Attachments: | |||
|
Description
Chris Dumez
2014-10-06 20:53:26 PDT
Created attachment 239381 [details]
Patch
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.
Created attachment 239382 [details]
Patch
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.
Created attachment 239383 [details]
Patch
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 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 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 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 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
Created attachment 239397 [details]
Patch
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 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 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 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. > 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 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. Created attachment 239415 [details]
Patch
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 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 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 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. 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
Created attachment 239422 [details]
Patch
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 on attachment 239422 [details] Patch Clearing flags on attachment: 239422 Committed r174403: <http://trac.webkit.org/changeset/174403> All reviewed patches have been landed. Closing bug. 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. Sorry, commented in the wrong bug. Please ignore. |