Bug 1449530 - clean up ATK states mapping, r=eeejay

accessible/atk/AccessibleWrap.cpp

@@ -930,9 +930,8 @@ TranslateStates(uint64_t aState, AtkStateSet* aStateSet)
     aState &= ~states::EDITABLE;
 
   // Convert every state to an entry in AtkStateMap
-  uint32_t stateIndex = 0;
   uint64_t bitMask = 1;
-  while (gAtkStateMap[stateIndex].stateMapEntryType != kNoSuchState) {
+  for (auto stateIndex = 0U; stateIndex < gAtkStateMapLen; stateIndex++) {
     if (gAtkStateMap[stateIndex].atkState) { // There's potentially an ATK state for this
       bool isStateOn = (aState & bitMask) != 0;
       if (gAtkStateMap[stateIndex].stateMapEntryType == kMapOpposite) {
@@ -943,7 +942,6 @@ TranslateStates(uint64_t aState, AtkStateSet* aStateSet)
       }
     }
     bitMask <<= 1;
-    ++ stateIndex;
   }
 }
 
@@ -1539,24 +1537,32 @@ a11y::ProxyCaretMoveEvent(ProxyAccessible* aTarget, int32_t aOffset)
 void
 MaiAtkObject::FireStateChangeEvent(uint64_t aState, bool aEnabled)
 {
-    int32_t stateIndex = AtkStateMap::GetStateIndexFor(aState);
+  auto state = aState;
-    if (stateIndex >= 0) {
+  int32_t stateIndex = -1;
-        NS_ASSERTION(gAtkStateMap[stateIndex].stateMapEntryType != kNoSuchState,
+  while (state > 0) {
-                     "No such state");
+    ++stateIndex;
+    state >>= 1;
+  }
 
-        if (gAtkStateMap[stateIndex].atkState != kNone) {
+  MOZ_ASSERT(stateIndex >= 0 && stateIndex < gAtkStateMapLen,
-            NS_ASSERTION(gAtkStateMap[stateIndex].stateMapEntryType != kNoStateChange,
+             "No ATK state for internal state was found");
-                         "State changes should not fired for this state");
+  if (stateIndex < 0 || stateIndex >= static_cast<int32_t>(gAtkStateMapLen)) {
+    return;
+  }
 
-            if (gAtkStateMap[stateIndex].stateMapEntryType == kMapOpposite)
+  if (gAtkStateMap[stateIndex].atkState != kNone) {
-                aEnabled = !aEnabled;
+    MOZ_ASSERT(gAtkStateMap[stateIndex].stateMapEntryType != kNoStateChange,
+                 "State changes should not fired for this state");
 
-            // Fire state change for first state if there is one to map
+    if (gAtkStateMap[stateIndex].stateMapEntryType == kMapOpposite) {
-            atk_object_notify_state_change(&parent,
+      aEnabled = !aEnabled;
-                                           gAtkStateMap[stateIndex].atkState,
-                                           aEnabled);
-        }
     }
+
+    // Fire state change for first state if there is one to map
+    atk_object_notify_state_change(&parent,
+                                   gAtkStateMap[stateIndex].atkState,
+                                   aEnabled);
+  }
 }
 
 void

accessible/atk/nsStateMap.h

@@ -7,6 +7,8 @@
 #include <atk/atk.h>
 #include "AccessibleWrap.h"
 
+#include <type_traits>
+
 /******************************************************************************
 The following accessible states aren't translated, just ignored:
   STATE_READONLY:        Supported indirectly via EXT_STATE_EDITABLE
@@ -39,7 +41,6 @@ enum EStateMapEntryType {
   kMapDirectly,
   kMapOpposite,   // For example, UNAVAILABLE is the opposite of ENABLED
   kNoStateChange, // Don't fire state change event
-  kNoSuchState
 };
 
 const AtkStateType kNone = ATK_STATE_INVALID;
@@ -47,16 +48,6 @@ const AtkStateType kNone = ATK_STATE_INVALID;
 struct AtkStateMap {
   AtkStateType atkState;
   EStateMapEntryType stateMapEntryType;
-
-  static int32_t GetStateIndexFor(uint64_t aState)
-  {
-    int32_t stateIndex = -1;
-    while (aState > 0) {
-      ++ stateIndex;
-      aState >>= 1;
-    }
-    return stateIndex;  // Returns -1 if not mapped
-  }
 };
 
 
@@ -110,6 +101,10 @@ static const AtkStateMap gAtkStateMap[] = {                     // Cross Platfor
   { ATK_STATE_SENSITIVE,                      kMapDirectly },   // states::SENSITIVE               = 1 << 45
   { ATK_STATE_EXPANDABLE,                     kMapDirectly },   // states::EXPANDABLE              = 1 << 46
   { kNone,                                    kMapDirectly },   // states::PINNED                  = 1 << 47
-  { ATK_STATE_ACTIVE,                         kMapDirectly },   // states::CURRENT                 = 1 << 48
+  { ATK_STATE_ACTIVE,                         kMapDirectly }    // states::CURRENT                 = 1 << 48
-  { kNone,                                    kNoSuchState },   //                                 = 1 << 49
 };
+
+static const auto gAtkStateMapLen = std::extent<decltype(gAtkStateMap)>::value;
+
+static_assert(((uint64_t) 0x1) << (gAtkStateMapLen - 1) == mozilla::a11y::states::LAST_ENTRY,
+              "ATK states map is out of sync with internal states");

accessible/base/States.h

@@ -283,6 +283,11 @@ namespace states {
    */
   const uint64_t CURRENT = ((uint64_t) 0x1) << 48;
 
+  /**
+   * Not a real state, used for static assertions.
+   */
+  const uint64_t LAST_ENTRY = CURRENT;
+
 } // namespace states
 } // namespace a11y
 } // namespace mozilla