Skip to content

Commit

Permalink
ambient: clean up new url feature flag
Browse files Browse the repository at this point in the history 
BUG=None

Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Change-Id: Icd3ed9943cd826d8d710857384288b67dc5ee9a1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3564111
Reviewed-by: Jason Thai <jasontt@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/main@{#988596}
  • Loading branch information
Xiaohui Chen authored and Chromium LUCI CQ committed Apr 4, 2022
1 parent aaa4ec6 commit 343bbbc
Show file tree
Hide file tree
Showing 11 changed files with 71 additions and 174 deletions.
54 changes: 20 additions & 34 deletions ash/ambient/ambient_photo_cache.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,10 @@ std::unique_ptr<network::SimpleURLLoader> CreateSimpleURLLoader(
resource_request->method = "GET";
resource_request->credentials_mode = network::mojom::CredentialsMode::kOmit;

if (ash::features::IsAmbientModeNewUrlEnabled()) {
if (token.empty())
DVLOG(2) << "Failed to fetch access token";
else
resource_request->headers.SetHeader("Authorization", "Bearer " + token);
}
if (token.empty())
DVLOG(2) << "Failed to fetch access token";
else
resource_request->headers.SetHeader("Authorization", "Bearer " + token);

return network::SimpleURLLoader::Create(std::move(resource_request),
kAmbientPhotoCacheNetworkTag);
Expand Down Expand Up @@ -173,41 +171,29 @@ class AmbientPhotoCacheImpl : public AmbientPhotoCache {
void DownloadPhoto(
const std::string& url,
base::OnceCallback<void(std::string&&)> callback) override {
if (ash::features::IsAmbientModeNewUrlEnabled()) {
access_token_controller_.RequestAccessToken(
base::BindOnce(&AmbientPhotoCacheImpl::DownloadPhotoInternal,
weak_factory_.GetWeakPtr(), url, std::move(callback)));
} else {
DownloadPhotoInternal(url, std::move(callback), /*gaia_id=*/std::string(),
/*access_token=*/std::string());
}
access_token_controller_.RequestAccessToken(
base::BindOnce(&AmbientPhotoCacheImpl::DownloadPhotoInternal,
weak_factory_.GetWeakPtr(), url, std::move(callback)));
}

void DownloadPhotoToFile(const std::string& url,
int cache_index,
base::OnceCallback<void(bool)> callback) override {
auto file_path = GetCachePath(cache_index, root_directory_);
base::OnceClosure download_callback;
if (ash::features::IsAmbientModeNewUrlEnabled()) {
download_callback = base::BindOnce(
[](base::WeakPtr<AmbientPhotoCacheImpl> weak_ptr,
base::OnceCallback<void(const std::string&, const std::string&)>
callback) {
if (!weak_ptr)
return;
weak_ptr->access_token_controller_.RequestAccessToken(
std::move(callback));
},
weak_factory_.GetWeakPtr(),
base::BindOnce(&AmbientPhotoCacheImpl::DownloadPhotoToFileInternal,
weak_factory_.GetWeakPtr(), url, std::move(callback),
file_path));
} else {
download_callback = base::BindOnce(
&AmbientPhotoCacheImpl::DownloadPhotoToFileInternal,
weak_factory_.GetWeakPtr(), url, std::move(callback), file_path,
/*gaia_id=*/std::string(), /*access_token=*/std::string());
}
download_callback = base::BindOnce(
[](base::WeakPtr<AmbientPhotoCacheImpl> weak_ptr,
base::OnceCallback<void(const std::string&, const std::string&)>
callback) {
if (!weak_ptr)
return;
weak_ptr->access_token_controller_.RequestAccessToken(
std::move(callback));
},
weak_factory_.GetWeakPtr(),
base::BindOnce(&AmbientPhotoCacheImpl::DownloadPhotoToFileInternal,
weak_factory_.GetWeakPtr(), url, std::move(callback),
file_path));

task_runner_->PostTaskAndReply(
FROM_HERE,
Expand Down
29 changes: 2 additions & 27 deletions ash/ambient/ambient_photo_cache_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,31 +141,9 @@ TEST_F(AmbientPhotoCacheTest, SetsDataToEmptyStringWhenFilesMissing) {
}
}

TEST_F(AmbientPhotoCacheTest, DisableNewUrlDisablesTokenFetch) {
TEST_F(AmbientPhotoCacheTest, AttachTokenToDownloadRequest) {
std::string fake_url = "https://faketesturl/";

// not token request if new url feature is not enabled.
base::test::ScopedFeatureList feature;
feature.InitAndDisableFeature(features::kAmbientModeNewUrl);

EXPECT_FALSE(IsAccessTokenRequestPending());
photo_cache()->DownloadPhoto(fake_url, base::BindOnce([](std::string&&) {}));

RunUntilIdle();
EXPECT_FALSE(IsAccessTokenRequestPending());

photo_cache()->DownloadPhotoToFile(fake_url, /*cache_index=*/1,
base::BindOnce([](bool) {}));

RunUntilIdle();
EXPECT_FALSE(IsAccessTokenRequestPending());
}

TEST_F(AmbientPhotoCacheTest, EnableNewUrlAttachesTokenToDownloadRequest) {
std::string fake_url = "https://faketesturl/";
base::test::ScopedFeatureList feature;
feature.InitAndEnableFeature(features::kAmbientModeNewUrl);

photo_cache()->DownloadPhoto(fake_url, base::BindOnce([](std::string&&) {}));
RunUntilIdle();
EXPECT_TRUE(IsAccessTokenRequestPending());
Expand All @@ -182,11 +160,8 @@ TEST_F(AmbientPhotoCacheTest, EnableNewUrlAttachesTokenToDownloadRequest) {
std::string("Bearer ") + TestAmbientClient::kTestAccessToken);
}

TEST_F(AmbientPhotoCacheTest,
EnableNewUrlAttachesTokenToDownloadToFileRequest) {
TEST_F(AmbientPhotoCacheTest, AttachTokenToDownloadToFileRequest) {
std::string fake_url = "https://faketesturl/";
base::test::ScopedFeatureList feature;
feature.InitAndEnableFeature(features::kAmbientModeNewUrl);

photo_cache()->DownloadPhotoToFile(fake_url, /*cache_index=*/1,
base::BindOnce([](bool) {}));
Expand Down
5 changes: 2 additions & 3 deletions ash/ambient/backdrop/ambient_backend_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,7 @@ void AmbientBackendControllerImpl::FetchScreenUpdateInfoInternal(
std::string client_id = GetClientId();
BackdropClientConfig::Request request =
backdrop_client_config_.CreateFetchScreenUpdateRequest(
num_topics, gaia_id, access_token, client_id,
/*use_new_url=*/features::IsAmbientModeNewUrlEnabled());
num_topics, gaia_id, access_token, client_id);
auto resource_request = CreateResourceRequest(request);

DCHECK(!screen_size.IsEmpty());
Expand Down Expand Up @@ -603,7 +602,7 @@ void AmbientBackendControllerImpl::FetchPersonalAlbumsInternal(
BackdropClientConfig::Request request =
backdrop_client_config_.CreateFetchPersonalAlbumsRequest(
banner_width, banner_height, num_albums, resume_token, gaia_id,
access_token, features::IsAmbientModeNewUrlEnabled());
access_token);
std::unique_ptr<network::ResourceRequest> resource_request =
CreateResourceRequest(request);
auto backdrop_url_loader = std::make_unique<BackdropURLLoader>();
Expand Down
8 changes: 0 additions & 8 deletions ash/constants/ash_features.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,10 +116,6 @@ const base::Feature kAmbientModeDevUseProdFeature{
const base::Feature kAmbientModePhotoPreviewFeature{
"ChromeOSAmbientModePhotoPreview", base::FEATURE_ENABLED_BY_DEFAULT};

// Controls whether to fetch ambient mode images using new url format.
const base::Feature kAmbientModeNewUrl{"ChromeOSAmbientModeNewUrl",
base::FEATURE_ENABLED_BY_DEFAULT};

// Controls whether to enable ARC account restrictions.
const base::Feature kArcAccountRestrictions{"ArcAccountRestrictions",
base::FEATURE_ENABLED_BY_DEFAULT};
Expand Down Expand Up @@ -1503,10 +1499,6 @@ bool IsAmbientModePhotoPreviewEnabled() {
return base::FeatureList::IsEnabled(kAmbientModePhotoPreviewFeature);
}

bool IsAmbientModeNewUrlEnabled() {
return base::FeatureList::IsEnabled(kAmbientModeNewUrl);
}

bool IsAppNotificationsPageEnabled() {
return base::FeatureList::IsEnabled(kOsSettingsAppNotificationsPage);
}
Expand Down
2 changes: 0 additions & 2 deletions ash/constants/ash_features.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kAmbientModeDevUseProdFeature;
COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kAmbientModePhotoPreviewFeature;
COMPONENT_EXPORT(ASH_CONSTANTS) extern const base::Feature kAmbientModeNewUrl;
COMPONENT_EXPORT(ASH_CONSTANTS)
extern const base::Feature kArcAccountRestrictions;
COMPONENT_EXPORT(ASH_CONSTANTS)
Expand Down Expand Up @@ -566,7 +565,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAmbientModeAnimationEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAmbientModeDevUseProdEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAmbientModeEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAmbientModePhotoPreviewEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAmbientModeNewUrlEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsAppNotificationsPageEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsArcInputOverlayEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsArcNetworkDiagnosticsButtonEnabled();
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/about_flags.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7854,12 +7854,6 @@ const FeatureEntry kFeatureEntries[] = {
FEATURE_VALUE_TYPE(features::kAppDiscoveryForOobe)},
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

#if BUILDFLAG(IS_CHROMEOS_ASH)
{"ambient-mode-new-url", flag_descriptions::kAmbientModeNewUrlName,
flag_descriptions::kAmbientModeNewUrlDescription, kOsCrOS,
FEATURE_VALUE_TYPE(ash::features::kAmbientModeNewUrl)},
#endif // BUILDFLAG(IS_CHROMEOS_ASH)

#if BUILDFLAG(IS_ANDROID)
{"request-desktop-site-global",
flag_descriptions::kRequestDesktopSiteGlobalName,
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/flag_descriptions.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4081,12 +4081,6 @@ const char kAmbientModeAnimationDescription[] =
"slideshow) when entering ambient mode. Currently, there is only one "
"animation theme available (feel the breeze).";

const char kAmbientModeNewUrlName[] =
"Use new backend URL format for ChromeOS Screensaver";
const char kAmbientModeNewUrlDescription[] =
"Use new backend URL format for ChromeOS Screensaver. This helps with "
"testing new backend migration.";

const char kAppDiscoveryForOobeName[] =
"OOBE app recommendations with App Discovery Service.";
const char kAppDiscoveryForOobeDescription[] =
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/flag_descriptions.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2343,9 +2343,6 @@ extern const char kAlwaysEnableHdcpType1[];
extern const char kAmbientModeAnimationName[];
extern const char kAmbientModeAnimationDescription[];

extern const char kAmbientModeNewUrlName[];
extern const char kAmbientModeNewUrlDescription[];

extern const char kAppDiscoveryForOobeName[];
extern const char kAppDiscoveryForOobeDescription[];

Expand Down
39 changes: 16 additions & 23 deletions chrome/browser/ui/ash/ambient/ambient_client_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,29 +176,22 @@ void AmbientClientImpl::RequestAccessToken(GetAccessTokenCallback callback) {
void AmbientClientImpl::DownloadImage(
const std::string& url,
ash::ImageDownloader::DownloadCallback callback) {
if (ash::features::IsAmbientModeNewUrlEnabled()) {
RequestAccessToken(base::BindOnce(
[](const std::string& url,
ash::ImageDownloader::DownloadCallback callback,
const std::string& gaia_id, const std::string& access_token,
const base::Time& expiration_time) {
if (access_token.empty()) {
std::move(callback).Run({});
return;
}
net::HttpRequestHeaders headers;
headers.SetHeader("Authorization", "Bearer " + access_token);
ash::ImageDownloader::Get()->Download(
GURL(url), kAmbientClientNetworkTag, headers,
/*credentials_account_id=*/absl::nullopt, std::move(callback));
},
url, std::move(callback)));
} else {
ash::ImageDownloader::Get()->Download(
GURL(url), kAmbientClientNetworkTag,
/*additional_headers=*/{},
/*credentials_account_id=*/absl::nullopt, std::move(callback));
}
RequestAccessToken(base::BindOnce(
[](const std::string& url,
ash::ImageDownloader::DownloadCallback callback,
const std::string& gaia_id, const std::string& access_token,
const base::Time& expiration_time) {
if (access_token.empty()) {
std::move(callback).Run({});
return;
}
net::HttpRequestHeaders headers;
headers.SetHeader("Authorization", "Bearer " + access_token);
ash::ImageDownloader::Get()->Download(
GURL(url), kAmbientClientNetworkTag, headers,
/*credentials_account_id=*/absl::nullopt, std::move(callback));
},
url, std::move(callback)));
}

scoped_refptr<network::SharedURLLoaderFactory>
Expand Down
17 changes: 1 addition & 16 deletions chrome/browser/ui/ash/ambient/ambient_client_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,18 +119,6 @@ TEST_F(AmbientClientImplTest, DisallowedByEmailDomain) {
}

TEST_F(AmbientClientImplTest, DownloadImage) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndDisableFeature(ash::features::kAmbientModeNewUrl);
ambient_client().DownloadImage("test_url", base::DoNothing());
base::RunLoop().RunUntilIdle();

EXPECT_TRUE(image_downloader().last_request_headers().IsEmpty());
}

TEST_F(AmbientClientImplTest, DownloadImageWithNewUrl) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(ash::features::kAmbientModeNewUrl);

identity_test_env()->SetAutomaticIssueOfAccessTokens(true);
AddAndLoginUser(AccountId::FromUserEmailGaiaId(
profile()->GetProfileUserName(), kTestGaiaId));
Expand All @@ -143,10 +131,7 @@ TEST_F(AmbientClientImplTest, DownloadImageWithNewUrl) {
EXPECT_EQ("Bearer access_token", out);
}

TEST_F(AmbientClientImplTest, DownloadImageWithNewUrlMultipleTimes) {
base::test::ScopedFeatureList scoped_feature_list;
scoped_feature_list.InitAndEnableFeature(ash::features::kAmbientModeNewUrl);

TEST_F(AmbientClientImplTest, DownloadImageMultipleTimes) {
identity_test_env()->SetAutomaticIssueOfAccessTokens(true);
AddAndLoginUser(AccountId::FromUserEmailGaiaId(
profile()->GetProfileUserName(), kTestGaiaId));
Expand Down

0 comments on commit 343bbbc

Please sign in to comment.