Flow refactoring to address confirmation chip stickiness

This CL forwards tab visibility update events from the PRM to the chip_controller instead of using location bar view update events. This simplifies event handling and avoids event order dependencies.

Bug: 1450815

(cherry picked from commit 7a97a66)

Change-Id: I5a591c4317e3be7bdf7cb5e71e00a1e8f3287393
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4570985
Commit-Queue: Florian Jacky <fjacky@chromium.org>
Reviewed-by: Olesia Marukhno <olesiamarukhno@google.com>
Reviewed-by: Elias Klim <elklm@chromium.org>
Auto-Submit: Florian Jacky <fjacky@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1152583}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4591690
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Olesia Marukhno <olesiamarukhno@google.com>
Cr-Commit-Position: refs/branch-heads/5790@{#399}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}

chrome/browser/ui/views/location_bar/location_bar_view.cc

@@ -853,10 +853,6 @@ void LocationBarView::Update(WebContents* contents) {
   RefreshPageActionIconViews();
   location_icon_view_->Update(/*suppress_animations=*/contents);
 
-  if (is_initialized_ && chip_controller_) {
-    chip_controller_->OnWebContentsChanged();
-  }
-
   if (intent_chip_)
     intent_chip_->Update();
 
@@ -1571,7 +1567,7 @@ void LocationBarView::UpdateChipVisibility() {
   if (IsEditingOrEmpty()) {
     // If a user starts typing, a permission request should be ignored and the
     // chip finalized.
-    chip_controller_->ResetChip();
+    chip_controller_->ResetPermissionPromptChip();
   }
 }
 

chrome/browser/ui/views/permissions/chip_controller.cc

@@ -30,6 +30,7 @@
 #include "components/permissions/permission_util.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/visibility.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/accessibility/view_accessibility.h"
 #include "ui/views/controls/button/button.h"
@@ -94,19 +95,6 @@ void ChipController::OnPermissionRequestManagerDestructed() {
   }
 }
 
-void ChipController::OnWebContentsChanged() {
-  if (!is_waiting_for_confirmation_collapse) {
-    return;
-  }
-
-  if (!active_chip_permission_request_manager_.has_value() ||
-      !active_chip_permission_request_manager_.value()->IsRequestInProgress()) {
-    // Because the web contents changed, we should no longer display any chip
-    // that was displayed for the previous web contents.
-    ResetChip();
-  }
-}
-
 void ChipController::OnNavigation(
     content::NavigationHandle* navigation_handle) {
   // TODO(crbug.com/1416493): Refactor this so that this observer method is only
@@ -120,11 +108,8 @@ void ChipController::OnNavigation(
   ResetPermissionPromptChip();
 }
 
-void ChipController::OnPromptRemoved() {
-  bool is_tab_hidden = active_chip_permission_request_manager_.value()
-                           ->GetWebContents()
-                           .GetVisibility() == content::Visibility::HIDDEN;
-  if (is_tab_hidden || !is_confirmation_showing_) {
+void ChipController::OnTabVisibilityChanged(content::Visibility visibility) {
+  if (visibility == content::Visibility::HIDDEN) {
     ResetPermissionPromptChip();
   }
 }
@@ -139,7 +124,7 @@ void ChipController::OnRequestDecided(
       GetLocationBarView()->GetWidget()->IsFullscreen()) {
     // If the location bar isn't drawn or during fullscreen, the chip can't be
     // shown anywhere.
-    ResetChip();
+    ResetPermissionPromptChip();
   } else if (base::FeatureList::IsEnabled(
                  permissions::features::kConfirmationChip)) {
     HandleConfirmation(permission_action);
@@ -226,7 +211,7 @@ void ChipController::InitializePermissionPrompt(
     return;
   }
 
-  ResetChip();
+  ResetPermissionPromptChip();
 
   // Here we just initialize the controller with the current request. We might
   // not yet want to display the chip, for example when a prompt bubble without
@@ -298,16 +283,6 @@ void ChipController::ShowPermissionPrompt(
   }
 }
 
-void ChipController::ResetChip() {
-  // This is a placeholder method, additional chip functionality will be added
-  // and will be reset here.
-  ResetPermissionPromptChip();
-}
-
-void ChipController::ResetChipCallbacks() {
-  chip_->SetCallback(base::RepeatingCallback<void()>(base::DoNothing()));
-}
-
 void ChipController::RemoveBubbleObserverAndResetTimersAndChipCallbacks() {
   views::Widget* const bubble_widget = GetBubbleWidget();
   if (bubble_widget) {
@@ -316,8 +291,10 @@ void ChipController::RemoveBubbleObserverAndResetTimersAndChipCallbacks() {
     bubble_widget->Close();
   }
 
+  // Reset button click callback
+  chip_->SetCallback(base::RepeatingCallback<void()>(base::DoNothing()));
+
   ResetTimers();
-  ResetChipCallbacks();
 }
 
 void ChipController::ResetPermissionPromptChip() {
@@ -434,7 +411,7 @@ void ChipController::HandleConfirmation(
     collapse_timer_.Start(FROM_HERE, kConfirmationDisplayDuration, this,
                           &ChipController::CollapseConfirmation);
   } else {
-    ResetChip();
+    ResetPermissionPromptChip();
   }
 }
 
@@ -642,10 +619,10 @@ void ChipController::SyncChipWithModel() {
 }
 
 void ChipController::StartCollapseTimer() {
-  collapse_timer_.Start(
-      FROM_HERE, kDelayBeforeCollapsingChip,
-      base::BindOnce(&ChipController::CollapsePrompt, base::Unretained(this),
-                     /*allow_restart=*/true));
+  collapse_timer_.Start(FROM_HERE, kDelayBeforeCollapsingChip,
+                        base::BindOnce(&ChipController::CollapsePrompt,
+                                       weak_factory_.GetWeakPtr(),
+                                       /*allow_restart=*/true));
 }
 
 void ChipController::StartDismissTimer() {

chrome/browser/ui/views/permissions/chip_controller.h

@@ -48,10 +48,7 @@ class ChipController : public permissions::PermissionRequestManager::Observer,
 
   // PermissionRequestManager::Observer:
   void OnPermissionRequestManagerDestructed() override;
-
-  void OnPromptRemoved() override;
-
-  void OnWebContentsChanged();
+  void OnTabVisibilityChanged(content::Visibility visibility) override;
 
   // OnBubbleRemoved only triggers when a request chip (bubble) is removed, when
   // the user navigates while a confirmation chip is showing, the request is
@@ -89,10 +86,6 @@ class ChipController : public permissions::PermissionRequestManager::Observer,
   // Chip View.
   OmniboxChipButton* chip() { return chip_; }
 
-  // Hide and clean up the entire chip and associated observers, callback timers
-  // and callbacks.
-  void ResetChip();
-
   // Hide and clean up permission parts of the chip.
   void ResetPermissionPromptChip();
 
@@ -176,10 +169,6 @@ class ChipController : public permissions::PermissionRequestManager::Observer,
   void OnPageInfoBubbleClosed(views::Widget::ClosedReason closed_reason,
                               bool reload_prompt);
 
-  // Resets all chip callbacks such as click callback, but also
-  // animation-related callbacks.
-  void ResetChipCallbacks();
-
   // Clean up utility.
   void RemoveBubbleObserverAndResetTimersAndChipCallbacks();
 

components/permissions/permission_request_manager.cc

@@ -492,7 +492,7 @@ void PermissionRequestManager::OnVisibilityChanged(
   tab_is_hidden_ = visibility == content::Visibility::HIDDEN;
   if (tab_was_hidden == tab_is_hidden_)
     return;
-
+  NotifyTabVisibilityChanged(visibility);
   if (tab_is_hidden_) {
     if (view_) {
       switch (view_->GetTabSwitchingBehavior()) {
@@ -1267,14 +1267,22 @@ bool PermissionRequestManager::ShouldDropCurrentRequestIfCannotShowQuietly()
   return false;
 }
 
+void PermissionRequestManager::NotifyTabVisibilityChanged(
+    content::Visibility visibility) {
+  for (Observer& observer : observer_list_) {
+    observer.OnTabVisibilityChanged(visibility);
+  }
+}
+
 void PermissionRequestManager::NotifyPromptAdded() {
   for (Observer& observer : observer_list_)
     observer.OnPromptAdded();
 }
 
 void PermissionRequestManager::NotifyPromptRemoved() {
-  for (Observer& observer : observer_list_)
+  for (Observer& observer : observer_list_) {
     observer.OnPromptRemoved();
+  }
 }
 
 void PermissionRequestManager::NotifyPromptRecreateFailed() {

components/permissions/permission_request_manager.h

@@ -76,6 +76,7 @@ class PermissionRequestManager
  public:
   class Observer : public base::CheckedObserver {
    public:
+    virtual void OnTabVisibilityChanged(content::Visibility visibility) {}
     virtual void OnPromptAdded() {}
     virtual void OnPromptRemoved() {}
     // Called when recreation of the permission prompt is not possible. It means
@@ -363,6 +364,7 @@ class PermissionRequestManager
   // Calls RequestFinished on a request and all its duplicates.
   void RequestFinishedIncludingDuplicates(PermissionRequest* request);
 
+  void NotifyTabVisibilityChanged(content::Visibility visibility);
   void NotifyPromptAdded();
   void NotifyPromptRemoved();
   void NotifyPromptRecreateFailed();