Remove backward compat code from ArcAccessibilityHelperBridge

- Interface change has happend at https://crrev.com/c/844052 around the
  end of February. It should be fine to remove backward compat code now.

Bug: 768439
Test: unit_tests:ArcAccessibilityHelperBridgeTest
Change-Id: Ibac321f3889d605393928839b5292e64d27b0094
Reviewed-on: https://chromium-review.googlesource.com/1039405Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Commit-Queue: Yuki Awano <yawano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558605}

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

@@ -245,23 +245,8 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
 
       // This bridge must receive OnNotificationStateChanged call for the
       // notification_key before this receives an accessibility event for it.
-      // TODO(yawano): change this to DCHECK once we remove backward
-      //               compatibility logic.
-      if (notification_keys_.find(notification_key) !=
-          notification_keys_.end()) {
-        tree_source = GetFromNotificationKey(notification_key);
-        DCHECK(tree_source);
-      } else {
-        // Backward compatibility logic.
-        // TODO(yawano): remove this once this becomes unnecessary.
-        if (event_data->event_type ==
-            arc::mojom::AccessibilityEventType::WINDOW_STATE_CHANGED) {
-          tree_source = CreateFromNotificationKey(notification_key);
-          backward_compat_notification_keys_[notification_key]++;
-        } else {
-          tree_source = GetFromNotificationKey(notification_key);
-        }
-      }
+      tree_source = GetFromNotificationKey(notification_key);
+      DCHECK(tree_source);
     } else {
       if (event_data->task_id == kNoTaskId)
         return;
@@ -283,40 +268,21 @@ void ArcAccessibilityHelperBridge::OnAccessibilityEvent(
 
     tree_source->NotifyAccessibilityEvent(event_data.get());
 
-    if (is_notification_event) {
-      switch (event_data->event_type) {
-        case arc::mojom::AccessibilityEventType::VIEW_TEXT_SELECTION_CHANGED: {
-          // If text selection changed event is dispatched from Android, it
-          // means that user is trying to type a text in Android notification.
-          // Dispatch text selection changed event to notification content view
-          // as the view can take necessary actions, e.g. activate itself, etc.
-          auto* surface_manager = ArcNotificationSurfaceManager::Get();
-          if (!surface_manager)
-            break;
-
-          ArcNotificationSurface* surface = surface_manager->GetArcSurface(
-              event_data->notification_key.value());
-          if (!surface)
-            break;
-
+    if (is_notification_event &&
+        event_data->event_type ==
+            arc::mojom::AccessibilityEventType::VIEW_TEXT_SELECTION_CHANGED) {
+      // If text selection changed event is dispatched from Android, it
+      // means that user is trying to type a text in Android notification.
+      // Dispatch text selection changed event to notification content view
+      // as the view can take necessary actions, e.g. activate itself, etc.
+      auto* surface_manager = ArcNotificationSurfaceManager::Get();
+      if (surface_manager) {
+        ArcNotificationSurface* surface = surface_manager->GetArcSurface(
+            event_data->notification_key.value());
+        if (surface) {
           surface->GetAttachedHost()->NotifyAccessibilityEvent(
               ax::mojom::Event::kTextSelectionChanged, true);
-          break;
-        }
-        case arc::mojom::AccessibilityEventType::WINDOW_STATE_CHANGED: {
-          // TODO(yawano): Move this to OnNotificationStateChanged. This can be
-          // moved there if we don't need to care about backward compat.
-          ui::AXTreeData tree_data;
-          if (!tree_source->GetTreeData(&tree_data))
-            break;
-
-          DCHECK(event_data->notification_key.has_value());
-          UpdateTreeIdOfNotificationSurface(
-              event_data->notification_key.value(), tree_data.tree_id);
-          break;
         }
-        default:
-          break;
       }
     }
 
@@ -335,12 +301,19 @@ void ArcAccessibilityHelperBridge::OnNotificationStateChanged(
     const std::string& notification_key,
     arc::mojom::AccessibilityNotificationStateType state) {
   switch (state) {
-    case arc::mojom::AccessibilityNotificationStateType::SURFACE_CREATED:
-      CreateFromNotificationKey(notification_key);
-      notification_keys_.insert(notification_key);
+    case arc::mojom::AccessibilityNotificationStateType::SURFACE_CREATED: {
+      AXTreeSourceArc* tree_source =
+          CreateFromNotificationKey(notification_key);
+
+      ui::AXTreeData tree_data;
+      if (!tree_source->GetTreeData(&tree_data)) {
+        NOTREACHED();
+        return;
+      }
+      UpdateTreeIdOfNotificationSurface(notification_key, tree_data.tree_id);
       break;
+    }
     case arc::mojom::AccessibilityNotificationStateType::SURFACE_REMOVED:
-      notification_keys_.erase(notification_key);
       notification_key_to_tree_.erase(notification_key);
       UpdateTreeIdOfNotificationSurface(notification_key, kInvalidTreeId);
       break;
@@ -599,22 +572,4 @@ void ArcAccessibilityHelperBridge::OnNotificationSurfaceAdded(
   }
 }
 
-void ArcAccessibilityHelperBridge::OnNotificationSurfaceRemoved(
-    ArcNotificationSurface* surface) {
-  const std::string& notification_key = surface->GetNotificationKey();
-
-  auto it = backward_compat_notification_keys_.find(notification_key);
-  if (it == backward_compat_notification_keys_.end())
-    return;
-
-  it->second--;
-
-  DCHECK(it->second >= 0);
-
-  if (it->second == 0) {
-    notification_key_to_tree_.erase(notification_key);
-    backward_compat_notification_keys_.erase(notification_key);
-  }
-}
-
 }  // namespace arc

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

@@ -83,7 +83,7 @@ class ArcAccessibilityHelperBridge
 
   // ArcNotificationSurfaceManager::Observer overrides.
   void OnNotificationSurfaceAdded(ArcNotificationSurface* surface) override;
-  void OnNotificationSurfaceRemoved(ArcNotificationSurface* surface) override;
+  void OnNotificationSurfaceRemoved(ArcNotificationSurface* surface) override{};
 
   const std::map<int32_t, std::unique_ptr<AXTreeSourceArc>>&
   task_id_to_tree_for_test() const {
@@ -128,16 +128,6 @@ class ArcAccessibilityHelperBridge
   std::unique_ptr<chromeos::AccessibilityStatusSubscription>
       accessibility_status_subscription_;
 
-  // Map for managing notifications in backward compatible way, creating
-  // notification with WINDOW_STATE_CHANGED event.
-  // Key: notification key
-  // Value: retain counter
-  // TODO(yawano): Remove this after this becomes unnecessary.
-  std::map<std::string, int32_t> backward_compat_notification_keys_;
-
-  // TODO(yawano): Remove this after this becomes unnecessary.
-  std::set<std::string> notification_keys_;
-
   DISALLOW_COPY_AND_ASSIGN(ArcAccessibilityHelperBridge);
 };
 

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

@@ -404,107 +404,6 @@ TEST_F(ArcAccessibilityHelperBridgeTest, NotificationEventArriveFirst) {
   arc_notification_surface_manager_->RemoveSurface(&test_surface_2);
 }
 
-// This is the case for testing backward compatibility with creating
-// notification by WINDOW_STATE_CHANGED event.
-//
-// mojo: notification 1 created
-// wayland: surface 1 created
-// mojo: notification 2 created
-// wayland: surface 1 removed
-// wayland: surface 2 created
-// wayland: surface 2 removed
-//
-// wayland: surface 3 created
-// mojo: notification 3 created
-// wayland: surface 3 removed
-TEST_F(ArcAccessibilityHelperBridgeTest, NotificationBackwardCompat) {
-  base::CommandLine::ForCurrentProcess()->AppendSwitch(
-      chromeos::switches::kEnableChromeVoxArcSupport);
-
-  TestArcAccessibilityHelperBridge* helper_bridge =
-      accessibility_helper_bridge();
-  arc_notification_surface_manager_->AddObserver(helper_bridge);
-
-  const auto& notification_key_to_tree_ =
-      helper_bridge->notification_key_to_tree_for_test();
-  ASSERT_EQ(0U, notification_key_to_tree_.size());
-
-  // mojo: notification 1 created
-  auto event1 = arc::mojom::AccessibilityEventData::New();
-  event1->event_type = arc::mojom::AccessibilityEventType::WINDOW_STATE_CHANGED;
-  event1->notification_key = base::make_optional<std::string>(kNotificationKey);
-  event1->node_data.push_back(arc::mojom::AccessibilityNodeInfoData::New());
-  helper_bridge->OnAccessibilityEvent(event1.Clone());
-
-  EXPECT_EQ(1U, notification_key_to_tree_.size());
-
-  // wayland: surface 1 added
-  FakeArcNotificationSurface test_surface1(kNotificationKey);
-  arc_notification_surface_manager_->AddSurface(&test_surface1);
-
-  auto it = notification_key_to_tree_.find(kNotificationKey);
-  EXPECT_NE(notification_key_to_tree_.end(), it);
-  AXTreeSourceArc* tree = it->second.get();
-  ui::AXTreeData tree_data;
-  tree->GetTreeData(&tree_data);
-  EXPECT_EQ(tree_data.tree_id, test_surface1.GetAXTreeId());
-
-  // mojo: notification 2 created
-  auto event2 = arc::mojom::AccessibilityEventData::New();
-  event2->event_type = arc::mojom::AccessibilityEventType::WINDOW_STATE_CHANGED;
-  event2->notification_key = base::make_optional<std::string>(kNotificationKey);
-  event2->node_data.push_back(arc::mojom::AccessibilityNodeInfoData::New());
-  helper_bridge->OnAccessibilityEvent(event2.Clone());
-
-  auto it2 = notification_key_to_tree_.find(kNotificationKey);
-  EXPECT_NE(notification_key_to_tree_.end(), it);
-  AXTreeSourceArc* tree2 = it2->second.get();
-  ui::AXTreeData tree_data2;
-  tree2->GetTreeData(&tree_data2);
-  EXPECT_EQ(tree_data2.tree_id, test_surface1.GetAXTreeId());
-
-  // wayland: surface 1 removed
-  arc_notification_surface_manager_->RemoveSurface(&test_surface1);
-
-  EXPECT_EQ(1U, notification_key_to_tree_.size());
-
-  // wayland: surface 2 added
-  FakeArcNotificationSurface test_surface2(kNotificationKey);
-  arc_notification_surface_manager_->AddSurface(&test_surface2);
-
-  EXPECT_EQ(tree_data2.tree_id, test_surface2.GetAXTreeId());
-
-  // wayland: surface 2 removed
-  arc_notification_surface_manager_->RemoveSurface(&test_surface2);
-
-  EXPECT_EQ(0U, notification_key_to_tree_.size());
-
-  // wayland: surface 3 added
-  FakeArcNotificationSurface test_surface3(kNotificationKey);
-  arc_notification_surface_manager_->AddSurface(&test_surface3);
-
-  // mojo: notification 3 created
-  auto event3 = arc::mojom::AccessibilityEventData::New();
-  event3->event_type = arc::mojom::AccessibilityEventType::WINDOW_STATE_CHANGED;
-  event3->notification_key = base::make_optional<std::string>(kNotificationKey);
-  event3->node_data.push_back(arc::mojom::AccessibilityNodeInfoData::New());
-  helper_bridge->OnAccessibilityEvent(event3.Clone());
-
-  EXPECT_EQ(1U, notification_key_to_tree_.size());
-
-  auto it3 = notification_key_to_tree_.find(kNotificationKey);
-  EXPECT_NE(notification_key_to_tree_.end(), it3);
-  AXTreeSourceArc* tree3 = it3->second.get();
-  ui::AXTreeData tree_data3;
-  tree3->GetTreeData(&tree_data3);
-  EXPECT_EQ(tree_data3.tree_id, test_surface3.GetAXTreeId());
-
-  // wayland: surface 3 removed
-  arc_notification_surface_manager_->RemoveSurface(&test_surface3);
-
-  EXPECT_EQ(0U, notification_key_to_tree_.size());
-}
-
 // This is the case where surface creation/removal arrive before mojo events.
 //
 // wayland: surface 1 added