From 97a0edf4f0e680048c1848c3c044c0c2e7cfe9ef Mon Sep 17 00:00:00 2001 From: Shakti Sahu Date: Mon, 5 Jun 2023 23:20:15 +0000 Subject: [PATCH] [CSC] Auto-refresh on sign-in state change This CL 1. Disables auto-refresh on tab visibility change. 2. Auto-refreshes whenever the user's sign-in state change. We ignore the the sync state changes as it is already observed through unified consent manager observer. (cherry picked from commit 7feb4190c5461ccae9b94ec5d4e34f9d08c28e0a) Bug: b/277800496, 1449021 Change-Id: I8bfe4ddf182bfb97f009672db37137d15a0fe981 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4581841 Reviewed-by: David Roger Commit-Queue: Shakti Sahu Cr-Original-Commit-Position: refs/heads/main@{#1152401} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590477 Reviewed-by: Michael Crouse Auto-Submit: Shakti Sahu Commit-Queue: Michael Crouse Cr-Commit-Position: refs/branch-heads/5790@{#383} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../companion_page_browsertest.cc | 40 ++++--------------- .../companion/companion_page_handler.cc | 36 ++++++++--------- .../companion/companion_page_handler.h | 21 +++++++--- 3 files changed, 40 insertions(+), 57 deletions(-) diff --git a/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc b/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc index e728b79f5c736..eab9b1a8aace2 100644 --- a/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc +++ b/chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc @@ -584,7 +584,8 @@ IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, AutoRefreshOnMsbb) { EXPECT_EQ(proto->page_url(), CreateUrl(kHost, kRelativeUrl1)); } -IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, AutoRefreshOnTabForegrounded) { +IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, + AutoRefreshOnSigninStateChange) { EnableSignInMsbbExps(/*signed_in=*/false, /*msbb=*/false, /*exps=*/false); // Load a page on the active tab and open companion side panel @@ -595,6 +596,7 @@ IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, AutoRefreshOnTabForegrounded) { WaitForCompanionToBeLoaded(); EXPECT_EQ(side_panel_coordinator()->GetCurrentEntryId(), SidePanelEntry::Id::kSearchCompanion); + auto* companion_web_contents = GetCompanionWebContents(browser()); // Inspect the URL from the proto. This will reset the proto. auto proto = GetLastCompanionProtoFromUrlLoad(); @@ -604,43 +606,17 @@ IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, AutoRefreshOnTabForegrounded) { // Navigate to a new tab. chrome::NewTab(browser()); - // Go back to the original tab. This should refresh the companion. - browser()->tab_strip_model()->ActivateTabAt(0); - WaitForCompanionIframeReload(); + // Sign-in to chrome. The companion should refresh automatically even though + // it's in background. + content::TestNavigationObserver nav_observer(companion_web_contents, 1); + EnableSignInMsbbExps(/*signed_in=*/true, /*msbb=*/false, /*exps=*/false); + nav_observer.Wait(); proto = GetLastCompanionProtoFromUrlLoad(); EXPECT_TRUE(proto.has_value()); EXPECT_TRUE(proto->page_url().empty()); } -IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, - DontAutoRefreshIfHasAllPermissions) { - EnableSignInMsbbExps(/*signed_in=*/true, /*msbb=*/true, /*exps=*/true); - - // Load a page on the active tab and open companion side panel - ASSERT_TRUE( - ui_test_utils::NavigateToURL(browser(), CreateUrl(kHost, kRelativeUrl1))); - side_panel_coordinator()->Show(SidePanelEntry::Id::kSearchCompanion); - - WaitForCompanionToBeLoaded(); - EXPECT_EQ(side_panel_coordinator()->GetCurrentEntryId(), - SidePanelEntry::Id::kSearchCompanion); - - // Inspect the URL from the proto. This will reset the proto. - auto proto = GetLastCompanionProtoFromUrlLoad(); - EXPECT_TRUE(proto.has_value()); - EXPECT_FALSE(proto->page_url().empty()); - EXPECT_EQ(proto->page_url(), CreateUrl(kHost, kRelativeUrl1)); - - // Navigate to a new tab. - chrome::NewTab(browser()); - - // Go back to the original tab. This should not refresh the companion. - browser()->tab_strip_model()->ActivateTabAt(0); - proto = GetLastCompanionProtoFromUrlLoad(); - EXPECT_FALSE(proto.has_value()); -} - IN_PROC_BROWSER_TEST_F(CompanionPageBrowserTest, SamePageNavigationsAreSkipped) { EnableSignInMsbbExps(/*signed_in=*/true, /*msbb=*/true, /*exps=*/true); diff --git a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc index c9c505a4c2b15..3820039b4fd84 100644 --- a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc +++ b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc @@ -14,6 +14,7 @@ #include "chrome/browser/companion/text_finder/text_highlighter_manager.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search/search.h" +#include "chrome/browser/signin/identity_manager_factory.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_window.h" #include "chrome/browser/ui/side_panel/companion/companion_side_panel_controller_utils.h" @@ -53,11 +54,12 @@ CompanionPageHandler::CompanionPageHandler( consent_helper_(unified_consent::UrlKeyedDataCollectionConsentHelper:: NewAnonymizedDataCollectionConsentHelper( GetProfile()->GetPrefs())) { - consent_helper_->AddObserver(this); + identity_manager_observation_.Observe( + IdentityManagerFactory::GetForProfile(GetProfile())); + consent_helper_observation_.Observe(consent_helper_.get()); } CompanionPageHandler::~CompanionPageHandler() { - consent_helper_->RemoveObserver(this); if (web_contents() && !web_contents()->IsBeingDestroyed()) { auto* tab_helper = companion::CompanionTabHelper::FromWebContents(web_contents()); @@ -65,6 +67,18 @@ CompanionPageHandler::~CompanionPageHandler() { } } +void CompanionPageHandler::OnPrimaryAccountChanged( + const signin::PrimaryAccountChangeEvent& event_details) { + // We only care about the sign-in state changes. Sync state change is already + // captured through consent helper observer. + if (event_details.GetEventTypeFor(signin::ConsentLevel::kSignin) == + signin::PrimaryAccountChangeEvent::Type::kNone) { + return; + } + + NotifyURLChanged(/*is_full_reload=*/true); +} + void CompanionPageHandler::OnUrlKeyedDataCollectionConsentStateChanged( unified_consent::UrlKeyedDataCollectionConsentHelper* consent_helper) { NotifyURLChanged(/*is_full_reload=*/true); @@ -110,16 +124,6 @@ void CompanionPageHandler::DidFinishNavigation( NotifyURLChanged(/*is_full_reload=*/false); } -void CompanionPageHandler::OnVisibilityChanged(content::Visibility visibility) { - // Refresh companion whenever the tab is back to foreground state. - // Do this only when the user didn't have all the access permissions to begin - // with. - if (visibility == content::Visibility::VISIBLE && - !MeetsAllAccessRequirements()) { - NotifyURLChanged(/*is_full_reload=*/true); - } -} - void CompanionPageHandler::ShowUI() { if (auto embedder = companion_untrusted_ui_->embedder()) { embedder->ShowUI(); @@ -181,14 +185,6 @@ void CompanionPageHandler::NotifyURLChanged(bool is_full_reload) { } } -bool CompanionPageHandler::MeetsAllAccessRequirements() { - auto* pref_service = GetProfile()->GetPrefs(); - bool is_exps_opted_in = pref_service->GetBoolean(kExpsOptInStatusGrantedPref); - bool is_msbb_enabled = - IsUserPermittedToSharePageInfoWithCompanion(pref_service); - return signin_delegate_->IsSignedIn() && is_msbb_enabled && is_exps_opted_in; -} - void CompanionPageHandler::OnImageQuery( side_panel::mojom::ImageQuery image_query) { GURL modified_upload_url = url_builder_->AppendCompanionParamsToURL( diff --git a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.h b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.h index 1d2b8501c3b13..33d9d4e8396c4 100644 --- a/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.h +++ b/chrome/browser/ui/webui/side_panel/companion/companion_page_handler.h @@ -7,10 +7,12 @@ #include "base/memory/raw_ptr.h" #include "base/memory/weak_ptr.h" +#include "base/scoped_observation.h" #include "chrome/browser/companion/core/constants.h" #include "chrome/browser/companion/core/mojom/companion.mojom.h" #include "chrome/browser/ui/side_panel/side_panel_enums.h" #include "components/lens/buildflags.h" +#include "components/signin/public/identity_manager/identity_manager.h" #include "components/unified_consent/url_keyed_data_collection_consent_helper.h" #include "content/public/browser/navigation_handle.h" #include "content/public/browser/web_contents_observer.h" @@ -32,6 +34,7 @@ class SigninDelegate; class CompanionPageHandler : public side_panel::mojom::CompanionPageHandler, public content::WebContentsObserver, + public signin::IdentityManager::Observer, public unified_consent::UrlKeyedDataCollectionConsentHelper::Observer { public: CompanionPageHandler( @@ -65,7 +68,10 @@ class CompanionPageHandler // content::WebContentsObserver overrides. void DidFinishNavigation( content::NavigationHandle* navigation_handle) override; - void OnVisibilityChanged(content::Visibility visibility) override; + + // IdentityManager::Observer overrides. + void OnPrimaryAccountChanged( + const signin::PrimaryAccountChangeEvent& event) override; // UrlKeyedDataCollectionConsentHelper::Observer overrides. void OnUrlKeyedDataCollectionConsentStateChanged( @@ -95,10 +101,6 @@ class CompanionPageHandler void DidFinishFindingCqTexts( const std::vector>& text_found_vec); - // Helper method to determine whether the user has met all the access - // requirements, i.e. signed in, msbb enabled, and has exps access. - bool MeetsAllAccessRequirements(); - mojo::Receiver receiver_; mojo::Remote page_; raw_ptr companion_untrusted_ui_ = nullptr; @@ -114,6 +116,15 @@ class CompanionPageHandler // The current URL of the main frame. GURL page_url_; + // Observers for sign-in and MSBB status. + base::ScopedObservation + identity_manager_observation_{this}; + base::ScopedObservation< + unified_consent::UrlKeyedDataCollectionConsentHelper, + unified_consent::UrlKeyedDataCollectionConsentHelper::Observer> + consent_helper_observation_{this}; + base::WeakPtrFactory weak_ptr_factory_{this}; }; } // namespace companion