From 7e0a193e56df046324f6eac4e6e3daef8fcc5923 Mon Sep 17 00:00:00 2001 From: "Angus L. M. McLean IV" Date: Sat, 26 Mar 2022 09:07:23 +0000 Subject: [PATCH] personalization: Take no action on failed Google Photos wallpaper request Currently, if a request fails when trying to fetch the URL for a Google Photos wallpaper, we reset to the default wallpaper. We only want to reset the wallpaper if the photo does not exist. We don't want to reset the wallpaper if the request failed entirely, so we need a way to differentiate between those two error states in the controller. Otherwise, the user disconnecting from WiFi could cause the wallpaper to reset unnecessarily. This would be a rare occurrence right now, but once an upcoming CL enables periodic staleness checking for Google Photos wallpapers, it would happen any time a user is disconnected from WiFi for a substantial period of time. Bug: b:226198384 Change-Id: I1e60971a1ace376cf6b26a421fda2f0cdf072d39 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3543355 Reviewed-by: Colin Kincaid Reviewed-by: David Black Reviewed-by: Xiaohui Chen Commit-Queue: Angus McLean Cr-Commit-Position: refs/heads/main@{#985654} --- .../wallpaper/wallpaper_controller_client.h | 3 ++- .../test_wallpaper_controller_client.cc | 10 ++++--- ash/wallpaper/wallpaper_controller_impl.cc | 15 ++++++++--- ash/wallpaper/wallpaper_controller_impl.h | 3 ++- .../wallpaper_handlers/wallpaper_handlers.cc | 26 ++++++++++++++++++- .../wallpaper_handlers/wallpaper_handlers.h | 5 ++++ ...onalization_app_wallpaper_provider_impl.cc | 6 +++-- ...sonalization_app_wallpaper_provider_impl.h | 4 +-- .../ash/wallpaper_controller_client_impl.cc | 13 +++++++--- 9 files changed, 66 insertions(+), 19 deletions(-) diff --git a/ash/public/cpp/wallpaper/wallpaper_controller_client.h b/ash/public/cpp/wallpaper/wallpaper_controller_client.h index 3339eee8686a6d..2e404bda119ddc 100644 --- a/ash/public/cpp/wallpaper/wallpaper_controller_client.h +++ b/ash/public/cpp/wallpaper/wallpaper_controller_client.h @@ -71,7 +71,8 @@ class ASH_PUBLIC_EXPORT WallpaperControllerClient { FetchImagesForCollectionCallback callback) = 0; using FetchGooglePhotosPhotoCallback = base::OnceCallback)>; + mojo::StructPtr, + bool success)>; virtual void FetchGooglePhotosPhoto( const AccountId& account_id, const std::string& id, diff --git a/ash/wallpaper/test_wallpaper_controller_client.cc b/ash/wallpaper/test_wallpaper_controller_client.cc index cac9f495de1b5a..3dbf22639a1b63 100644 --- a/ash/wallpaper/test_wallpaper_controller_client.cc +++ b/ash/wallpaper/test_wallpaper_controller_client.cc @@ -101,11 +101,13 @@ void TestWallpaperControllerClient::FetchGooglePhotosPhoto( base::Time::Exploded exploded_time{2011, 6, 3, 15, 12, 0, 0, 0}; DCHECK(base::Time::FromUTCExploded(exploded_time, &time)); if (fetch_google_photos_photo_fails_) { - std::move(callback).Run(nullptr); + std::move(callback).Run(nullptr, /*success=*/false); } else { - std::move(callback).Run(personalization_app::mojom::GooglePhotosPhoto::New( - id, "test_name", base::TimeFormatFriendlyDate(time), - GURL("https://google.com/picture.png"))); + std::move(callback).Run( + personalization_app::mojom::GooglePhotosPhoto::New( + id, "test_name", base::TimeFormatFriendlyDate(time), + GURL("https://google.com/picture.png")), + /*success=*/true); } } diff --git a/ash/wallpaper/wallpaper_controller_impl.cc b/ash/wallpaper/wallpaper_controller_impl.cc index 8722e0e164a84d..99108b10a5149d 100644 --- a/ash/wallpaper/wallpaper_controller_impl.cc +++ b/ash/wallpaper/wallpaper_controller_impl.cc @@ -2203,10 +2203,17 @@ void WallpaperControllerImpl::SetOnlineWallpaperImpl( void WallpaperControllerImpl::OnGooglePhotosPhotoFetched( const GooglePhotosWallpaperParams& params, SetWallpaperCallback callback, - ash::personalization_app::mojom::GooglePhotosPhotoPtr photo) { - // TODO(angusmclean): Detect whether the photo doesn't exist or if the request - // simply failed. If the request failed, load from cache or exit as - // appropriate. If the photo doesn't exist, continue to below. + ash::personalization_app::mojom::GooglePhotosPhotoPtr photo, + bool success) { + // It should be impossible for us to get back a photo successfully from + // a request that failed. + DCHECK(success || !photo); + // If the request failed, there's nothing to do here, since we can't update + // the wallpaper but also don't want to delete the cache. + if (!success) { + std::move(callback).Run(false); + return; + } if (photo.is_null()) { // The photo doesn't exist, or has been deleted. If this photo is the // current wallpaper, we need to reset to the default. diff --git a/ash/wallpaper/wallpaper_controller_impl.h b/ash/wallpaper/wallpaper_controller_impl.h index 2e9167a97398ec..23fcc53b704f86 100644 --- a/ash/wallpaper/wallpaper_controller_impl.h +++ b/ash/wallpaper/wallpaper_controller_impl.h @@ -469,7 +469,8 @@ class ASH_EXPORT WallpaperControllerImpl void OnGooglePhotosPhotoFetched( const GooglePhotosWallpaperParams& params, SetWallpaperCallback callback, - ash::personalization_app::mojom::GooglePhotosPhotoPtr photo); + ash::personalization_app::mojom::GooglePhotosPhotoPtr photo, + bool success); void GetGooglePhotosWallpaperFromCacheOrDownload( const GooglePhotosWallpaperParams& params, diff --git a/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.cc b/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.cc index bc408c070d7965..47c0fceabf0fd7 100644 --- a/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.cc +++ b/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.cc @@ -37,6 +37,7 @@ #include "google_apis/gaia/google_service_auth_error.h" #include "net/base/load_flags.h" #include "net/base/url_util.h" +#include "net/http/http_status_code.h" #include "services/network/public/cpp/resource_request.h" #include "services/network/public/cpp/shared_url_loader_factory.h" #include "services/network/public/cpp/simple_url_loader.h" @@ -591,6 +592,12 @@ void GooglePhotosFetcher::AddRequestAndStartIfNecessary( signin::ConsentLevel::kSignin); } +template +absl::optional GooglePhotosFetcher::CreateErrorResponse( + int error_code) { + return absl::nullopt; +} + template void GooglePhotosFetcher::OnTokenReceived( const GURL& service_url, @@ -635,11 +642,15 @@ void GooglePhotosFetcher::OnJsonReceived( LOG(ERROR) << "Google Photos API request to " << service_url.spec() << " failed."; auto* response_info = url_loaders_[service_url]->ResponseInfo(); + absl::optional error_response; if (response_info && response_info->headers) { LOG(ERROR) << "HTTP response code: " << response_info->headers->response_code(); + error_response = + CreateErrorResponse(response_info->headers->response_code()); + return; } - OnResponseReady(service_url, start_time, absl::nullopt); + OnResponseReady(service_url, start_time, std::move(error_response)); return; } @@ -859,6 +870,19 @@ void GooglePhotosPhotosFetcher::AddRequestAndStartIfNecessary( std::move(callback)); } +absl::optional GooglePhotosPhotosFetcher::CreateErrorResponse( + int error_code) { + // If the server gives us 404, that means the request succeeded, but no + // photos with the given attributes exist. We return an empty list of photos + // to communicate this back to the caller. + if (error_code == net::HTTP_NOT_FOUND) { + absl::optional empty_list(base::Value::Type::DICT); + empty_list->SetKey("item", base::Value(base::Value::Type::LIST)); + return empty_list; + } + return absl::nullopt; +} + GooglePhotosPhotosCbkArgs GooglePhotosPhotosFetcher::ParseResponse( const base::Value::Dict* response) { auto parsed_response = diff --git a/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.h b/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.h index f64560ec582683..61a3466e08e7b8 100644 --- a/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.h +++ b/chrome/browser/ash/wallpaper_handlers/wallpaper_handlers.h @@ -176,6 +176,10 @@ class GooglePhotosFetcher : public signin::IdentityManager::Observer { // Returns the count of results contained within the specified `result`. virtual absl::optional GetResultCount(const T& result) = 0; + // Contains logic for different HTTP error codes that we receive, as they can + // carry information on the state of the user's Google Photos library. + virtual absl::optional CreateErrorResponse(int error_code); + private: void OnTokenReceived(const GURL& service_url, base::TimeTicks start_time, @@ -308,6 +312,7 @@ class GooglePhotosPhotosFetcher protected: // GooglePhotosFetcher: + absl::optional CreateErrorResponse(int error_code) override; GooglePhotosPhotosCbkArgs ParseResponse( const base::Value::Dict* response) override; absl::optional GetResultCount( diff --git a/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.cc b/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.cc index 3299c8adc67e73..edd19d097c95ad 100644 --- a/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.cc +++ b/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.cc @@ -762,10 +762,12 @@ void PersonalizationAppWallpaperProviderImpl::FindAttributionInCollection( void PersonalizationAppWallpaperProviderImpl::SendGooglePhotosAttribution( const ash::WallpaperInfo& info, const GURL& wallpaper_data_url, - mojo::StructPtr photo) { + mojo::StructPtr photo, + bool success) { std::vector attribution; - if (!photo.is_null()) + if (!photo.is_null()) { attribution.push_back(photo->name); + } NotifyWallpaperChanged(ash::personalization_app::mojom::CurrentWallpaper::New( wallpaper_data_url, attribution, info.layout, info.type, /*key=*/info.location)); diff --git a/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.h b/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.h index 8e4ba0c22e1671..9d58d4750ffd85 100644 --- a/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.h +++ b/chrome/browser/ash/web_applications/personalization_app/personalization_app_wallpaper_provider_impl.h @@ -223,8 +223,8 @@ class PersonalizationAppWallpaperProviderImpl void SendGooglePhotosAttribution( const ash::WallpaperInfo& info, const GURL& wallpaper_data_url, - mojo::StructPtr - photo); + mojo::StructPtr photo, + bool success); // Called when the user sets an image, or cancels/confirms preview wallpaper. // If a new image is set in preview mode, will minimize all windows except the diff --git a/chrome/browser/ui/ash/wallpaper_controller_client_impl.cc b/chrome/browser/ui/ash/wallpaper_controller_client_impl.cc index 53fc6aa922a4ed..056ab21df892ed 100644 --- a/chrome/browser/ui/ash/wallpaper_controller_client_impl.cc +++ b/chrome/browser/ui/ash/wallpaper_controller_client_impl.cc @@ -829,11 +829,16 @@ void WallpaperControllerClientImpl::OnGooglePhotosPhotoFetched( FetchGooglePhotosPhotoCallback callback, ash::personalization_app::mojom::FetchGooglePhotosPhotosResponsePtr response) { - if (!response->photos.has_value() || response->photos.value().size() != 1) { - std::move(callback).Run(nullptr); - return; + // If we have a `GooglePhotosPhoto`, pass that along. Otherwise, indicate to + // `callback` whether the the request succeeded or failed, since `callback` + // can take action if the `GooglePhotosPhoto` with the given id has been + // deleted. + if (response->photos.has_value() && response->photos.value().size() == 1) { + std::move(callback).Run(std::move(response->photos.value()[0]), + /*success=*/true); + } else { + std::move(callback).Run(nullptr, /*success=*/response->photos.has_value()); } - std::move(callback).Run(std::move(response->photos.value()[0])); } void WallpaperControllerClientImpl::ObserveVolumeManagerForAccountId(