Expand Chrome OS ARC support to create one tree source per package

chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.h"
 
 #include "base/command_line.h"
 #include "chrome/browser/chromeos/accessibility/accessibility_manager.h"
-#include "chrome/browser/chromeos/arc/accessibility/ax_tree_source_arc.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chromeos/chromeos_switches.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/exo/shell_surface.h"
 #include "components/exo/surface.h"
 #include "ui/aura/client/aura_constants.h"
 #include "ui/aura/window.h"
 #include "ui/gfx/geometry/rect.h"
-#include "ui/views/view.h"
-#include "ui/views/widget/widget.h"
 
 namespace {
 
-// This class keeps focus on a |ShellSurface| without interfering with default
-// focus management in |ShellSurface|. For example, touch causes the
-// |ShellSurface| to lose focus to its ancestor containing View.
-class FocusStealer : public views::View {
- public:
-  explicit FocusStealer(int32_t id) : id_(id) {
-    SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
-    set_owned_by_client();
-  }
-
-  // views::View overrides.
-  void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
-    node_data->AddIntAttribute(ui::AX_ATTR_CHILD_TREE_ID, id_);
-    node_data->role = ui::AX_ROLE_CLIENT;
-  }
-
- private:
-  int32_t id_;
-  DISALLOW_COPY_AND_ASSIGN(FocusStealer);
-};
+static int kNoTaskId = -1;

[Luis Héctor Chávez, 2017/04/26 15:34:59]

nit: constexpr int32_t

having something be static within the anonymous namespace is superfluous.

[David Tseng, 2017/04/28 00:27:44]

Done.

 
 exo::Surface* GetArcSurface(const aura::Window* window) {
   if (!window)
     return nullptr;
 
   exo::Surface* arc_surface = exo::Surface::AsSurface(window);
   if (!arc_surface)
     arc_surface = exo::ShellSurface::GetMainSurface(window);
   return arc_surface;
 }
 
+int32_t GetTaskId(aura::Window* window) {
+  const std::string arc_app_id = exo::ShellSurface::GetApplicationId(window);
+  if (arc_app_id.empty())
+    return kNoTaskId;
+
+  int task_id = kNoTaskId;

[Luis Héctor Chávez, 2017/04/26 15:34:59]

nit: int32_t

[David Tseng, 2017/04/28 00:27:43]

Done.

+  if (sscanf(arc_app_id.c_str(), "org.chromium.arc.%d", &task_id) != 1)
+    return kNoTaskId;
+
+  return task_id;
+}
+
 void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data) {
   chromeos::AccessibilityManager* accessibility_manager =
       chromeos::AccessibilityManager::Get();
   if (!accessibility_manager)
     return;
 
   exo::WMHelper* wmHelper = exo::WMHelper::GetInstance();
   aura::Window* focused_window = wmHelper->GetFocusedWindow();
   if (!focused_window)
     return;
 
   aura::Window* toplevel_window = focused_window->GetToplevelWindow();
 
   gfx::Rect bounds_in_screen = gfx::ScaleToEnclosingRect(
       node_data->boundsInScreen,
       1.0f / toplevel_window->layer()->device_scale_factor());
 
   accessibility_manager->OnViewFocusedInArc(bounds_in_screen);
 }
 
+arc::mojom::AccessibilityFilterType GetFilterType() {
+  chromeos::AccessibilityManager* accessibility_manager =
+      chromeos::AccessibilityManager::Get();
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          chromeos::switches::kEnableChromeVoxArcSupport)) {
+    return arc::mojom::AccessibilityFilterType::ALL;
+  }
+
+  if (!accessibility_manager)
+    return arc::mojom::AccessibilityFilterType::OFF;
+
+  if (accessibility_manager->IsSpokenFeedbackEnabled())
+    return arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME;
+
+  if (accessibility_manager->IsFocusHighlightEnabled())
+    return arc::mojom::AccessibilityFilterType::FOCUS;
+
+  return arc::mojom::AccessibilityFilterType::OFF;
+}
+
 }  // namespace
 
 namespace arc {
 
 ArcAccessibilityHelperBridge::ArcAccessibilityHelperBridge(
     ArcBridgeService* bridge_service)
-    : ArcService(bridge_service), binding_(this) {
+    : ArcService(bridge_service), binding_(this), current_task_id_(0) {

[yawano, 2017/04/27 10:01:27]

nit: current_task_id_(kNoTaskId) instead of 0?

[David Tseng, 2017/04/28 00:27:43]

Done.

   arc_bridge_service()->accessibility_helper()->AddObserver(this);
 }
 
 ArcAccessibilityHelperBridge::~ArcAccessibilityHelperBridge() {
   arc_bridge_service()->accessibility_helper()->RemoveObserver(this);
+  ArcAppListPrefs::Get(chromeos::AccessibilityManager::Get()->profile())
+      ->RemoveObserver(this);
 }
 
 void ArcAccessibilityHelperBridge::OnInstanceReady() {
+  ArcAppListPrefs::Get(chromeos::AccessibilityManager::Get()->profile())
+      ->AddObserver(this);
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
       arc_bridge_service()->accessibility_helper(), Init);
   DCHECK(instance);
   instance->Init(binding_.CreateInterfacePtrAndBind());
 
-  chromeos::AccessibilityManager* accessibility_manager =
-      chromeos::AccessibilityManager::Get();
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          chromeos::switches::kEnableChromeVoxArcSupport)) {
-    instance->SetFilter(arc::mojom::AccessibilityFilterType::ALL);
-    if (!tree_source_) {
-      tree_source_.reset(new AXTreeSourceArc(tree_id()));
-      focus_stealer_.reset(new FocusStealer(tree_source_->tree_id()));
-      exo::WMHelper::GetInstance()->AddActivationObserver(this);
-    }
-  } else if (accessibility_manager &&
-             accessibility_manager->IsFocusHighlightEnabled()) {
-    instance->SetFilter(arc::mojom::AccessibilityFilterType::FOCUS);
-  }
+  arc::mojom::AccessibilityFilterType filter_type = GetFilterType();
+  instance->SetFilter(filter_type);
+
+  if (filter_type == arc::mojom::AccessibilityFilterType::ALL ||
+      filter_type ==
+          arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME)

[hidehiko, 2017/04/26 17:13:55]

missing brace, for consistency with L67?

[David Tseng, 2017/04/28 00:27:43]

Done.

+    exo::WMHelper::GetInstance()->AddActivationObserver(this);

[yawano, 2017/04/27 10:01:27]

Remove this from activation observer in the destructor of
ArcAccessibilityHelperBridge.

[David Tseng, 2017/04/28 00:27:43]

I don't think we can do this without potentially hitting a NOTREACHED. We would
need to restructure the filter type to be updated rhater than computed.

[yawano, 2017/04/28 11:17:52]

I might be missing something, but I think RemoveObserver checks whether the list
contains it or not. It should be no problem to call RemoveObserver even for the
case where we haven't added ArcAccessibilityHelperBridge to it.

https://cs.chromium.org/chromium/src/base/observer_list.h?l=283

[David Tseng, 2017/04/28 15:21:17]

No, you're right. I was thinking of the case where the same observer was added
more than once.
We're not so strict on removals.

Done.

 }
 
 void ArcAccessibilityHelperBridge::OnAccessibilityEventDeprecated(
     mojom::AccessibilityEventType event_type,
     mojom::AccessibilityNodeInfoDataPtr event_source) {
   if (event_type == arc::mojom::AccessibilityEventType::VIEW_FOCUSED)
     DispatchFocusChange(event_source.get());
 }
 
 void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
     mojom::AccessibilityEventDataPtr event_data) {
-  if (tree_source_) {
-    tree_source_->NotifyAccessibilityEvent(event_data.get());
+  arc::mojom::AccessibilityFilterType filter_type = GetFilterType();
+
+  if (filter_type == arc::mojom::AccessibilityFilterType::ALL ||
+      filter_type ==
+          arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME) {
+    // Get the task id for this package.
+    if (event_data->nodeData.size() == 0)
+      return;
+
+    arc::mojom::AccessibilityNodeInfoData* node = event_data->nodeData[0].get();
+    if (!node->stringProperties)
+      return;
+
+    auto package_it = node->stringProperties->find(
+        arc::mojom::AccessibilityStringProperty::PACKAGE_NAME);
+    if (package_it == node->stringProperties->end())
+      return;
+
+    auto task_ids_it = package_name_to_task_ids_.find(package_it->second);
+    if (task_ids_it == package_name_to_task_ids_.end())
+      return;
+
+    auto task_ids = task_ids_it->second;

[hidehiko, 2017/04/26 17:13:55]

this copies whole set, which looks unexpected.
maybe "const auto& task_ids = ..."

[David Tseng, 2017/04/28 00:27:43]

Done.

+
+    // Reject updates to non-current task ids. We can do this currently
+    // because all events include the entire tree.
+    if (task_ids.count(current_task_id_) == 0)
+      return;
+
+    auto tree_it = package_name_to_tree_.find(package_it->second);
+    AXTreeSourceArc* tree_source;
+    if (tree_it == package_name_to_tree_.end()) {
+      package_name_to_tree_[package_it->second].reset(
+          new AXTreeSourceArc(this));
+      tree_source = package_name_to_tree_[package_it->second].get();
+    } else {
+      tree_source = tree_it->second.get();
+    }
+    tree_source->NotifyAccessibilityEvent(event_data.get());
     return;
   }
 
   if (event_data->eventType != arc::mojom::AccessibilityEventType::VIEW_FOCUSED)
     return;
 
   CHECK_EQ(1U, event_data.get()->nodeData.size());
   DispatchFocusChange(event_data.get()->nodeData[0].get());
 }
 
-void ArcAccessibilityHelperBridge::OnWindowActivated(
-    aura::Window* gained_active,
-    aura::Window* lost_active) {
-  if (gained_active == lost_active || !tree_source_)
-    return;
-
-  exo::Surface* active_surface = GetArcSurface(gained_active);
-  exo::Surface* inactive_surface = GetArcSurface(lost_active);
-
-  // Detach the accessibility tree from an inactive ShellSurface so that any
-  // client walking the desktop tree gets non-duplicated linearization.
-  if (inactive_surface) {
-    views::Widget* widget = views::Widget::GetWidgetForNativeView(lost_active);
-    if (widget && widget->GetContentsView()) {
-      views::View* view = widget->GetContentsView();
-      view->RemoveChildView(focus_stealer_.get());
-      view->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
-    }
-  }
-
-  if (!active_surface)
-    return;
-
-  views::Widget* widget = views::Widget::GetWidgetForNativeView(gained_active);
-  if (widget && widget->GetContentsView()) {
-    views::View* view = widget->GetContentsView();
-    if (!view->Contains(focus_stealer_.get()))
-      view->AddChildView(focus_stealer_.get());
-    focus_stealer_->RequestFocus();
-    view->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
-  }
-}
-
-void ArcAccessibilityHelperBridge::PerformAction(const ui::AXActionData& data) {
+void ArcAccessibilityHelperBridge::OnAction(
+    const ui::AXActionData& data) const {
   arc::mojom::AccessibilityActionType mojo_action;
   switch (data.action) {
     case ui::AX_ACTION_DO_DEFAULT:
       mojo_action = arc::mojom::AccessibilityActionType::CLICK;
       break;
     default:
       return;
   }
 
   auto* instance = ARC_GET_INSTANCE_FOR_METHOD(
       arc_bridge_service()->accessibility_helper(), PerformAction);
   instance->PerformAction(data.target_node_id, mojo_action);
 }
 
+void ArcAccessibilityHelperBridge::OnWindowActivated(
+    aura::Window* gained_active,
+    aura::Window* lost_active) {
+  if (gained_active == lost_active)
+    return;
+
+  if (!GetArcSurface(gained_active))
+    return;
+
+  // Grab the tree source associated with this app.
+  int32_t task_id = GetTaskId(gained_active);
+  std::string package_name;
+  for (auto task_ids_it : package_name_to_task_ids_) {

[hidehiko, 2017/04/26 17:13:55]

This copies entry (pair of key/value) for each element.
"const auto&"

Also, "_it" suffix implies iterator, but this is actually not. Maybe
package_name_to_task_ids_entry ? Or just entry may be fine, here, considering
its scope.

[David Tseng, 2017/04/28 00:27:43]

Done.

+    if (task_ids_it.second.count(task_id) != 0) {
+      package_name = task_ids_it.first;
+      break;
+    }
+  }
+
+  if (package_name.empty())
+    return;
+
+  auto it = package_name_to_tree_.find(package_name);
+  if (it != package_name_to_tree_.end())
+    it->second->Focus(gained_active);
+}
+
+void ArcAccessibilityHelperBridge::OnTaskCreated(
+    int task_id,
+    const std::string& package_name,
+    const std::string& activity,
+    const std::string& intent) {
+  package_name_to_task_ids_[package_name].insert(task_id);
+}
+
+void ArcAccessibilityHelperBridge::OnTaskDestroyed(int task_id) {
+  std::string package_name;
+  for (auto task_ids_it : package_name_to_task_ids_) {

[hidehiko, 2017/04/26 17:13:55]

Ditto.

[David Tseng, 2017/04/28 00:27:43]

Yes, partly. Definitely not an iterator, but cannot be const since I'm erasing
below. Made into a ref.

+    if (task_ids_it.second.count(task_id)) {
+      package_name = task_ids_it.first;
+      task_ids_it.second.erase(task_id);
+
+      if (task_ids_it.second.size() == 0)
+        package_name_to_task_ids_.erase(task_ids_it.first);

[hidehiko, 2017/04/26 17:13:55]

erase can take iterator.

[David Tseng, 2017/04/28 00:27:43]

Done.

+
+      break;
+    }
+  }
+
+  if (package_name.empty())
+    return;
+
+  auto it = package_name_to_tree_.find(package_name);

[hidehiko, 2017/04/26 17:13:55]

package_name_to_tree_.erase(package_name);

does what you want.
You do not need manually reset() the value, because its dtor has the
responsibility to delete the instance if necessary.

[David Tseng, 2017/04/28 00:27:43]

True and removed though the subtle issue here was to only delete the tree source
when there are no more task ids for a package. Imo, not making deletions more
explicit here obscures the readability of the code, but I will adhere to the
conventions within arc.

[hidehiko, 2017/05/01 05:15:26]

Which convention do you mean...? Anyway, commented on the latest PS. Could you
check it, instead?

+  if (it != package_name_to_tree_.end()) {
+    it->second.reset();
+    package_name_to_tree_.erase(it);
+  }
+}
+
+void ArcAccessibilityHelperBridge::OnTaskSetActive(int32_t task_id) {
+  current_task_id_ = task_id;
+}
+
 }  // namespace arc