Bug 145831 – Use modern for-loops in WebCore/rendering - 1

• Status: REOPENED
• Component: WebCore Misc.

[Hunseop Jeong]

Use modern for-loops in WebCore/rendering - 1 of 2

[Hunseop Jeong]

Created attachment 254626 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 22 files


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

[Build Bot]

Comment on attachment 254626 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 254627 [details]
Archive of layout-test-results from ews100 for mac-mavericks

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

[Hunseop Jeong]

Created attachment 254628 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 22 files


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

[Darin Adler]

Comment on attachment 254628 [details]
Patch

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

que

> Source/WebCore/rendering/FloatingObjects.cpp:270
> +        ASSERT(!(floatingObject->originatingLine()) || &(floatingObject->originatingLine()->renderer()) == &m_renderer);

The extra parentheses here make this harder to read for those used to WebKit coding style. Please write without them.

    ASSERT(!floatingObject->originatingLine() || &floatingObject->originatingLine()->renderer() == &m_renderer);

> Source/WebCore/rendering/FloatingObjects.cpp:271
> +        floatingObject->setOriginatingLine(0);

Please use nullptr.

> Source/WebCore/rendering/RenderBlock.cpp:196
>      if (std::unique_ptr<TrackedRendererListHashSet> descendantSet = descendantMap->take(block)) {

Should really use auto here instead of that long type!

> Source/WebCore/rendering/RenderBlock.cpp:198
> +            TrackedContainerMap::iterator it = containerMap->find(box);

Better to use auto here.

> Source/WebCore/rendering/RenderBlock.cpp:3728
> +    for (auto& currBox : *positionedDescendantSet)

I would call this just "box" rather than "currBox" to avoid the pointless abbreviation.

[Simon Fraser (smfr)]

I think we need to start ramping down on changes like this, since this time of year is a stabilization period for Apple.

[Darin Adler]

(In reply to comment #8)
> I think we need to start ramping down on changes like this, since this time
> of year is a stabilization period for Apple.

I understand your point. Our main tool for that is branching, though.

[Darin Adler]

But I will probably slow down on reviewing these kinds of patches.

[Hunseop Jeong]

Created attachment 254784 [details]
Patch

[Hunseop Jeong]

(In reply to comment #7)
> Comment on attachment 254628 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254628&action=review
> 
> que
> 
> > Source/WebCore/rendering/FloatingObjects.cpp:270
> > +        ASSERT(!(floatingObject->originatingLine()) || &(floatingObject->originatingLine()->renderer()) == &m_renderer);
> 
> The extra parentheses here make this harder to read for those used to WebKit
> coding style. Please write without them.
> 
>     ASSERT(!floatingObject->originatingLine() ||
> &floatingObject->originatingLine()->renderer() == &m_renderer);
> 
> > Source/WebCore/rendering/FloatingObjects.cpp:271
> > +        floatingObject->setOriginatingLine(0);
> 
> Please use nullptr.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:196
> >      if (std::unique_ptr<TrackedRendererListHashSet> descendantSet = descendantMap->take(block)) {
> 
> Should really use auto here instead of that long type!
> 
> > Source/WebCore/rendering/RenderBlock.cpp:198
> > +            TrackedContainerMap::iterator it = containerMap->find(box);
> 
> Better to use auto here.
> 
> > Source/WebCore/rendering/RenderBlock.cpp:3728
> > +    for (auto& currBox : *positionedDescendantSet)
> 
> I would call this just "box" rather than "currBox" to avoid the pointless
> abbreviation.

Done.
Thanks for the review. Could you check again?

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 22 files


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

[Hunseop Jeong]

(In reply to comment #13)
> Attachment 254784 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control
> clauses should use braces.  [whitespace/braces] [4]
> Total errors found: 2 in 22 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Aren't these errors false positives?
I don't know why it make the error.

[Darin Adler]

(In reply to comment #14)
> (In reply to comment #13)
> > Attachment 254784 [details] did not pass style-queue:
> > 
> > ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> > ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
> > Total errors found: 2 in 22 files
> > 
> > If any of these errors are false positives, please file a bug against
> > check-webkit-style.
> 
> Aren't these errors false positives?

Not 100% sure, but yes they seem to be false positives.

[WebKit Commit Bot]

Comment on attachment 254784 [details]
Patch

Clearing flags on attachment: 254784

Committed r185512: <http://trac.webkit.org/changeset/185512>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke the world, rolling out: <https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/4922>.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145932

[Simon Fraser (smfr)]

Please run tests with a debug build before submitting patches.

[Alexey Proskuryakov]

This also appears to have caused memory corruption detected by GuardMalloc on fast/multicol/newmulticol/multicol-inside-multicol.html:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010f376365 WebCore::RenderBlock::dirtyForLayoutFromPercentageHeightDescendants() + 101
1   com.apple.WebCore             	0x000000010fed7120 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) + 32
2   com.apple.WebCore             	0x000000010fed64c6 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 918
3   com.apple.WebCore             	0x000000010f375956 WebCore::RenderBlock::layout() + 54
4   com.apple.WebCore             	0x000000010f3757cd WebCore::RenderView::layout() + 765
5   com.apple.WebCore             	0x000000010f37131f WebCore::FrameView::layout(bool) + 3215
6   com.apple.WebCore             	0x000000010f36ff7f WebCore::Document::implicitClose() + 943
7   com.apple.WebCore             	0x000000010f36f783 WebCore::FrameLoader::checkCompleted() + 275
8   com.apple.WebCore             	0x000000010f36e554 WebCore::FrameLoader::finishedParsing() + 100
9   com.apple.WebCore             	0x000000010f36d2f1 WebCore::Document::finishedParsing() + 417
10  com.apple.WebCore             	0x000000010f3450b5 WebCore::HTMLDocumentParser::prepareToStopParsing() + 165
11  com.apple.WebCore             	0x000000010f43b891 WebCore::HTMLDocumentParser::resumeParsingAfterYield() + 129
12  com.apple.WebCore             	0x000000010f3243ff WebCore::ThreadTimers::sharedTimerFiredInternal() + 175
13  com.apple.WebCore             	0x000000010f324318 WebCore::timerFired(__CFRunLoopTimer*, void*) + 24

[Hunseop Jeong]

(In reply to comment #20)
> Please run tests with a debug build before submitting patches.

Okay, I will do that.

[Hunseop Jeong]

(In reply to comment #21)
> This also appears to have caused memory corruption detected by GuardMalloc
> on fast/multicol/newmulticol/multicol-inside-multicol.html:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebCore             	0x000000010f376365
> WebCore::RenderBlock::dirtyForLayoutFromPercentageHeightDescendants() + 101
> 1   com.apple.WebCore             	0x000000010fed7120
> WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) +
> 32
> 2   com.apple.WebCore             	0x000000010fed64c6
> WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 918
> 3   com.apple.WebCore             	0x000000010f375956
> WebCore::RenderBlock::layout() + 54
> 4   com.apple.WebCore             	0x000000010f3757cd
> WebCore::RenderView::layout() + 765
> 5   com.apple.WebCore             	0x000000010f37131f
> WebCore::FrameView::layout(bool) + 3215
> 6   com.apple.WebCore             	0x000000010f36ff7f
> WebCore::Document::implicitClose() + 943
> 7   com.apple.WebCore             	0x000000010f36f783
> WebCore::FrameLoader::checkCompleted() + 275
> 8   com.apple.WebCore             	0x000000010f36e554
> WebCore::FrameLoader::finishedParsing() + 100
> 9   com.apple.WebCore             	0x000000010f36d2f1
> WebCore::Document::finishedParsing() + 417
> 10  com.apple.WebCore             	0x000000010f3450b5
> WebCore::HTMLDocumentParser::prepareToStopParsing() + 165
> 11  com.apple.WebCore             	0x000000010f43b891
> WebCore::HTMLDocumentParser::resumeParsingAfterYield() + 129
> 12  com.apple.WebCore             	0x000000010f3243ff
> WebCore::ThreadTimers::sharedTimerFiredInternal() + 175
> 13  com.apple.WebCore             	0x000000010f324318
> WebCore::timerFired(__CFRunLoopTimer*, void*) + 24

Thank you to your confirmation.
After modification, I will upload the patch.

[Hunseop Jeong]

Created attachment 254866 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/rendering/RenderBlock.cpp:1312:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderBlock.cpp:1323:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 22 files


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

[Hunseop Jeong]

(In reply to comment #21)
> This also appears to have caused memory corruption detected by GuardMalloc
> on fast/multicol/newmulticol/multicol-inside-multicol.html:
> 
> Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
> 0   com.apple.WebCore             	0x000000010f376365
> WebCore::RenderBlock::dirtyForLayoutFromPercentageHeightDescendants() + 101
> 1   com.apple.WebCore             	0x000000010fed7120
> WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) +
> 32
> 2   com.apple.WebCore             	0x000000010fed64c6
> WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 918
> 3   com.apple.WebCore             	0x000000010f375956
> WebCore::RenderBlock::layout() + 54
> 4   com.apple.WebCore             	0x000000010f3757cd
> WebCore::RenderView::layout() + 765
> 5   com.apple.WebCore             	0x000000010f37131f
> WebCore::FrameView::layout(bool) + 3215
> 6   com.apple.WebCore             	0x000000010f36ff7f
> WebCore::Document::implicitClose() + 943
> 7   com.apple.WebCore             	0x000000010f36f783
> WebCore::FrameLoader::checkCompleted() + 275
> 8   com.apple.WebCore             	0x000000010f36e554
> WebCore::FrameLoader::finishedParsing() + 100
> 9   com.apple.WebCore             	0x000000010f36d2f1
> WebCore::Document::finishedParsing() + 417
> 10  com.apple.WebCore             	0x000000010f3450b5
> WebCore::HTMLDocumentParser::prepareToStopParsing() + 165
> 11  com.apple.WebCore             	0x000000010f43b891
> WebCore::HTMLDocumentParser::resumeParsingAfterYield() + 129
> 12  com.apple.WebCore             	0x000000010f3243ff
> WebCore::ThreadTimers::sharedTimerFiredInternal() + 175
> 13  com.apple.WebCore             	0x000000010f324318
> WebCore::timerFired(__CFRunLoopTimer*, void*) + 24

Source/WebCore/rendering/RenderBlock.cpp:1148
- for (auto& box : *descendants) {
+ for (auto* box : *descendants) {

I changed the reference to pointer because
'box' reassigned the pointer at 1160 line.

Source/WebCore/rendering/RenderBlock.cpp:1160
> box = box->containingBlock();

Maybe It makes the problem.


I ran tests with a debug build. Tests all passed.
I'll be cafeful before uploading the patch.