We can use auto and modern style loops
<rdar://problem/15805367>
Created attachment 221076 [details] patch
Attachment 221076 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AXObjectCache.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGrid.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp', u'Source/WebCore/accessibility/AccessibilityARIAGridRow.cpp', u'Source/WebCore/accessibility/AccessibilityListBox.cpp', u'Source/WebCore/accessibility/AccessibilityListBoxOption.cpp', u'Source/WebCore/accessibility/AccessibilityMenuList.cpp', u'Source/WebCore/accessibility/AccessibilityMenuListPopup.cpp', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp', u'Source/WebCore/accessibility/AccessibilityTable.cpp', u'Source/WebCore/accessibility/AccessibilityTableColumn.cpp', u'Source/WebCore/accessibility/AccessibilityTableHeaderContainer.cpp', u'Source/WebCore/accessibility/AccessibilityTableRow.cpp', u'Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', '--commit-queue']" exit_code: 1 ERROR: Source/WebCore/accessibility/AccessibilityTable.cpp:508: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 19 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 221077 [details] patch
Created attachment 221166 [details] patch Change from auto to auto& where appropriate
Comment on attachment 221166 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=221166&action=review > Source/WebCore/accessibility/AXObjectCache.cpp:127 > + for (const auto& object : m_objects) { > + AccessibilityObject* axObject = object.value.get(); > + detachWrapper(axObject, CacheDestroyed); > + axObject->detach(CacheDestroyed); > + removeAXID(axObject); > } This can just iterate over m_objects.values(), then you don't need the object.value.get(). > Source/WebCore/accessibility/AccessibilityARIAGridCell.cpp:87 > + auto& siblings = parent->children(); I think this can be const auto&. > Source/WebCore/accessibility/AccessibilityListBoxOption.cpp:223 > + auto& listItems = selectElement->listItems(); const auto&. > Source/WebCore/accessibility/AccessibilityMenuList.cpp:111 > + auto& childObjects = children(); const auto&. > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:338 > + auto& children = child->children(); const auto&. > Source/WebCore/accessibility/AccessibilityObject.cpp:420 > + auto& searchChildren = object->isAccessibilityTable() ? toAccessibilityTable(object)->rows() : object->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3553 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3566 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3580 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3604 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3619 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3636 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3649 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:3664 > + auto& children = this->children(); const auto&. > Source/WebCore/accessibility/AccessibilityTable.cpp:508 > + auto& children = m_rows[rowIndex]->children(); const auto&. > Source/WebCore/accessibility/AccessibilityTableRow.cpp:120 > + auto& rowChildren = children(); const auto&. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:350 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:370 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1076 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:82 > + for (auto& pair : pairs) { It looks like this can be const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:166 > + for (auto text : textOrder) { const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:195 > + for (auto text : textOrder) { const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperBase.mm:221 > + for (auto text : textOrder) { const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2378 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3612 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3638 > + auto& children = m_object->children(); const auto&. > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3670 > + auto& children = m_object->children(); const auto&.
Comment on attachment 221166 [details] patch Attachment 221166 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6323373109936128
Comment on attachment 221166 [details] patch Attachment 221166 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5036550979059712
Thanks Anders http://trac.webkit.org/changeset/161979
Comment on attachment 221166 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=221166&action=review > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1650 > + for (const auto& selectedRow : selectedRows) I don't believe this change is correct. It ignores the count variable that is set above depending on isMultiSelectable() so I am assuming we can now select more than 1 row even if it is not multiSelectable.
Comment on attachment 221166 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=221166&action=review >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1650 >> + for (const auto& selectedRow : selectedRows) > > I don't believe this change is correct. It ignores the count variable that is set above depending on isMultiSelectable() so I am assuming we can now select more than 1 row even if it is not multiSelectable. I think you're right too
(In reply to comment #11) > Comment on attachment 221166 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=221166&action=review > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1650 > >> + for (const auto& selectedRow : selectedRows) > > > > I don't believe this change is correct. It ignores the count variable that is set above depending on isMultiSelectable() so I am assuming we can now select more than 1 row even if it is not multiSelectable. > > I think you're right too This was reported by clang static analyzer. Would you mind making the fix? I don't feel like writing the layout test for this :)
(In reply to comment #12) > (In reply to comment #11) > > Comment on attachment 221166 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=221166&action=review > > > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1650 > > >> + for (const auto& selectedRow : selectedRows) > > > > > > I don't believe this change is correct. It ignores the count variable that is set above depending on isMultiSelectable() so I am assuming we can now select more than 1 row even if it is not multiSelectable. > > > > I think you're right too > > This was reported by clang static analyzer. > Would you mind making the fix? I don't feel like writing the layout test for > this :) https://bugs.webkit.org/show_bug.cgi?id=138241
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Comment on attachment 221166 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=221166&action=review > > > > > > >> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1650 > > > >> + for (const auto& selectedRow : selectedRows) > > > > > > > > I don't believe this change is correct. It ignores the count variable that is set above depending on isMultiSelectable() so I am assuming we can now select more than 1 row even if it is not multiSelectable. > > > > > > I think you're right too > > > > This was reported by clang static analyzer. > > Would you mind making the fix? I don't feel like writing the layout test for > > this :) > > https://bugs.webkit.org/show_bug.cgi?id=138241 Thanks!