[iOS][NTP] Add error metrics for user-uploaded backgrounds
This CL introduces error handling and metrics for failures when loading
user-uploaded background images.
Previously, if a user-uploaded image failed to load from disk, it would
fail silently. This change enhances the loading mechanism to return not
just the image, but also an error status.
Two new UMA histograms are added to record image loading failures:
IOS.HomeCustomization.Background.Ntp.ImageUserUploadedFetchError
IOS.HomeCustomization.Background.RecentlyUsed.ImageUserUploadedFetchError
Bug: 447392780
Change-Id: I2a7f06f925605fcbdb00f81d987e3f18a7561027
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6991331
Reviewed-by: Robbie Gibson <rkgibson@google.com>
Commit-Queue: Cheick Cisse <cheickcisse@google.com>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1523033}
NOKEYCHECK=True
GitOrigin-RevId: 41837bc9c664f21087eb49cd19113716f77bacb5
diff --git a/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.mm b/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.mm
index 22cf676..d2e48ec 100644
--- a/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.mm
+++ b/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.mm
@@ -306,7 +306,8 @@
- (void)fetchBackgroundCustomizationUserUploadedImage:(NSString*)imagePath
completion:
- (void (^)(UIImage*))completion {
+ (UserUploadImageCompletion)
+ completion {
DCHECK(imagePath.length > 0);
CHECK(_userUploadedImageManager);
diff --git a/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator_unittest.mm b/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator_unittest.mm
index af02a77..6a43c4d 100644
--- a/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator_unittest.mm
+++ b/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator_unittest.mm
@@ -737,7 +737,9 @@
[mediator_
fetchBackgroundCustomizationUserUploadedImage:base::SysUTF8ToNSString(
image_path.value())
- completion:^(UIImage* image) {
+ completion:^(
+ UIImage* image,
+ UserUploadedImageError error) {
actual_image = image;
run_loop_ptr->Quit();
}];
diff --git a/chrome/browser/home_customization/model/BUILD.gn b/chrome/browser/home_customization/model/BUILD.gn
index 508d65a..eec56a9 100644
--- a/chrome/browser/home_customization/model/BUILD.gn
+++ b/chrome/browser/home_customization/model/BUILD.gn
@@ -92,5 +92,6 @@
deps = [
":model",
"//base",
+ "//ios/chrome/browser/home_customization/utils",
]
}
diff --git a/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.h b/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.h
index f330a97..00c6a01 100644
--- a/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.h
+++ b/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.h
@@ -29,9 +29,8 @@
void StoreUserUploadedImage(
UIImage* image,
base::OnceCallback<void(base::FilePath)> callback) override;
- void LoadUserUploadedImage(
- base::FilePath relative_image_file_path,
- base::OnceCallback<void(UIImage*)> callback) override;
+ void LoadUserUploadedImage(base::FilePath relative_image_file_path,
+ UserUploadImageCallback callback) override;
void DeleteUserUploadedImage(
base::FilePath relative_image_file_path,
base::OnceClosure completion = base::DoNothing()) override;
diff --git a/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.mm b/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.mm
index b9b8f82..c8ff4e7 100644
--- a/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.mm
+++ b/chrome/browser/home_customization/model/fake_user_uploaded_image_manager.mm
@@ -9,6 +9,7 @@
#import "base/strings/strcat.h"
#import "base/task/sequenced_task_runner.h"
#import "base/uuid.h"
+#import "ios/chrome/browser/home_customization/utils/home_customization_constants.h"
FakeUserUploadedImageManager::FakeUserUploadedImageManager(
const scoped_refptr<base::SequencedTaskRunner>& task_runner)
@@ -44,11 +45,17 @@
void FakeUserUploadedImageManager::LoadUserUploadedImage(
base::FilePath relative_image_file_path,
- base::OnceCallback<void(UIImage*)> callback) {
+ UserUploadImageCallback callback) {
task_runner_->PostTask(
- FROM_HERE,
- base::BindOnce(std::move(callback),
- LoadUserUploadedImage(relative_image_file_path)));
+ FROM_HERE, base::BindOnce(
+ [](UIImage* image, UserUploadImageCallback cb) {
+ UserUploadedImageError error =
+ image ? UserUploadedImageError::kNone
+ : UserUploadedImageError::kFailedToReadFile;
+ std::move(cb).Run(image, error);
+ },
+ LoadUserUploadedImage(relative_image_file_path),
+ std::move(callback)));
}
void FakeUserUploadedImageManager::DeleteUserUploadedImageSynchronously(
diff --git a/chrome/browser/home_customization/model/user_uploaded_image_manager.h b/chrome/browser/home_customization/model/user_uploaded_image_manager.h
index 088a56d..a73c6a4 100644
--- a/chrome/browser/home_customization/model/user_uploaded_image_manager.h
+++ b/chrome/browser/home_customization/model/user_uploaded_image_manager.h
@@ -14,6 +14,7 @@
#import "base/sequence_checker.h"
#import "base/task/sequenced_task_runner.h"
#import "components/keyed_service/core/keyed_service.h"
+#import "ios/chrome/browser/home_customization/utils/home_customization_constants.h"
@class UIImage;
@@ -35,10 +36,11 @@
UIImage* image,
base::OnceCallback<void(base::FilePath)> callback);
+ using UserUploadImageCallback =
+ base::OnceCallback<void(UIImage*, UserUploadedImageError)>;
// Loads an image previously stored at the provided relative file path.
- virtual void LoadUserUploadedImage(
- base::FilePath relative_image_file_path,
- base::OnceCallback<void(UIImage*)> callback);
+ virtual void LoadUserUploadedImage(base::FilePath relative_image_file_path,
+ UserUploadImageCallback callback);
// Deletes an image previously stored at the provided relative file path.
virtual void DeleteUserUploadedImage(
diff --git a/chrome/browser/home_customization/model/user_uploaded_image_manager.mm b/chrome/browser/home_customization/model/user_uploaded_image_manager.mm
index 1b864d5..7bcee64 100644
--- a/chrome/browser/home_customization/model/user_uploaded_image_manager.mm
+++ b/chrome/browser/home_customization/model/user_uploaded_image_manager.mm
@@ -17,13 +17,18 @@
#import "base/strings/strcat.h"
#import "base/task/sequenced_task_runner.h"
#import "base/uuid.h"
-#import "ios/chrome/browser/home_customization/utils/home_customization_constants.h"
namespace {
const char image_directory[] = "BackgroundImages";
const char image_filename_prefix[] = "background_image_";
+// A struct that holds the result of a user-uploaded image load operation.
+struct LoadImageResult {
+ UIImage* image;
+ UserUploadedImageError error;
+};
+
// Compresses and saves `image` to the provided `directory_path`. Also generates
// a UUID-based filename for `image` and returns the full save path (or an empty
// path if saving failed).
@@ -70,22 +75,22 @@
}
// Loads the image at the given path.
-UIImage* LoadImageAtPath(const base::FilePath& path) {
+LoadImageResult LoadImageAtPath(const base::FilePath& path) {
NSURL* image_url =
[NSURL fileURLWithPath:base::apple::FilePathToNSString(path)];
// Load the image from disk.
NSData* image_data = [NSData dataWithContentsOfURL:image_url];
if (!image_data) {
- return nil;
+ return {nil, UserUploadedImageError::kFailedToReadFile};
}
UIImage* image = [UIImage imageWithData:image_data];
if (!image) {
- return nil;
+ return {nil, UserUploadedImageError::kFailedToCreateImageFromData};
}
- return image;
+ return {image, UserUploadedImageError::kNone};
}
// Deletes any unused image files that exist in `directory_path` but not in
@@ -127,14 +132,19 @@
void UserUploadedImageManager::LoadUserUploadedImage(
base::FilePath relative_image_file_path,
- base::OnceCallback<void(UIImage*)> callback) {
+ UserUploadImageCallback callback) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
base::FilePath full_file_path =
storage_directory_path_.Append(relative_image_file_path);
task_runner_->PostTaskAndReplyWithResult(
FROM_HERE, base::BindOnce(&LoadImageAtPath, full_file_path),
- std::move(callback));
+ base::BindOnce(
+ [](UserUploadImageCallback original_callback,
+ LoadImageResult result) {
+ std::move(original_callback).Run(result.image, result.error);
+ },
+ std::move(callback)));
}
void UserUploadedImageManager::DeleteUserUploadedImage(
diff --git a/chrome/browser/home_customization/model/user_uploaded_image_manager_unittest.mm b/chrome/browser/home_customization/model/user_uploaded_image_manager_unittest.mm
index a7379ac..1ae9c90 100644
--- a/chrome/browser/home_customization/model/user_uploaded_image_manager_unittest.mm
+++ b/chrome/browser/home_customization/model/user_uploaded_image_manager_unittest.mm
@@ -32,11 +32,13 @@
}];
}
-// Returns a callback that capture its argument and store it to `output`.
-template <typename T>
-base::OnceCallback<void(T)> CaptureArg(T& output) {
- return base::BindOnce([](T& output, T arg) { output = arg; },
- std::ref(output));
+// Returns a callback that captures its arguments and stores them into
+// `outputs`.
+template <typename... Types>
+base::OnceCallback<void(Types...)> CaptureArgs(Types&... outputs) {
+ return base::BindOnce(
+ [](Types&... outputs, Types... args) { ((outputs = args), ...); },
+ std::ref(outputs)...);
}
} // namespace
@@ -65,7 +67,7 @@
base::FilePath relative_image_file_path;
image_manager_->StoreUserUploadedImage(
test_image,
- CaptureArg(relative_image_file_path).Then(run_loop.QuitClosure()));
+ CaptureArgs(relative_image_file_path).Then(run_loop.QuitClosure()));
run_loop.Run();
@@ -83,19 +85,21 @@
base::FilePath relative_image_file_path;
image_manager_->StoreUserUploadedImage(
test_image,
- CaptureArg(relative_image_file_path).Then(store_run_loop.QuitClosure()));
+ CaptureArgs(relative_image_file_path).Then(store_run_loop.QuitClosure()));
store_run_loop.Run();
base::RunLoop load_run_loop;
UIImage* loaded_image;
+ UserUploadedImageError error;
image_manager_->LoadUserUploadedImage(
relative_image_file_path,
- CaptureArg(loaded_image).Then(load_run_loop.QuitClosure()));
+ CaptureArgs(loaded_image, error).Then(load_run_loop.QuitClosure()));
load_run_loop.Run();
ASSERT_NSNE(nil, loaded_image);
+ EXPECT_EQ(UserUploadedImageError::kNone, error);
// The image is compressed and converted before being stored, so the bytes may
// not be identical. Compare sizes to check similarity.
@@ -106,13 +110,15 @@
TEST_F(UserUploadedImageManagerTest, LoadNonexistentImage) {
base::RunLoop run_loop;
UIImage* loaded_image = GenerateTestImage(CGSizeMake(3, 3));
+ UserUploadedImageError error = UserUploadedImageError::kNone;
image_manager_->LoadUserUploadedImage(
base::FilePath("nonexistent_image.png"),
- CaptureArg(loaded_image).Then(run_loop.QuitClosure()));
+ CaptureArgs(loaded_image, error).Then(run_loop.QuitClosure()));
run_loop.Run();
ASSERT_NSEQ(nil, loaded_image);
+ EXPECT_EQ(UserUploadedImageError::kFailedToReadFile, error);
}
// Tests that images can be deleted.
@@ -123,7 +129,7 @@
base::FilePath relative_image_file_path;
image_manager_->StoreUserUploadedImage(
test_image,
- CaptureArg(relative_image_file_path).Then(store_run_loop.QuitClosure()));
+ CaptureArgs(relative_image_file_path).Then(store_run_loop.QuitClosure()));
store_run_loop.Run();
@@ -150,11 +156,11 @@
base::RunLoop store_run_loop;
base::FilePath relative_image_file_path1;
base::FilePath relative_image_file_path2;
- image_manager_->StoreUserUploadedImage(test_image1,
- CaptureArg(relative_image_file_path1));
image_manager_->StoreUserUploadedImage(
- test_image2,
- CaptureArg(relative_image_file_path2).Then(store_run_loop.QuitClosure()));
+ test_image1, CaptureArgs(relative_image_file_path1));
+ image_manager_->StoreUserUploadedImage(
+ test_image2, CaptureArgs(relative_image_file_path2)
+ .Then(store_run_loop.QuitClosure()));
store_run_loop.Run();
diff --git a/chrome/browser/home_customization/ui/home_customization_background_configuration_mutator.h b/chrome/browser/home_customization/ui/home_customization_background_configuration_mutator.h
index 308bb14..6e95ede 100644
--- a/chrome/browser/home_customization/ui/home_customization_background_configuration_mutator.h
+++ b/chrome/browser/home_customization/ui/home_customization_background_configuration_mutator.h
@@ -7,8 +7,11 @@
#import <UIKit/UIKit.h>
+#import "ios/chrome/browser/home_customization/utils/home_customization_constants.h"
#import "url/gurl.h"
+using UserUploadImageCompletion = void (^)(UIImage*, UserUploadedImageError);
+
@protocol BackgroundCustomizationConfiguration;
// Mutator protocol for the background customization views to make model
@@ -31,7 +34,8 @@
// collection view cell when it becomes visible.
- (void)fetchBackgroundCustomizationUserUploadedImage:(NSString*)imagePath
completion:
- (void (^)(UIImage*))completion;
+ (UserUploadImageCompletion)
+ completion;
// Applies the given background configuration to the NTP.
// This method updates the background based on the provided configuration.
diff --git a/chrome/browser/home_customization/ui/home_customization_main_view_controller.mm b/chrome/browser/home_customization/ui/home_customization_main_view_controller.mm
index 217c998..f66db93 100644
--- a/chrome/browser/home_customization/ui/home_customization_main_view_controller.mm
+++ b/chrome/browser/home_customization/ui/home_customization_main_view_controller.mm
@@ -410,10 +410,17 @@
HomeCustomizationFramingCoordinates* framingCoordinates =
backgroundConfiguration.userUploadedFramingCoordinates;
__weak __typeof(self) weakSelf = self;
- void (^imageHandler)(UIImage*) = ^(UIImage* image) {
+ void (^imageHandler)(UIImage*, UserUploadedImageError) = ^(
+ UIImage* image, UserUploadedImageError error) {
[weakSelf handleLoadedUserUploadedImage:image
framingCoordinates:framingCoordinates
backgroundCell:backgroundCell];
+ if (!image) {
+ base::UmaHistogramEnumeration(
+ "IOS.HomeCustomization.Background.RecentlyUsed."
+ "ImageUserUploadedFetchError",
+ error);
+ }
};
[self.customizationMutator
fetchBackgroundCustomizationUserUploadedImage:backgroundConfiguration
diff --git a/chrome/browser/home_customization/utils/home_customization_constants.h b/chrome/browser/home_customization/utils/home_customization_constants.h
index 040ef20..f930180 100644
--- a/chrome/browser/home_customization/utils/home_customization_constants.h
+++ b/chrome/browser/home_customization/utils/home_customization_constants.h
@@ -173,8 +173,14 @@
// Failed to convert the image to JPEG.
kFailedToConvertToJPEG,
+ // Failed to read the image data from its file path.
+ kFailedToReadFile,
+
+ // Failed to create a UIImage from the loaded data.
+ kFailedToCreateImageFromData,
+
// Must be last.
- kMaxValue = kFailedToConvertToJPEG,
+ kMaxValue = kFailedToCreateImageFromData,
};
// LINT.ThenChange(/tools/metrics/histograms/metadata/ios/enums.xml:IOSUserUploadedImageError)
diff --git a/chrome/browser/ntp/ui_bundled/new_tab_page_mediator.mm b/chrome/browser/ntp/ui_bundled/new_tab_page_mediator.mm
index ca0d2d1..530804e 100644
--- a/chrome/browser/ntp/ui_bundled/new_tab_page_mediator.mm
+++ b/chrome/browser/ntp/ui_bundled/new_tab_page_mediator.mm
@@ -735,12 +735,17 @@
__weak __typeof(self) weakSelf = self;
_userUploadedImageManager->LoadUserUploadedImage(
base::FilePath(userBackground.image_path),
- base::BindOnce(^(UIImage* image) {
+ base::BindOnce(^(UIImage* image, UserUploadedImageError error) {
[weakSelf handleUserUploadedImage:image
framingCoordinates:framingCoordinates];
[traitAccessor setBoolForNewTabPageImageBackgroundTrait:YES];
[traitAccessor
setObjectForNewTabPageTrait:[NewTabPageTrait defaultValue]];
+ if (!image) {
+ base::UmaHistogramEnumeration("IOS.HomeCustomization.Background."
+ "Ntp.ImageUserUploadedFetchError",
+ error);
+ }
}));
if (initialLoad) {
base::UmaHistogramEnumeration(
