Add support for attribute change callbacks in AXNodeData.

ui/accessibility/ax_tree.cc

Index: ui/accessibility/ax_tree.cc
diff --git a/ui/accessibility/ax_tree.cc b/ui/accessibility/ax_tree.cc
index 2d2a535f7e985a8ec5c0814b2628a2a05818cd77..96f14a41e30d64cb1c1dcbff4185b836316f8919 100644
--- a/ui/accessibility/ax_tree.cc
+++ b/ui/accessibility/ax_tree.cc
@@ -24,6 +24,62 @@ std::string TreeToStringHelper(AXNode* node, int indent) {
   return result;
 }
 
+template <typename K, typename V>
+bool KeyValuePairsKeysMatch(std::vector<std::pair<K, V>> pairs1,
+                            std::vector<std::pair<K, V>> pairs2) {
+  if (pairs1.size() != pairs2.size())
+    return false;
+  for (size_t i = 0; i < pairs1.size(); ++i) {
+    if (pairs1[i].first != pairs2[i].first)
+      return false;
+  }
+  return true;
+}
+
+template <typename K, typename V>
+std::map<K, V> MapFromKeyValuePairs(std::vector<std::pair<K, V>> pairs) {
+  std::map<K, V> result;
+  for (size_t i = 0; i < pairs.size(); ++i)
+    result[pairs[i].first] = pairs[i].second;
+  return result;
+}
+
+// Given two vectors of <K, V> key, value pairs representing an "old" vs "new"
+// state, or "before" vs "after", calls a callback function for each key that
+// changed value.
+template <typename K, typename V, typename F>

[dmazzoni, 2017/03/10 23:40:26]

The typename F here is the only part I'm not thrilled about.
I couldn't figure out the right signature for F since
because a lamba that captures is not interchangeable with
a function pointer. I could probably use std::function, but
I'm not sure that would compile away.

Apparently using <typename F> when you want to pass a
lambda is a pretty common trick - just let the compiler
figure out what the type is.

[aboxhall, 2017/03/13 03:29:29]

I had a play with this because I wanted to understand everything going on here!

I don't understand what you mean about the lambda "compiling away". Do lambdas
have some kind of "pretend I'm not here" magic?

Otherwise, yeah, the typename F is a bit weird but I think it's ok.

[dmazzoni, 2017/03/13 16:32:37]

No, but presumably the overhead of a lambda is not much
more than any other function call, whereas std::function is
a whole object and comes with more overhead.
Sounds good

[aboxhall, 2017/03/13 20:43:22]

Oh! In that case would base::BindRepeating() have worked? I had a play with that
yesterday. You would have to make some local wrapper functions to get a
consistent type signature, but then you could get rid of the F magic. Not really
a clear win.

+void CallIfAttributeValuesChanged(const std::vector<std::pair<K, V>>& pairs1,
+                                  const std::vector<std::pair<K, V>>& pairs2,
+                                  const V& empty_value,
+                                  F callback) {
+  // void (*callback)(K, const V&, const V&)) {

[aboxhall, 2017/03/13 03:29:29]

Stray commented out code :)

[dmazzoni, 2017/03/13 16:32:37]

Done.

+  if (KeyValuePairsKeysMatch(pairs1, pairs2)) {
+    // Fast path - if they both have the same keys in the same order.
+    for (size_t i = 0; i < pairs1.size(); ++i) {
+      if (pairs1[i].second != pairs2[i].second)
+        callback(pairs1[i].first, pairs1[i].second, pairs2[i].second);
+    }
+  } else {

[aboxhall, 2017/03/13 03:29:29]

Early out instead of else {} ?

[dmazzoni, 2017/03/13 16:32:37]

Good idea! Helps readability.

+    // Slower path - they don't have the same keys in the same order, so
+    // check all keys against each other, using maps to prevent this from
+    // becoming O(n^2) as the size grows.
+    auto map1 = MapFromKeyValuePairs(pairs1);
+    auto map2 = MapFromKeyValuePairs(pairs2);
+    for (size_t i = 0; i < pairs1.size(); ++i) {
+      const auto& new_iter = map2.find(pairs1[i].first);
+      if (pairs1[i].second != empty_value && new_iter == map2.end())
+        callback(pairs1[i].first, pairs1[i].second, empty_value);
+    }
+    for (size_t i = 0; i < pairs2.size(); ++i) {
+      const auto& iter = map1.find(pairs2[i].first);
+      if (iter == map1.end())
+        callback(pairs2[i].first, empty_value, pairs2[i].second);
+      else if (iter->second != pairs2[i].second)
+        callback(pairs2[i].first, iter->second, pairs2[i].second);
+    }
+  }
+}
+
 }  // namespace
 
 // Intermediate state to keep track of during a tree update.
@@ -228,9 +284,8 @@ bool AXTree::UpdateNode(const AXNodeData& src,
   AXNode* node = GetFromId(src.id);
   if (node) {
     update_state->pending_nodes.erase(node);
-    if (delegate_ &&
-        update_state->new_nodes.find(node) == update_state->new_nodes.end())
-      delegate_->OnNodeDataWillChange(this, node->data(), src);
+    if (update_state->new_nodes.find(node) == update_state->new_nodes.end())
+      CallNodeChangeCallbacks(node, src);
     node->SetData(src);
   } else {
     if (!is_new_root) {
@@ -291,6 +346,67 @@ bool AXTree::UpdateNode(const AXNodeData& src,
   return success;
 }
 
+void AXTree::CallNodeChangeCallbacks(AXNode* node, const AXNodeData& new_data) {
+  if (!delegate_)
+    return;
+
+  const AXNodeData& old_data = node->data();
+  delegate_->OnNodeDataWillChange(this, old_data, new_data);
+
+  if (old_data.role != new_data.role)
+    delegate_->OnRoleChanged(this, node, old_data.role, new_data.role);
+
+  if (old_data.state != new_data.state) {
+    for (int i = AX_STATE_NONE + 1; i <= AX_STATE_LAST; ++i) {
+      AXState state = static_cast<AXState>(i);
+      if (old_data.HasStateFlag(state) != new_data.HasStateFlag(state)) {
+        delegate_->OnStateChanged(this, node, state,
+                                  new_data.HasStateFlag(state));
+      }
+    }
+  }
+
+  CallIfAttributeValuesChanged(
+      old_data.string_attributes, new_data.string_attributes, std::string(),
+      [this, node](AXStringAttribute attr, const std::string& old_string,

[aboxhall, 2017/03/13 03:29:29]

Maybe pull the lambda out into a local variable for clarity? It's kind of a lot
going on otherwise.

[dmazzoni, 2017/03/13 16:32:37]

Done.

+                   const std::string& new_string) {
+        delegate_->OnStringAttributeChanged(this, node, attr, old_string,
+                                            new_string);
+      });
+
+  CallIfAttributeValuesChanged(
+      old_data.bool_attributes, new_data.bool_attributes, false,
+      [this, node](AXBoolAttribute attr, const bool& old_bool,
+                   const bool& new_bool) {
+        delegate_->OnBoolAttributeChanged(this, node, attr, new_bool);
+      });
+
+  CallIfAttributeValuesChanged(
+      old_data.float_attributes, new_data.float_attributes, 0.0f,
+      [this, node](AXFloatAttribute attr, const float& old_float,
+                   const float& new_float) {
+        delegate_->OnFloatAttributeChanged(this, node, attr, old_float,
+                                           new_float);
+      });
+
+  CallIfAttributeValuesChanged(
+      old_data.int_attributes, new_data.int_attributes, 0,
+      [this, node](AXIntAttribute attr, const int& old_int,
+                   const int& new_int) {
+        delegate_->OnIntAttributeChanged(this, node, attr, old_int, new_int);
+      });
+
+  CallIfAttributeValuesChanged(
+      old_data.intlist_attributes, new_data.intlist_attributes,
+      std::vector<int32_t>(),
+      [this, node](AXIntListAttribute attr,
+                   const std::vector<int32_t>& old_intlist,
+                   const std::vector<int32_t>& new_intlist) {
+        delegate_->OnIntListAttributeChanged(this, node, attr, old_intlist,
+                                             new_intlist);
+      });
+}
+
 void AXTree::DestroySubtree(AXNode* node,
                             AXTreeUpdateState* update_state) {
   DCHECK(update_state);