Summary: | Use modern for-loops in WebCore/dom. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Hunseop Jeong <hs85.jeong> | ||||||||
Component: | WebCore Misc. | Assignee: | Hunseop Jeong <hs85.jeong> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | allan.jensen, commit-queue, darin, esprehn+autocc, kangil.han, WebkitBugTracker | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Hunseop Jeong
2015-10-28 23:56:08 PDT
Created attachment 264306 [details]
Patch
Created attachment 264307 [details]
Patch
Comment on attachment 264307 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264307&action=review > Source/WebCore/dom/ContainerNode.cpp:285 > + treeScope().adoptIfNeeded(&child.get()); I believe the preferred style is child.ptr() rather than &child.get(). > Source/WebCore/dom/DocumentMarkerController.cpp:619 > + for (auto& marker : m_markers) { Not good to use "marker" as the name both for this outer loop and the inner loop. > Source/WebCore/dom/DocumentMarkerController.cpp:623 > - MarkerList* list = i->value.get(); > + MarkerList* list = marker.value.get(); Not sure why we need this local variable. > Source/WebCore/dom/DocumentMarkerController.cpp:800 > + for (auto& documentMarker : *marker.value.get()) We don’t need .get() here. > Source/WebCore/dom/EventListenerMap.cpp:71 > + for (auto& eventListener : *entry.second.get()) { No need for .get() here. > Source/WebCore/dom/SelectorQuery.cpp:448 > + SelectorCompiler::QuerySelectorSimpleSelectorChecker selectorChecker = SelectorCompiler::querySelectorSimpleSelectorCheckerFunction(compiledSelectorChecker, selector.compilationStatus); We should use auto here; this line is so long. > Source/WebCore/dom/SelectorQuery.cpp:452 > + SelectorCompiler::QuerySelectorSelectorCheckerWithCheckingContext selectorChecker = SelectorCompiler::querySelectorSelectorCheckerFunctionWithCheckingContext(compiledSelectorChecker, selector.compilationStatus); We should use auto here; this line is so long. Created attachment 264378 [details]
Patch
(In reply to comment #3) > Comment on attachment 264307 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264307&action=review > > > Source/WebCore/dom/ContainerNode.cpp:285 > > + treeScope().adoptIfNeeded(&child.get()); > > I believe the preferred style is child.ptr() rather than &child.get(). Done. > > > Source/WebCore/dom/DocumentMarkerController.cpp:619 > > + for (auto& marker : m_markers) { > > Not good to use "marker" as the name both for this outer loop and the inner > loop. Oops,, I changed the "marker" to documentMarker in inner loop. > > > Source/WebCore/dom/DocumentMarkerController.cpp:623 > > - MarkerList* list = i->value.get(); > > + MarkerList* list = marker.value.get(); > > Not sure why we need this local variable. > > > Source/WebCore/dom/DocumentMarkerController.cpp:800 > > + for (auto& documentMarker : *marker.value.get()) > > We don’t need .get() here. Removed the .get() > > > Source/WebCore/dom/EventListenerMap.cpp:71 > > + for (auto& eventListener : *entry.second.get()) { > > No need for .get() here. Ditto. > > > Source/WebCore/dom/SelectorQuery.cpp:448 > > + SelectorCompiler::QuerySelectorSimpleSelectorChecker selectorChecker = SelectorCompiler::querySelectorSimpleSelectorCheckerFunction(compiledSelectorChecker, selector.compilationStatus); > > We should use auto here; this line is so long. Done. > > > Source/WebCore/dom/SelectorQuery.cpp:452 > > + SelectorCompiler::QuerySelectorSelectorCheckerWithCheckingContext selectorChecker = SelectorCompiler::querySelectorSelectorCheckerFunctionWithCheckingContext(compiledSelectorChecker, selector.compilationStatus); > > We should use auto here; this line is so long. Ditto. Thanks for the review. Comment on attachment 264378 [details] Patch Clearing flags on attachment: 264378 Committed r191792: <http://trac.webkit.org/changeset/191792> All reviewed patches have been landed. Closing bug. |