From 22ab54d4ea15862de12e189739b6706216f3eabd Mon Sep 17 00:00:00 2001 From: Florian Jacky Date: Tue, 6 Jun 2023 10:13:50 +0000 Subject: [PATCH] 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 7a97a667d0f472baa89945c24785c8edc54468d3) Change-Id: I5a591c4317e3be7bdf7cb5e71e00a1e8f3287393 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4570985 Commit-Queue: Florian Jacky Reviewed-by: Olesia Marukhno Reviewed-by: Elias Klim Auto-Submit: Florian Jacky Cr-Original-Commit-Position: refs/heads/main@{#1152583} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4591690 Bot-Commit: Rubber Stamper Commit-Queue: Olesia Marukhno Cr-Commit-Position: refs/branch-heads/5790@{#399} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../views/location_bar/location_bar_view.cc | 6 +-- .../ui/views/permissions/chip_controller.cc | 49 +++++-------------- .../ui/views/permissions/chip_controller.h | 13 +---- .../permissions/permission_request_manager.cc | 12 ++++- .../permissions/permission_request_manager.h | 2 + 5 files changed, 27 insertions(+), 55 deletions(-) diff --git a/chrome/browser/ui/views/location_bar/location_bar_view.cc b/chrome/browser/ui/views/location_bar/location_bar_view.cc index fd23e864d0df4..f5f366196814f 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.cc +++ b/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(); } } diff --git a/chrome/browser/ui/views/permissions/chip_controller.cc b/chrome/browser/ui/views/permissions/chip_controller.cc index b615377c84d32..db127c5ca7ac6 100644 --- a/chrome/browser/ui/views/permissions/chip_controller.cc +++ b/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(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(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() { diff --git a/chrome/browser/ui/views/permissions/chip_controller.h b/chrome/browser/ui/views/permissions/chip_controller.h index 2ed519920fbba..a2858a55aeddf 100644 --- a/chrome/browser/ui/views/permissions/chip_controller.h +++ b/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(); diff --git a/components/permissions/permission_request_manager.cc b/components/permissions/permission_request_manager.cc index 972665902f87f..e723ce599c299 100644 --- a/components/permissions/permission_request_manager.cc +++ b/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() { diff --git a/components/permissions/permission_request_manager.h b/components/permissions/permission_request_manager.h index 066a4f79f1184..4f3f5e8814964 100644 --- a/components/permissions/permission_request_manager.h +++ b/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();