Fix computation of Automation API location offsets in WebViews.

chrome/renderer/extensions/automation_internal_custom_bindings.cc

Index: chrome/renderer/extensions/automation_internal_custom_bindings.cc
diff --git a/chrome/renderer/extensions/automation_internal_custom_bindings.cc b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
index e56944b1f4928389aae3fb68e502e1dc620804e8..9cba94914646b0fde747577e1842af2f740fa0d6 100644
--- a/chrome/renderer/extensions/automation_internal_custom_bindings.cc
+++ b/chrome/renderer/extensions/automation_internal_custom_bindings.cc
@@ -538,7 +538,6 @@ AutomationInternalCustomBindings::AutomationInternalCustomBindings(
         gfx::RectF local_bounds = ComputeLocalNodeBounds(cache, node);
         gfx::Rect global_bounds =
             ComputeGlobalNodeBounds(cache, node, local_bounds);
-        global_bounds.Offset(cache->location_offset);
         result.Set(RectToV8Object(isolate, global_bounds));
       });
   RouteNodeIDFunction(
@@ -578,7 +577,6 @@ AutomationInternalCustomBindings::AutomationInternalCustomBindings(
         if (node->data().role != ui::AX_ROLE_INLINE_TEXT_BOX) {
           gfx::Rect global_bounds =
               ComputeGlobalNodeBounds(cache, node, local_bounds);
-          global_bounds.Offset(cache->location_offset);
           result.Set(RectToV8Object(isolate, global_bounds));
         }
 
@@ -620,7 +618,6 @@ AutomationInternalCustomBindings::AutomationInternalCustomBindings(
         // transformations.
         gfx::Rect global_bounds =
             ComputeGlobalNodeBounds(cache, node, local_bounds);
-        global_bounds.Offset(cache->location_offset);
         result.Set(RectToV8Object(isolate, global_bounds));
       });
 
@@ -1014,8 +1011,12 @@ ui::AXNode* AutomationInternalCustomBindings::GetParent(
     return node->parent();
 
   int parent_tree_id = (*in_out_cache)->tree.data().parent_tree_id;

[David Tseng, 2017/03/23 17:58:29]

Can you check the js bindings? This code doesn't seem relevant given that we
track the hosting node explicitly in the root or perhaps I'm missing something.
If GetParent is supposed to work as written, then we shouldn't have to track the
host node in js, but with the trade off of a non-constant time lookup.

[dmazzoni, 2017/03/23 22:02:33]

Good idea.  Honestly I'd far prefer if JS didn't have to keep track
of the host node. I'd like to keep the JS bindings much simpler. Note
that the lookup is supposed to be non-constant only once, then we cache
it. So the performance difference should be negligible.

I uploaded that follow-up here:
https://codereview.chromium.org/2770713008

OK to go ahead with this change as-is and then work on that follow-up?

[David Tseng, 2017/03/24 21:49:06]

Yes this change is fine, but I don't really feel comfortable with this code
since it clearly never got hit before. As far as performance goes, you can do
better if you just ported the same ideas over from js (see the above cl). Please
don't dismiss the work that went into designing the js side of things.

+
+  // Try the desktop tree if the parent is unknown. If this tree really is
+  // a child of the desktop tree, we'll find its parent, and if not, the
+  // search, below, will fail until the real parent tree loads.
   if (parent_tree_id < 0)
-    return nullptr;
+    parent_tree_id = api::automation::kDesktopTreeID;
 
   TreeCache* parent_cache = GetTreeCacheFromTreeID(parent_tree_id);
   if (!parent_cache)
@@ -1023,9 +1024,9 @@ ui::AXNode* AutomationInternalCustomBindings::GetParent(
 
   // Try to use the cached parent node from the most recent time this
   // was called.
-  if (parent_cache->parent_node_id_from_parent_tree > 0) {
+  if ((*in_out_cache)->parent_node_id_from_parent_tree > 0) {
     ui::AXNode* parent = parent_cache->tree.GetFromId(
-        parent_cache->parent_node_id_from_parent_tree);
+        (*in_out_cache)->parent_node_id_from_parent_tree);
     if (parent) {
       int parent_child_tree_id =
           parent->data().GetIntAttribute(ui::AX_ATTR_CHILD_TREE_ID);
@@ -1133,7 +1134,6 @@ void AutomationInternalCustomBindings::OnAccessibilityEvent(
   }
 
   // Update the internal state whether it's the active profile or not.
-  cache->location_offset = params.location_offset;
   deleted_node_ids_.clear();
   v8::Isolate* isolate = GetIsolate();
   v8::HandleScope handle_scope(isolate);