Bug 126915 - AX: Modernize AccessibilityChildrenVector loops
Summary: AX: Modernize AccessibilityChildrenVector loops
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-13 10:10 PST by chris fleizach
Modified: 2014-10-30 23:29 PDT (History)
12 users (show)

See Also:


Attachments
patch (42.18 KB, patch)
2014-01-13 13:02 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (42.18 KB, patch)
2014-01-13 13:06 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (43.42 KB, patch)
2014-01-14 08:44 PST, chris fleizach
andersca: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description chris fleizach 2014-01-13 10:10:26 PST
We can use auto and modern style loops
Comment 1 Radar WebKit Bug Importer 2014-01-13 10:12:21 PST
<rdar://problem/15805367>
Comment 2 chris fleizach 2014-01-13 13:02:40 PST
Created attachment 221076 [details]
patch
Comment 3 WebKit Commit Bot 2014-01-13 13:04:19 PST
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.
Comment 4 chris fleizach 2014-01-13 13:06:30 PST
Created attachment 221077 [details]
patch
Comment 5 chris fleizach 2014-01-14 08:44:36 PST
Created attachment 221166 [details]
patch

Change from auto to auto& where appropriate
Comment 6 Anders Carlsson 2014-01-14 09:05:50 PST
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 7 Build Bot 2014-01-14 09:09:55 PST
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 8 Build Bot 2014-01-14 09:35:46 PST
Comment on attachment 221166 [details]
patch

Attachment 221166 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5036550979059712
Comment 9 chris fleizach 2014-01-14 10:07:57 PST
Thanks Anders

http://trac.webkit.org/changeset/161979
Comment 10 Chris Dumez 2014-10-30 23:17:45 PDT
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 11 chris fleizach 2014-10-30 23:19:12 PDT
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
Comment 12 Chris Dumez 2014-10-30 23:22:27 PDT
(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 :)
Comment 13 chris fleizach 2014-10-30 23:24:56 PDT
(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
Comment 14 Chris Dumez 2014-10-30 23:29:39 PDT
(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!