[Extensions c2s] Non-destructively update site access menu items

When an extension is updated, it may change site access and therefore
be on a different site access section.

Previously this is handled in two ways:
- When one site access item changed: update item, and then move between
sections if necessary
- When potentially many site access items changed: remove all site
access items, and insert in the correct section.

We can combine the update site access item and move to the correct
section into one method. This fixes two things:
- Risk of forgetting to "move items between sections if necessary"
after updating a site access item..
- Removing and re-adding all the views is a destructive operation that
can break things like current focus for a11y (crbug.com/1038821)

Bug: 1263310
Change-Id: Icf0f9e250556564f6313ee22731045897e12697c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3512478
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org>
Commit-Queue: Emilia Paz <emiliapaz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#986208}
diff --git a/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.cc b/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.cc
index 85d52d0..58565ba 100644
--- a/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.cc
+++ b/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.cc
@@ -166,29 +166,6 @@
          children.begin();
 }
 
-// Updates the `installed_extension_view` state and its position under
-// `parent_view`.
-void UpdateInstalledExtensionMenuItem(
-    views::View* parent_view,
-    InstalledExtensionMenuItemView* installed_extension_view) {
-  installed_extension_view->Update();
-
-  int new_index =
-      FindIndex(parent_view,
-                installed_extension_view->view_controller()->GetActionName());
-  parent_view->ReorderChildView(installed_extension_view, new_index);
-}
-
-// Updates the `site_access_view` state and its position under `parent_view`.
-void UpdateSiteAccessMenuItem(views::View* parent_view,
-                              SiteAccessMenuItemView* site_access_view) {
-  site_access_view->Update();
-
-  int new_index = FindIndex(
-      parent_view, site_access_view->view_controller()->GetActionName());
-  parent_view->ReorderChildView(site_access_view, new_index);
-}
-
 // Returns the web content's host. This method should only be called when
 // web contents are present.
 std::u16string GetCurrentHost(content::WebContents* web_contents) {
@@ -401,27 +378,9 @@
 
 void ExtensionsTabbedMenuView::OnToolbarActionUpdated(
     const ToolbarActionsModel::ActionId& action_id) {
-  auto update_installed_extension_item =
-      [](views::View* parent_view,
-         const ToolbarActionsModel::ActionId& action_id) {
-        auto* item_view = GetInstalledExtensionMenuItem(parent_view, action_id);
-        if (item_view)
-          UpdateInstalledExtensionMenuItem(parent_view, item_view);
-      };
+  UpdateInstalledExtensionMenuItems({action_id});
+  UpdateSiteAccessMenuItems({action_id});
 
-  auto update_site_access_item =
-      [](views::View* parent_view,
-         const ToolbarActionsModel::ActionId& action_id) {
-        auto* item_view = GetSiteAccessMenuItem(parent_view, action_id);
-        if (item_view)
-          UpdateSiteAccessMenuItem(parent_view, item_view);
-      };
-
-  update_installed_extension_item(installed_items_, action_id);
-  update_site_access_item(requests_access_.items, action_id);
-  update_site_access_item(has_access_.items, action_id);
-
-  MoveItemsBetweenSectionsIfNecessary();
   UpdateSiteAccessTab();
 
   ConsistencyCheck();
@@ -475,22 +434,16 @@
 
 void ExtensionsTabbedMenuView::UserPermissionsSettingsChanged(
     const extensions::PermissionsManager::UserPermissionsSettings& settings) {
+  UpdateSiteAccessMenuItems(toolbar_model_->action_ids());
   UpdateSiteAccessTab();
+
+  SizeToContents();
 }
 
 void ExtensionsTabbedMenuView::Update() {
-  // An extension that previously did not want access, and therefore was not in
-  // a site access section, may want access now. This means moving existent
-  // items between sections is not sufficient. Therefore, we need to clear the
-  // site access sections and re-insert items in the correct place.
-  requests_access_.items->RemoveAllChildViews();
-  has_access_.items->RemoveAllChildViews();
-
-  for (views::View* view : installed_items_->children()) {
-    auto* item_view = GetAsInstalledExtensionMenuItem(view);
-    UpdateInstalledExtensionMenuItem(installed_items_, item_view);
-    MaybeCreateAndInsertSiteAccessItem(item_view->view_controller()->GetId());
-  }
+  const auto& action_ids = toolbar_model_->action_ids();
+  UpdateInstalledExtensionMenuItems(action_ids);
+  UpdateSiteAccessMenuItems(action_ids);
 
   UpdateSiteAccessTab();
 
@@ -680,40 +633,65 @@
   section->items->AddChildViewAt(std::move(item), index);
 }
 
-void ExtensionsTabbedMenuView::MoveItemsBetweenSectionsIfNecessary() {
-  content::WebContents* const web_contents =
-      browser_->tab_strip_model()->GetActiveWebContents();
+void ExtensionsTabbedMenuView::UpdateInstalledExtensionMenuItems(
+    const base::flat_set<ToolbarActionsModel::ActionId>& action_ids) {
+  for (const auto& action_id : action_ids) {
+    auto* item = GetInstalledExtensionMenuItem(installed_items_, action_id);
+    // Extensions should always have an installed extension menu entry.
+    DCHECK(item);
 
-  auto move_items_between_sections_if_necessary =
-      [web_contents, this](SiteAccessSection* section) {
-        // Collect the views to move separately, so that we don't change the
-        // children of the view during iteration.
-        std::vector<SiteAccessMenuItemView*> items_to_move;
-        for (views::View* view : section->items->children()) {
-          auto* item_view = GetAsSiteAccessMenuItem(view);
-          auto site_interaction =
-              item_view->view_controller()->GetSiteInteraction(web_contents);
-          if (site_interaction == section->site_interaction)
-            continue;
+    item->Update();
+    installed_items_->ReorderChildView(
+        item,
+        FindIndex(installed_items_, item->view_controller()->GetActionName()));
+  }
+}
 
-          items_to_move.push_back(item_view);
-        }
+void ExtensionsTabbedMenuView::UpdateSiteAccessMenuItems(
+    const base::flat_set<ToolbarActionsModel::ActionId>& action_ids) {
+  for (const auto& action_id : action_ids) {
+    // Retrieve the current section and item for the action id, if any.
+    SiteAccessSection* section = nullptr;
+    SiteAccessMenuItemView* item = nullptr;
+    if (auto* current_item =
+            GetSiteAccessMenuItem(requests_access_.items, action_id)) {
+      section = &requests_access_;
+      item = current_item;
+    } else if (auto* current_item =
+                   GetSiteAccessMenuItem(has_access_.items, action_id)) {
+      section = &has_access_;
+      item = current_item;
+    }
 
-        for (SiteAccessMenuItemView* item_view : items_to_move) {
-          auto item_view_to_move = section->items->RemoveChildViewT(item_view);
-          auto site_interaction =
-              item_view_to_move->view_controller()->GetSiteInteraction(
-                  web_contents);
-          auto* new_section = GetSectionForSiteInteraction(site_interaction);
-          if (!new_section)
-            return;
+    // Create item when it was not on a site access section. This can happen
+    // when an extension didn't previously have or request access, and now does.
+    if (!item) {
+      MaybeCreateAndInsertSiteAccessItem(action_id);
+      continue;
+    }
 
-          InsertSiteAccessItem(std::move(item_view_to_move), new_section);
-        }
-      };
+    // Reorder item when it is in the same section.
+    auto site_interaction = item->view_controller()->GetSiteInteraction(
+        browser_->tab_strip_model()->GetActiveWebContents());
+    if (site_interaction == section->site_interaction) {
+      item->Update();
+      int new_index =
+          FindIndex(section->items, item->view_controller()->GetActionName());
+      section->items->ReorderChildView(item, new_index);
+      return;
+    }
 
-  move_items_between_sections_if_necessary(&requests_access_);
-  move_items_between_sections_if_necessary(&has_access_);
+    // Remove item when it is in a different section or no section at all.
+    std::unique_ptr<SiteAccessMenuItemView> item_to_move =
+        section->items->RemoveChildViewT(item);
+    auto* new_section = GetSectionForSiteInteraction(site_interaction);
+    if (!new_section)
+      return;
+
+    // Re insert item to the correct section.
+    item_to_move->Update();
+    InsertSiteAccessItem(std::move(item_to_move), new_section);
+  }
 }
 
 void ExtensionsTabbedMenuView::UpdateSiteAccessTab() {
@@ -740,7 +718,6 @@
     case extensions::PermissionsManager::UserSiteSetting::kGrantAllExtensions:
       SetButtonChecked(site_settings_, kGrantAllExtensionsIndex);
       // TODO(crbug.com/1263310): Remove combobox from SiteAccessMenuItems.
-      MoveItemsBetweenSectionsIfNecessary();
       UpdateSiteAccessSectionsVisibility();
       // TODO(crbug.com/1263310): After finishing implementation of user
       // permission (grant user permissions with precedence over extension
diff --git a/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.h b/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.h
index 50aa1954..7a18845 100644
--- a/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.h
+++ b/chrome/browser/ui/views/extensions/extensions_tabbed_menu_view.h
@@ -169,9 +169,18 @@
   void InsertSiteAccessItem(std::unique_ptr<SiteAccessMenuItemView> item,
                             SiteAccessSection* section);
 
-  // Moves items between site access sections if their site access status
-  // changed. Called when one or more items are updated.
-  void MoveItemsBetweenSectionsIfNecessary();
+  // Updates the installed extension menu items corresponding to `action_ids`,
+  // and their positions in the extensions tab.
+  void UpdateInstalledExtensionMenuItems(
+      const base::flat_set<ToolbarActionsModel::ActionId>& action_ids);
+
+  // Updates the site access menu items corresponding to `action_ids`, and their
+  // positions in their corresponding site access section (moving the item
+  // between sections if necessary). Note that if there is no site access item
+  // for an action id, it creates and inserts a site access item in its
+  // corresponding site access section.
+  void UpdateSiteAccessMenuItems(
+      const base::flat_set<ToolbarActionsModel::ActionId>& action_ids);
 
   // Updates the visibility and contents of the site access tab based on the
   // current site setting.