From 6eca156d8074d83621c46234b5d137b2a8a74e77 Mon Sep 17 00:00:00 2001 From: Illia Klimov Date: Wed, 23 Nov 2022 18:44:12 +0000 Subject: [PATCH] Restore a permission prompt on tab change. This CL fixes a bug where we resolve a permission request if a user opened a new tab and started editing the location bar view. In that case, when the user switched back to the first tab, the permission request will be automatically ignored. Bug: 1379249,931657,1003747 Change-Id: I8cd9d573be9c9baba467f4be4bae3c41da103427 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3999327 Commit-Queue: Illia Klimov Reviewed-by: Evan Stade Reviewed-by: Ravjit Uppal Cr-Commit-Position: refs/heads/main@{#1075251} --- .../permission_request_manager_browsertest.cc | 116 +++++++++++------- .../views/location_bar/location_bar_view.cc | 10 ++ .../permissions/permission_prompt_factory.cc | 16 ++- .../permission_request_chip_browsertest.cc | 12 +- .../permissions/permission_request_manager.cc | 34 ++++- .../permissions/permission_request_manager.h | 16 +++ .../permission_request_manager_unittest.cc | 6 +- components/permissions/permission_uma_util.cc | 40 ++++++ components/permissions/permission_uma_util.h | 5 + .../test/permission_request_observer.cc | 10 ++ .../test/permission_request_observer.h | 8 ++ .../metadata/permissions/histograms.xml | 13 ++ 12 files changed, 232 insertions(+), 54 deletions(-) diff --git a/chrome/browser/permissions/permission_request_manager_browsertest.cc b/chrome/browser/permissions/permission_request_manager_browsertest.cc index 0a63adaa78e42..9c9a71ee20a33 100644 --- a/chrome/browser/permissions/permission_request_manager_browsertest.cc +++ b/chrome/browser/permissions/permission_request_manager_browsertest.cc @@ -38,6 +38,7 @@ #include "components/permissions/request_type.h" #include "components/permissions/test/mock_permission_prompt_factory.h" #include "components/permissions/test/mock_permission_request.h" +#include "components/permissions/test/permission_request_observer.h" #include "components/variations/variations_associated_data.h" #include "content/public/browser/permission_controller.h" #include "content/public/browser/render_frame_host.h" @@ -60,7 +61,6 @@ #include "url/origin.h" namespace { - const char* kPermissionsKillSwitchFieldStudy = permissions::PermissionContextBase::kPermissionsKillSwitchFieldStudy; const char* kPermissionsKillSwitchBlockedValue = @@ -291,7 +291,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, EXPECT_EQ(2, bubble_factory()->TotalRequestCount()); } -// Requests before the load should not be bundled with a request after the load. +// Requests before the load should not be bundled with a request after the +// load. IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, RequestsBeforeAfterLoad) { ASSERT_TRUE(embedded_test_server()->Start()); @@ -307,9 +308,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, EXPECT_EQ(1, bubble_factory()->TotalRequestCount()); } -// Navigating twice to the same URL should be equivalent to refresh. This means -// showing the bubbles twice. -// http://crbug.com/512849 flaky +// Navigating twice to the same URL should be equivalent to refresh. This +// means showing the bubbles twice. http://crbug.com/512849 flaky IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, DISABLED_NavTwice) { ASSERT_TRUE(embedded_test_server()->Start()); @@ -329,9 +329,9 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, DISABLED_NavTwice) { EXPECT_EQ(2, bubble_factory()->TotalRequestCount()); } -// Navigating twice to the same URL with a hash should be navigation within the -// page. This means the bubble is only shown once. -// http://crbug.com/512849 flaky +// Navigating twice to the same URL with a hash should be navigation within +// the page. This means the bubble is only shown once. http://crbug.com/512849 +// flaky IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, DISABLED_NavTwiceWithHash) { ASSERT_TRUE(embedded_test_server()->Start()); @@ -376,9 +376,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, // Prompts are only shown for active tabs and (on Desktop) hidden on tab // switching -// Flaky on bots crbug.com/1003747. -IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, - DISABLED_MultipleTabs) { +IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, MultipleTabs) { ASSERT_TRUE(embedded_test_server()->Start()); ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( @@ -399,10 +397,27 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, ASSERT_EQ(2, tab_strip_model->count()); ASSERT_EQ(1, tab_strip_model->active_index()); - // Request geolocation in foreground tab, prompt should be shown. - ExecuteScriptAndGetValue( - tab_strip_model->GetWebContentsAt(1)->GetPrimaryMainFrame(), - "navigator.geolocation.getCurrentPosition(function(){});"); + constexpr char kRequestNotifications[] = R"( + new Promise(resolve => { + Notification.requestPermission().then(function (permission) { + resolve(permission) + }); + }) + )"; + + { + permissions::PermissionRequestObserver observer( + tab_strip_model->GetWebContentsAt(1)); + + // Request permission in foreground tab, prompt should be shown. + EXPECT_TRUE(content::ExecJs( + tab_strip_model->GetWebContentsAt(1)->GetPrimaryMainFrame(), + kRequestNotifications, + content::EvalJsOptions::EXECUTE_SCRIPT_NO_RESOLVE_PROMISES)); + + observer.Wait(); + } + EXPECT_EQ(1, bubble_factory_1->show_count()); EXPECT_FALSE(bubble_factory_0->is_visible()); EXPECT_TRUE(bubble_factory_1->is_visible()); @@ -416,11 +431,21 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, EXPECT_FALSE(bubble_factory_0->is_visible()); EXPECT_TRUE(bubble_factory_1->is_visible()); - // Request notification in background tab. No prompt is shown until the tab - // itself is activated. - ExecuteScriptAndGetValue( - tab_strip_model->GetWebContentsAt(0)->GetPrimaryMainFrame(), - "Notification.requestPermission()"); + { + permissions::PermissionRequestObserver observer( + tab_strip_model->GetWebContentsAt(0)); + + // Request notification in background tab. No prompt is shown until the + // tab itself is activated. + EXPECT_TRUE(content::ExecJs( + tab_strip_model->GetWebContentsAt(0)->GetPrimaryMainFrame(), + kRequestNotifications, + content::EvalJsOptions::EXECUTE_SCRIPT_NO_RESOLVE_PROMISES)); + + observer.Wait(); + EXPECT_TRUE(observer.is_prompt_show_failed_hidden_tab()); + } + EXPECT_FALSE(bubble_factory_0->is_visible()); EXPECT_EQ(2, bubble_factory_1->show_count()); @@ -430,36 +455,37 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, EXPECT_EQ(2, bubble_factory_1->show_count()); } -// Regularly timing out in Windows, Linux and macOS Debug Builds. -// https://crbug.com/931657 IN_PROC_BROWSER_TEST_F(PermissionRequestManagerBrowserTest, - DISABLED_BackgroundTabNavigation) { + BackgroundTabNavigation) { ASSERT_TRUE(embedded_test_server()->Start()); ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( browser(), embedded_test_server()->GetURL("/empty.html"), 1); - // Request camera, prompt should be shown. + // Request Notifications, prompt should be shown. ExecuteScriptAndGetValue( browser()->tab_strip_model()->GetWebContentsAt(0)->GetPrimaryMainFrame(), - "navigator.getUserMedia({video: true}, ()=>{}, ()=>{})"); + "Notification.requestPermission()"); bubble_factory()->WaitForPermissionBubble(); EXPECT_TRUE(bubble_factory()->is_visible()); EXPECT_EQ(1, bubble_factory()->show_count()); - // SetUp() only creates a mock prompt factory for the first tab but this test - // doesn't request any permissions in the second tab so it doesn't need one. + // SetUp() only creates a mock prompt factory for the first tab but this + // test doesn't request any permissions in the second tab so it doesn't need + // one. ui_test_utils::NavigateToURLWithDisposition( browser(), embedded_test_server()->GetURL("/empty.html"), WindowOpenDisposition::NEW_FOREGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); // Navigate background tab, prompt should be removed. + content::TestNavigationObserver observer( + browser()->tab_strip_model()->GetWebContentsAt(0)); + ExecuteScriptAndGetValue( browser()->tab_strip_model()->GetWebContentsAt(0)->GetPrimaryMainFrame(), "window.location = 'simple.html'"); - content::TestNavigationObserver observer( - browser()->tab_strip_model()->GetWebContentsAt(0)); + observer.Wait(); EXPECT_FALSE(bubble_factory()->is_visible()); @@ -742,6 +768,11 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest, SetCannedUiDecision(QuietUiReason::kTriggeredDueToAbusiveContent, WarningReason::kAbusiveContent); + ASSERT_TRUE(embedded_test_server()->Start()); + + ui_test_utils::NavigateToURLBlockUntilNavigationsComplete( + browser(), embedded_test_server()->GetURL("/empty.html"), 1); + auto* web_contents = browser()->tab_strip_model()->GetActiveWebContents(); permissions::MockPermissionRequest request_quiet( permissions::RequestType::kNotifications); @@ -753,9 +784,12 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest, auto disposition_from_prompt_bubble = manager->view_for_testing()->GetPromptDisposition(); - // There will be no instance of PermissionPromptImpl after a tab marked as - // HIDDEN. - manager->OnVisibilityChanged(content::Visibility::HIDDEN); + // There will be no instance of PermissionPromptImpl after a new tab is opened + // and existing tab marked as HIDDEN. + ui_test_utils::NavigateToURLWithDisposition( + browser(), embedded_test_server()->GetURL("/empty.html"), + WindowOpenDisposition::NEW_FOREGROUND_TAB, + ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP); absl::optional disposition = manager->current_request_prompt_disposition_for_testing(); @@ -764,7 +798,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerQuietUiBrowserTest, EXPECT_EQ(disposition.value(), disposition_from_prompt_bubble); // DCHECK failure if Closing executed on HIDDEN PermissionRequestManager. - manager->OnVisibilityChanged(content::Visibility::VISIBLE); + browser()->tab_strip_model()->ActivateTabAt(0); manager->Dismiss(); base::RunLoop().RunUntilIdle(); } @@ -912,10 +946,10 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithBackForwardCacheBrowserTest, content::RenderFrameHostWrapper rfh_b(GetActiveMainFrame()); // Mic, camera, and pan/tilt/zoom requests are grouped if they come from the - // same origin. Make sure this will not include requests from a cached frame. - // Note pages will not be cached when navigating within the same origin, so we - // have different urls in the navigations above but use the same (default) url - // for the MockPermissionRequest here. + // same origin. Make sure this will not include requests from a cached + // frame. Note pages will not be cached when navigating within the same + // origin, so we have different urls in the navigations above but use the + // same (default) url for the MockPermissionRequest here. permissions::MockPermissionRequest req_a_1( permissions::RequestType::kCameraPanTiltZoom, permissions::PermissionRequestGestureType::GESTURE); @@ -1098,8 +1132,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithPrerenderingTest, base::RunLoop().RunUntilIdle(); - // Permission request from main frame should be granted, similar request from - // prerender should be denied. + // Permission request from main frame should be granted, similar request + // from prerender should be denied. EXPECT_TRUE(request_1.granted()); EXPECT_TRUE(request_2->cancelled()); EXPECT_TRUE(deleted_observer.deleted()); @@ -1183,8 +1217,8 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestManagerWithFencedFrameTest, })(); )"; - // The result of query 'geolocation' permission in the fenced frame should be - // 'denied'. + // The result of query 'geolocation' permission in the fenced frame should + // be 'denied'. EXPECT_EQ("denied", content::EvalJs(fenced_frame_host, kQueryPermission)); const char kQueryCurrentPosition[] = R"( 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 3caff12ee71fa..b7e76645b6ef0 100644 --- a/chrome/browser/ui/views/location_bar/location_bar_view.cc +++ b/chrome/browser/ui/views/location_bar/location_bar_view.cc @@ -89,6 +89,7 @@ #include "components/omnibox/common/omnibox_features.h" #include "components/performance_manager/public/features.h" #include "components/permissions/features.h" +#include "components/permissions/permission_request_manager.h" #include "components/prefs/pref_service.h" #include "components/safe_browsing/core/common/features.h" #include "components/search_engines/template_url.h" @@ -831,6 +832,15 @@ void LocationBarView::Update(WebContents* contents) { qr_generator_icon->SetVisible(false); OnChanged(); // NOTE: Calls Layout(). + + // A permission prompt may be suspended due to an invalid state (empty or + // editing location bar). Restore the suspended prompt if possible. + if (contents && !IsEditingOrEmpty()) { + auto* permission_request_manager = + permissions::PermissionRequestManager::FromWebContents(contents); + if (permission_request_manager->CanRestorePrompt()) + permission_request_manager->RestorePrompt(); + } } void LocationBarView::ResetTabState(WebContents* contents) { diff --git a/chrome/browser/ui/views/permissions/permission_prompt_factory.cc b/chrome/browser/ui/views/permissions/permission_prompt_factory.cc index 18923a76ceea2..82a50b7e22dfe 100644 --- a/chrome/browser/ui/views/permissions/permission_prompt_factory.cc +++ b/chrome/browser/ui/views/permissions/permission_prompt_factory.cc @@ -16,6 +16,7 @@ #include "chrome/common/webui_url_constants.h" #include "components/permissions/features.h" #include "components/permissions/permission_request.h" +#include "components/permissions/permission_uma_util.h" #include "components/permissions/request_type.h" #include "content/public/browser/web_contents.h" @@ -46,8 +47,10 @@ LocationBarView* GetLocationBarView(Browser* browser) { // A permission request should be auto-ignored if a user interacts with the // LocationBar. The only exception is the NTP page where the user needs to press // on a microphone icon to get a permission request. -bool ShouldIgnorePermissionRequest(content::WebContents* web_contents, - Browser* browser) { +bool ShouldIgnorePermissionRequest( + content::WebContents* web_contents, + Browser* browser, + permissions::PermissionPrompt::Delegate* delegate) { DCHECK(web_contents); DCHECK(browser); @@ -58,7 +61,12 @@ bool ShouldIgnorePermissionRequest(content::WebContents* web_contents, } LocationBarView* location_bar = GetLocationBarView(browser); - return location_bar && location_bar->IsEditingOrEmpty(); + bool can_display_prompt = location_bar && location_bar->IsEditingOrEmpty(); + + permissions::PermissionUmaUtil::RecordPermissionPromptAttempt( + delegate->Requests(), can_display_prompt); + + return can_display_prompt; } bool ShouldUseChip(permissions::PermissionPrompt::Delegate* delegate) { @@ -168,7 +176,7 @@ std::unique_ptr CreatePermissionPrompt( } // Auto-ignore the permission request if a user is typing into location bar. - if (ShouldIgnorePermissionRequest(web_contents, browser)) { + if (ShouldIgnorePermissionRequest(web_contents, browser, delegate)) { return nullptr; } diff --git a/chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc b/chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc index 059ee0da5487c..f2850ed73f826 100644 --- a/chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc +++ b/chrome/browser/ui/views/permissions/permission_request_chip_browsertest.cc @@ -18,6 +18,7 @@ #include "chrome/browser/ui/views/toolbar/toolbar_view.h" #include "chrome/test/base/ui_test_utils.h" #include "chrome/test/permissions/permission_request_manager_test_api.h" +#include "components/metrics/content/subprocess_metrics_provider.h" #include "components/omnibox/browser/omnibox_edit_model.h" #include "components/omnibox/browser/omnibox_view.h" #include "components/omnibox/browser/open_tab_provider.h" @@ -133,6 +134,7 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest, IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest, PermissionRequestIsAutoIgnored) { ASSERT_TRUE(embedded_test_server()->Start()); + base::HistogramTester histograms; content::WebContents* embedder_contents = browser()->tab_strip_model()->GetActiveWebContents(); @@ -193,13 +195,19 @@ IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest, // Wait until a permission request is shown or finalized. observer.Wait(); - // Permission request was finalized without showing a prompt bubble. - EXPECT_FALSE(manager->IsRequestInProgress()); + // Permission request was is in progress without showing a prompt bubble. + EXPECT_TRUE(manager->IsRequestInProgress()); EXPECT_FALSE(observer.request_shown()); + EXPECT_TRUE(observer.is_view_recreate_failed()); + EXPECT_FALSE(manager->view_for_testing()); EXPECT_FALSE(content::EvalJs(main_rfh, kCheckMicrophone, content::EXECUTE_SCRIPT_DEFAULT_OPTIONS, 1) .value.GetBool()); + + metrics::SubprocessMetricsProvider::MergeHistogramDeltasForTesting(); + histograms.ExpectBucketCount( + "Permissions.Prompt.AudioCapture.Gesture.Attempt", true, 1); } IN_PROC_BROWSER_TEST_F(PermissionRequestChipGestureSensitiveBrowserTest, diff --git a/components/permissions/permission_request_manager.cc b/components/permissions/permission_request_manager.cc index 0bdc0e130cccb..c5cdad1b5a82c 100644 --- a/components/permissions/permission_request_manager.cc +++ b/components/permissions/permission_request_manager.cc @@ -691,7 +691,10 @@ bool PermissionRequestManager::RecreateView() { if (!view_) { current_request_prompt_disposition_ = PermissionPromptDisposition::NONE_VISIBLE; - FinalizeCurrentRequests(PermissionAction::IGNORED); + if (ShouldDropCurrentRequestIfCannotShowQuietly()) { + FinalizeCurrentRequests(PermissionAction::IGNORED); + } + NotifyPromptRecreateFailed(); return false; } @@ -704,7 +707,6 @@ PermissionRequestManager::PermissionRequestManager( : content::WebContentsObserver(web_contents), content::WebContentsUserData(*web_contents), view_factory_(base::BindRepeating(&PermissionPrompt::Create)), - view_(nullptr), tab_is_hidden_(web_contents->GetVisibility() == content::Visibility::HIDDEN), auto_response_for_test_(NONE), @@ -808,8 +810,10 @@ void PermissionRequestManager::ShowPrompt() { DCHECK(web_contents()->IsDocumentOnLoadCompletedInPrimaryMainFrame()); DCHECK(current_request_ui_to_use_); - if (tab_is_hidden_) + if (tab_is_hidden_) { + NotifyPromptCreationFailedHiddenTab(); return; + } if (!ReprioritizeCurrentRequestIfNeeded()) return; @@ -1072,6 +1076,20 @@ bool PermissionRequestManager::IsRequestInProgress() const { return !requests_.empty(); } +bool PermissionRequestManager::CanRestorePrompt() { +#if BUILDFLAG(IS_ANDROID) + return false; +#else + return IsRequestInProgress() && + current_request_prompt_disposition_.has_value() && !view_; +#endif +} + +void PermissionRequestManager::RestorePrompt() { + if (CanRestorePrompt()) + ShowPrompt(); +} + bool PermissionRequestManager::ShouldDropCurrentRequestIfCannotShowQuietly() const { absl::optional quiet_ui_reason = ReasonForUsingQuietUi(); @@ -1102,6 +1120,16 @@ void PermissionRequestManager::NotifyPromptRemoved() { observer.OnPromptRemoved(); } +void PermissionRequestManager::NotifyPromptRecreateFailed() { + for (Observer& observer : observer_list_) + observer.OnPromptRecreateViewFailed(); +} + +void PermissionRequestManager::NotifyPromptCreationFailedHiddenTab() { + for (Observer& observer : observer_list_) + observer.OnPromptCreationFailedHiddenTab(); +} + void PermissionRequestManager::NotifyRequestDecided( permissions::PermissionAction permission_action) { for (Observer& observer : observer_list_) diff --git a/components/permissions/permission_request_manager.h b/components/permissions/permission_request_manager.h index 799dfde16ca61..b868a35f51585 100644 --- a/components/permissions/permission_request_manager.h +++ b/components/permissions/permission_request_manager.h @@ -75,6 +75,13 @@ class PermissionRequestManager public: virtual void OnPromptAdded() {} virtual void OnPromptRemoved() {} + // Called when recreation of the permission prompt is not possible. It means + // that `PermissionRequestManager` is ready to display a prompt but the UI + // layer was not able to display it. + virtual void OnPromptRecreateViewFailed() {} + // Called when permission prompt creation was aborted because the current + // tab is no longer visible, hance it is not possible to display a prompt. + virtual void OnPromptCreationFailedHiddenTab() {} // Called when the current batch of requests have been handled and the // prompt is no longer visible. Note that there might be some queued // permission requests that will get shown after this. This differs from @@ -119,6 +126,13 @@ class PermissionRequestManager bool IsRequestInProgress() const; + // Returns `true` if a permission request is in progress but a prompt view is + // nullptr. + bool CanRestorePrompt(); + + // Recreates a permission prompt. + void RestorePrompt(); + // Do NOT use this methods in production code. Use this methods in browser // tests that need to accept or deny permissions when requested in // JavaScript. Your test needs to set this appropriately, and then the bubble @@ -316,6 +330,8 @@ class PermissionRequestManager void NotifyPromptAdded(); void NotifyPromptRemoved(); + void NotifyPromptRecreateFailed(); + void NotifyPromptCreationFailedHiddenTab(); void NotifyRequestDecided(permissions::PermissionAction permission_action); void StorePermissionActionForUMA(const GURL& origin, diff --git a/components/permissions/permission_request_manager_unittest.cc b/components/permissions/permission_request_manager_unittest.cc index bc14171b02246..4fad316fba0f0 100644 --- a/components/permissions/permission_request_manager_unittest.cc +++ b/components/permissions/permission_request_manager_unittest.cc @@ -463,6 +463,7 @@ TEST_P(PermissionRequestManagerTest, MixOfMediaAndNotMediaRequests) { // Tab switching //////////////////////////////////////////////////////////////////////////////// +#if BUILDFLAG(IS_ANDROID) TEST_P(PermissionRequestManagerTest, TwoRequestsTabSwitch) { manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request_mic_); manager_->AddRequest(web_contents()->GetPrimaryMainFrame(), &request_camera_); @@ -472,11 +473,7 @@ TEST_P(PermissionRequestManagerTest, TwoRequestsTabSwitch) { ASSERT_EQ(prompt_factory_->request_count(), 2); MockTabSwitchAway(); -#if BUILDFLAG(IS_ANDROID) EXPECT_TRUE(prompt_factory_->is_visible()); -#else - EXPECT_FALSE(prompt_factory_->is_visible()); -#endif MockTabSwitchBack(); WaitForBubbleToBeShown(); @@ -487,6 +484,7 @@ TEST_P(PermissionRequestManagerTest, TwoRequestsTabSwitch) { EXPECT_TRUE(request_mic_.granted()); EXPECT_TRUE(request_camera_.granted()); } +#endif // BUILDFLAG(IS_ANDROID) TEST_P(PermissionRequestManagerTest, PermissionRequestWhileTabSwitchedAway) { MockTabSwitchAway(); diff --git a/components/permissions/permission_uma_util.cc b/components/permissions/permission_uma_util.cc index 25f741f4f4bc8..cf4ef6f8f179d 100644 --- a/components/permissions/permission_uma_util.cc +++ b/components/permissions/permission_uma_util.cc @@ -499,6 +499,46 @@ void PermissionUmaUtil::RecordEmbargoStatus( embargo_status, PermissionEmbargoStatus::NUM); } +void PermissionUmaUtil::RecordPermissionPromptAttempt( + const std::vector& requests, + bool IsLocationBarEditingOrEmpty) { + DCHECK(!requests.empty()); + + RequestTypeForUma request_type = RequestTypeForUma::MULTIPLE; + PermissionRequestGestureType gesture_type = + PermissionRequestGestureType::UNKNOWN; + if (requests.size() == 1) { + request_type = GetUmaValueForRequestType(requests[0]->request_type()); + gesture_type = requests[0]->GetGestureType(); + } + + std::string permission_type = GetPermissionRequestString(request_type); + + std::string gesture; + + switch (gesture_type) { + case PermissionRequestGestureType::UNKNOWN: { + gesture = "Unknown"; + break; + } + case PermissionRequestGestureType::GESTURE: { + gesture = "Gesture"; + break; + } + case PermissionRequestGestureType::NO_GESTURE: { + gesture = "NoGesture"; + break; + } + default: + NOTREACHED(); + } + + std::string histogram_name = + "Permissions.Prompt." + permission_type + "." + gesture + ".Attempt"; + + base::UmaHistogramBoolean(histogram_name, IsLocationBarEditingOrEmpty); +} + void PermissionUmaUtil::PermissionPromptShown( const std::vector& requests) { DCHECK(!requests.empty()); diff --git a/components/permissions/permission_uma_util.h b/components/permissions/permission_uma_util.h index 583b16a8fdd65..4b2816eec54d3 100644 --- a/components/permissions/permission_uma_util.h +++ b/components/permissions/permission_uma_util.h @@ -356,6 +356,11 @@ class PermissionUmaUtil { static void RecordEmbargoStatus(PermissionEmbargoStatus embargo_status); + // Recorded when a permission prompt creation is in progress. + static void RecordPermissionPromptAttempt( + const std::vector& requests, + bool IsLocationBarEditingOrEmpty); + // UMA specifically for when permission prompts are shown. This should be // roughly equivalent to the metrics above, however it is // useful to have separate UMA to a few reasons: diff --git a/components/permissions/test/permission_request_observer.cc b/components/permissions/test/permission_request_observer.cc index ba0bd2919da46..d95a658508810 100644 --- a/components/permissions/test/permission_request_observer.cc +++ b/components/permissions/test/permission_request_observer.cc @@ -26,6 +26,16 @@ void PermissionRequestObserver::OnRequestsFinalized() { loop_.Quit(); } +void PermissionRequestObserver::OnPromptRecreateViewFailed() { + is_view_recreate_failed_ = true; + loop_.Quit(); +} + +void PermissionRequestObserver::OnPromptCreationFailedHiddenTab() { + is_prompt_show_failed_hidden_tab_ = true; + loop_.Quit(); +} + void PermissionRequestObserver::OnPermissionRequestManagerDestructed() { observation_.Reset(); } diff --git a/components/permissions/test/permission_request_observer.h b/components/permissions/test/permission_request_observer.h index 48b32f13d85d3..950ae937e9db4 100644 --- a/components/permissions/test/permission_request_observer.h +++ b/components/permissions/test/permission_request_observer.h @@ -22,6 +22,10 @@ class PermissionRequestObserver : public PermissionRequestManager::Observer { ~PermissionRequestObserver() override; bool request_shown() const { return request_shown_; } + bool is_view_recreate_failed() const { return is_view_recreate_failed_; } + bool is_prompt_show_failed_hidden_tab() const { + return is_prompt_show_failed_hidden_tab_; + } // Blocks until a request is shown. void Wait(); @@ -29,6 +33,8 @@ class PermissionRequestObserver : public PermissionRequestManager::Observer { // PermissionRequestManager::Observer: void OnPromptAdded() override; void OnRequestsFinalized() override; + void OnPromptRecreateViewFailed() override; + void OnPromptCreationFailedHiddenTab() override; void OnPermissionRequestManagerDestructed() override; private: @@ -37,6 +43,8 @@ class PermissionRequestObserver : public PermissionRequestManager::Observer { observation_{this}; base::RunLoop loop_; bool request_shown_ = false; + bool is_view_recreate_failed_ = false; + bool is_prompt_show_failed_hidden_tab_ = false; }; } // namespace permissions diff --git a/tools/metrics/histograms/metadata/permissions/histograms.xml b/tools/metrics/histograms/metadata/permissions/histograms.xml index 1a9a2cd00844b..c4fcc65b81083 100644 --- a/tools/metrics/histograms/metadata/permissions/histograms.xml +++ b/tools/metrics/histograms/metadata/permissions/histograms.xml @@ -958,6 +958,19 @@ chromium-metrics-reviews@google.com. + + elklm@chromium.org + src/components/permissions/PERMISSIONS_OWNERS + Recorded when a permission prompt creation is in progress. + + + + + + + + elklm@chromium.org