Bug 126915 – AX: Modernize AccessibilityChildrenVector loops

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[chris fleizach]

We can use auto and modern style loops

[Radar WebKit Bug Importer]

<rdar://problem/15805367>

[chris fleizach]

Created attachment 221076 [details]
patch

[WebKit Commit Bot]

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.

[chris fleizach]

Created attachment 221077 [details]
patch

[chris fleizach]

Created attachment 221166 [details]
patch

Change from auto to auto& where appropriate

[Anders Carlsson]

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&.

[Build Bot]

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

[Build Bot]

Comment on attachment 221166 [details]
patch

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

[chris fleizach]

Thanks Anders

http://trac.webkit.org/changeset/161979

[Chris Dumez]

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.

[chris fleizach]

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

[Chris Dumez]

(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 :)

[chris fleizach]

(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

[Chris Dumez]

(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!