Use modern for-loops in WebCore/rendering - 1 of 2
Created attachment 254626 [details] Patch
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.
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.
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
Created attachment 254628 [details] Patch
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.
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.
I think we need to start ramping down on changes like this, since this time of year is a stabilization period for Apple.
(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.
But I will probably slow down on reviewing these kinds of patches.
Created attachment 254784 [details] Patch
(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?
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.
(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.
(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.
Comment on attachment 254784 [details] Patch Clearing flags on attachment: 254784 Committed r185512: <http://trac.webkit.org/changeset/185512>
All reviewed patches have been landed. Closing bug.
This broke the world, rolling out: <https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/4922>.
Re-opened since this is blocked by bug 145932
Please run tests with a debug build before submitting patches.
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
(In reply to comment #20) > Please run tests with a debug build before submitting patches. Okay, I will do that.
(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.
Created attachment 254866 [details] Patch
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.
(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.