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

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

Index: chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
diff --git a/chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc b/chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
index 908e8737ed31b5d7d29ff1076bbef939e58ea61d..59916bc4a0cb815f538bf1f2cf0d7d2efe434f27 100644
--- a/chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
+++ b/chrome/browser/chromeos/arc/accessibility/arc_accessibility_helper_bridge.cc
@@ -25,10 +25,20 @@ namespace {
 class FocusStealer : public views::View {
  public:
   explicit FocusStealer(int32_t id) : id_(id) {
-    SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
     set_owned_by_client();
   }
 
+  void StartStealing() {
+    SetFocusBehavior(views::View::FocusBehavior::ALWAYS);
+    RequestFocus();
+    parent()->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
+  }
+
+  void StopStealing() {
+    SetFocusBehavior(views::View::FocusBehavior::NEVER);
+    parent()->NotifyAccessibilityEvent(ui::AX_EVENT_CHILDREN_CHANGED, false);
+  }
+
   // views::View overrides.
   void GetAccessibleNodeData(ui::AXNodeData* node_data) override {
     node_data->AddIntAttribute(ui::AX_ATTR_CHILD_TREE_ID, id_);
@@ -70,6 +80,25 @@ void DispatchFocusChange(arc::mojom::AccessibilityNodeInfoData* node_data) {
   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))

[Luis Héctor Chávez, 2017/04/21 15:20:07]

nit: you cannot elide braces on this block since the condition spans multiple
lines.

[David Tseng, 2017/04/25 18:29:48]

Done (though note that this seems somewhat ambiguous since you see both styles
in the codebase).

+    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 {
@@ -90,19 +119,21 @@ void ArcAccessibilityHelperBridge::OnInstanceReady() {
   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);
+
+  switch (filter_type) {
+    case arc::mojom::AccessibilityFilterType::ALL:

[Luis Héctor Chávez, 2017/04/21 15:20:07]

Consider adding

// fallthrough

on the cases where you want to fall through.

[David Tseng, 2017/04/25 18:29:48]

Acknowledged.

+    case arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME:
+      if (!tree_source_) {
+        tree_source_.reset(new AXTreeSourceArc(tree_id()));
+        focus_stealer_.reset(new FocusStealer(tree_source_->tree_id()));
+        exo::WMHelper::GetInstance()->AddActivationObserver(this);
+      }
+      break;
+    case arc::mojom::AccessibilityFilterType::FOCUS:
+    case arc::mojom::AccessibilityFilterType::OFF:
+      break;

[hidehiko, 2017/04/21 06:27:02]

nit: how about commenting
// Do nothing.

to explicitly say this is intentional.

[David Tseng, 2017/04/25 18:29:48]

Acknowledged.

   }
 }
 
@@ -115,6 +146,15 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEventDeprecated(
 
 void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
     mojom::AccessibilityEventDataPtr event_data) {
+  arc::mojom::AccessibilityFilterType filter_type = GetFilterType();
+
+  if (!tree_source_ &&

[yawano, 2017/04/21 04:26:36]

How about to write this function as

switch (filter_type) {
  case ALL:
  case WHITELISTED_PACKAGE_NAME:
    // Reset tree_source_
    // NotifyAccessibilityEvent
    break;
  case FOCUS:
    // DispatchFocusChange
}

?

[David Tseng, 2017/04/25 18:29:48]

Acknowledged.

+      (filter_type == arc::mojom::AccessibilityFilterType::ALL ||
+       filter_type ==
+           arc::mojom::AccessibilityFilterType::WHITELISTED_PACKAGE_NAME)) {
+    tree_source_.reset(new AXTreeSourceArc(tree_id()));
+  }
+
   if (tree_source_) {
     tree_source_->NotifyAccessibilityEvent(event_data.get());
     return;
@@ -127,10 +167,16 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
   DispatchFocusChange(event_data.get()->nodeData[0].get());
 }
 
+void ArcAccessibilityHelperBridge::OnAccessibilityTreeDestroyed() {
+  tree_source_.reset(nullptr);

[hidehiko, 2017/04/21 06:27:02]

nit: s/reset(nullptr)/reset()/

[David Tseng, 2017/04/25 18:29:48]

Acknowledged.

+  if (focus_stealer_->parent())
+    focus_stealer_->parent()->RemoveChildView(focus_stealer_.get());
+}
+
 void ArcAccessibilityHelperBridge::OnWindowActivated(
     aura::Window* gained_active,
     aura::Window* lost_active) {
-  if (gained_active == lost_active || !tree_source_)
+  if (gained_active == lost_active)
     return;
 
   exo::Surface* active_surface = GetArcSurface(gained_active);
@@ -142,8 +188,7 @@ void ArcAccessibilityHelperBridge::OnWindowActivated(
     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);
+      focus_stealer_->StopStealing();
     }
   }
 
@@ -155,8 +200,7 @@ void ArcAccessibilityHelperBridge::OnWindowActivated(
     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);
+    focus_stealer_->StartStealing();
   }
 }