Skip to content

Commit

Permalink
[CSC] Auto-refresh on sign-in state change
Browse files Browse the repository at this point in the history 
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 7feb419)

Bug: b/277800496, 1449021
Change-Id: I8bfe4ddf182bfb97f009672db37137d15a0fe981
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4581841
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Shakti Sahu <shaktisahu@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1152401}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4590477
Reviewed-by: Michael Crouse <mcrouse@chromium.org>
Auto-Submit: Shakti Sahu <shaktisahu@chromium.org>
Commit-Queue: Michael Crouse <mcrouse@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#383}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Shakti Sahu authored and Chromium LUCI CQ committed Jun 5, 2023
1 parent 409d62d commit 97a0edf
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 57 deletions.
40 changes: 8 additions & 32 deletions chrome/browser/ui/views/side_panel/search_companion/companion_page_browsertest.cc
View file
Expand Up @@ -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
Expand All @@ -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();
Expand All @@ -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);
Expand Down
36 changes: 16 additions & 20 deletions chrome/browser/ui/webui/side_panel/companion/companion_page_handler.cc
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -53,18 +54,31 @@ 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());
tab_helper->OnCompanionSidePanelClosed();
}
}

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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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(
Expand Down
21 changes: 16 additions & 5 deletions chrome/browser/ui/webui/side_panel/companion/companion_page_handler.h
View file
Expand Up @@ -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"
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -95,10 +101,6 @@ class CompanionPageHandler
void DidFinishFindingCqTexts(
const std::vector<std::pair<std::string, bool>>& 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<side_panel::mojom::CompanionPageHandler> receiver_;
mojo::Remote<side_panel::mojom::CompanionPage> page_;
raw_ptr<CompanionSidePanelUntrustedUI> companion_untrusted_ui_ = nullptr;
Expand All @@ -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<signin::IdentityManager,
signin::IdentityManager::Observer>
identity_manager_observation_{this};
base::ScopedObservation<
unified_consent::UrlKeyedDataCollectionConsentHelper,
unified_consent::UrlKeyedDataCollectionConsentHelper::Observer>
consent_helper_observation_{this};

base::WeakPtrFactory<CompanionPageHandler> weak_ptr_factory_{this};
};
} // namespace companion
Expand Down

0 comments on commit 97a0edf

Please sign in to comment.