[iOS] Hide account picker early for Save to Photos
This CL makes SaveToPhotosMediator hide the account picker as soon as an
account has been selected by the user, rather than later when the upload
has fully completed. It also ensures interaction with the Manage Storage
alert is properly reflected in the Save to Photos UI outcome histogram.
(cherry picked from commit a4d7c8dc6518b19531c9fd3f2c66519015103135)
Bug: 327400910
Change-Id: Idc167b5c03eaba9f4347d34402d582ebfa8515ab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5331637
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Commit-Queue: Quentin Pubert <qpubert@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1269591}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5360854
Cr-Commit-Position: refs/branch-heads/6312@{#506}
Cr-Branched-From: 6711dcdae48edaf98cbc6964f90fac85b7d9986e-refs/heads/main@{#1262506}
diff --git a/ios/chrome/browser/photos/model/photos_metrics.h b/ios/chrome/browser/photos/model/photos_metrics.h
index e70fe42..2dcc58a2 100644
--- a/ios/chrome/browser/photos/model/photos_metrics.h
+++ b/ios/chrome/browser/photos/model/photos_metrics.h
@@ -23,7 +23,7 @@
enum class SaveToPhotosActions {
kFailureWebStateDestroyed = 0,
kFailureUserSignedOut = 1,
- kFailureUserCancelledWithAlert =
+ kFailureUserCancelledWithTryAgainAlert =
2, // User cancelled using the "Cancel" option in the "This File Could
// not be Uploaded" alert
kFailureUserCancelledWithAccountPicker = 3,
@@ -32,7 +32,9 @@
kSuccessAndOpenPhotosApp = 6,
kSuccessAndOpenStoreKitAndAppNotInstalled = 7,
kSuccessAndOpenStoreKitAndAppInstalled = 8,
- kMaxValue = kSuccessAndOpenStoreKitAndAppInstalled,
+ kFailureOutOfStorageDidManageStorage = 9,
+ kFailureOutOfStorageDidNotManageStorage = 10,
+ kMaxValue = kFailureOutOfStorageDidNotManageStorage,
};
// LINT.ThenChange(/tools/metrics/histograms/metadata/ios/enums.xml)
diff --git a/ios/chrome/browser/ui/save_to_photos/save_to_photos_coordinator.mm b/ios/chrome/browser/ui/save_to_photos/save_to_photos_coordinator.mm
index 8feb400..1666652c 100644
--- a/ios/chrome/browser/ui/save_to_photos/save_to_photos_coordinator.mm
+++ b/ios/chrome/browser/ui/save_to_photos/save_to_photos_coordinator.mm
@@ -310,7 +310,8 @@
UIAlertAction* cancelAction =
[UIAlertAction actionWithTitle:l10n_util::GetNSString(IDS_CANCEL)
style:UIAlertActionStyleCancel
- handler:^(UIAlertAction* action){
+ handler:^(UIAlertAction* action) {
+ [weakMediator manageStorageAlertDidCancel];
}];
[_alertController addAction:manageStorageAction];
[_alertController addAction:cancelAction];
diff --git a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.h b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.h
index 61cdd5ae..35d9b79 100644
--- a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.h
+++ b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.h
@@ -98,6 +98,9 @@
// Shows the "Manage Storage" web page for `identity` in a new tab.
- (void)showManageStorageForIdentity:(id<SystemIdentity>)identity;
+// Called when the user taps "Cancel" in the "Manage Storage" alert.
+- (void)manageStorageAlertDidCancel;
+
@end
#endif // IOS_CHROME_BROWSER_UI_SAVE_TO_PHOTOS_SAVE_TO_PHOTOS_MEDIATOR_H_
diff --git a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.mm b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.mm
index c332e37..852eb77c 100644
--- a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.mm
+++ b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator.mm
@@ -96,7 +96,6 @@
NSData* _imageData;
BOOL _userTappedSuccessSnackbarButton;
base::TimeTicks _uploadStart;
- BOOL _showingAccountPicker;
BOOL _successSnackbarAppeared;
BOOL _successSnackbarDisappeared;
BOOL _uploadCompletedSuccessfully;
@@ -185,6 +184,7 @@
_identity = identity;
[self.delegate startValidationSpinnerForAccountPicker];
+ [self.delegate hideAccountPicker];
[self tryUploadImage];
}
@@ -193,14 +193,12 @@
base::UmaHistogramEnumeration(kSaveToPhotosAccountPickerActionsHistogram,
SaveToPhotosAccountPickerActions::kCancelled);
[self.delegate hideAccountPicker];
- _showingAccountPicker = NO;
return;
}
// If `_identity` is not nil while the account picker is presented, that means
// the user has already tapped "Save" for a given identity.
_photosService->CancelUpload();
- [self.delegate stopValidationSpinnerForAccountPicker];
_identity = nil;
}
@@ -210,12 +208,7 @@
kSaveToPhotosActionsHistogram,
SaveToPhotosActions::kFailureUserCancelledWithAccountPicker);
[self.delegate hideSaveToPhotos];
- return;
}
-
- // If `_identity` is not nil at this point, then an image has been uploaded
- // with this identity successfully.
- [self showSnackbarWithSuccessMessageAndOpenButton];
}
- (void)storeKitWantsToHide {
@@ -250,6 +243,17 @@
OpenNewTabCommand* newTabCommand =
[OpenNewTabCommand commandWithURLFromChrome:manageStorageURL];
[_applicationHandler openURLInNewTab:newTabCommand];
+ base::UmaHistogramEnumeration(
+ kSaveToPhotosActionsHistogram,
+ SaveToPhotosActions::kFailureOutOfStorageDidManageStorage);
+ [self.delegate hideSaveToPhotos];
+}
+
+- (void)manageStorageAlertDidCancel {
+ base::UmaHistogramEnumeration(
+ kSaveToPhotosActionsHistogram,
+ SaveToPhotosActions::kFailureOutOfStorageDidNotManageStorage);
+ [self.delegate hideSaveToPhotos];
}
#pragma mark - Private
@@ -306,7 +310,6 @@
IDS_IOS_SAVE_TO_PHOTOS_ACCOUNT_PICKER_ASK_EVERY_TIME);
[self.delegate showAccountPickerWithConfiguration:configuration
selectedIdentity:defaultIdentity];
- _showingAccountPicker = YES;
}
// Once the destination account is known, tries to upload the image using the
@@ -350,7 +353,6 @@
// upload with the same identity.
- (void)retryUploadImageWithIdentity:(id<SystemIdentity>)failureIdentity {
self.identity = failureIdentity;
- [self.delegate startValidationSpinnerForAccountPicker];
[self tryUploadImage];
}
@@ -374,8 +376,6 @@
}
base::UmaHistogramEnumeration(kSaveToPhotosUploadFailureTypeHistogram,
result.failure_type);
- __weak __typeof(self) weakSelf = self;
- [self.delegate stopValidationSpinnerForAccountPicker];
// If the user is out of storage, offer to manage their storage.
if (result.failure_type ==
PhotosServiceUploadFailureType::kUploadPhoto2NotEnoughStorage) {
@@ -384,6 +384,7 @@
return;
}
// Otherwise let the user "Try Again" with the same account.
+ __weak __typeof(self) weakSelf = self;
[self showTryAgainOrCancelAlertWithTryAgainBlock:^{
[weakSelf retryUploadImageWithIdentity:failureIdentity];
}];
@@ -394,17 +395,6 @@
base::TimeTicks::Now() - _uploadStart);
_uploadCompletedSuccessfully = YES;
- // Option 1: user did not skip the account picker so it is on the screen now,
- // and it can be hidden.
- if (_showingAccountPicker) {
- [self.delegate hideAccountPicker];
- _showingAccountPicker = NO;
- return;
- }
-
- // Option 2: user did skip the account picker and so it is not on the screen
- // now.
-
if (!_successSnackbarAppeared) {
// If the success snackbar did not appear for some reason (no progress has
// been reported), show it now.
@@ -430,12 +420,7 @@
- (void)photosServiceReportedUploadProgress:
(const PhotosService::UploadProgress&)progress {
- if (_showingAccountPicker) {
- // If the account picker is presented, do nothing here.
- return;
- }
-
- // Otherwise, if all of the image data has been uploaded, show success early.
+ // If all of the image data has been uploaded, show success early.
if (progress.total_bytes_sent == progress.total_bytes_expected_to_send) {
[self showSnackbarWithSuccessMessageAndOpenButton];
}
@@ -455,7 +440,6 @@
NSString* tryAgainTitle = l10n_util::GetNSString(
IDS_IOS_SAVE_TO_PHOTOS_THIS_FILE_COULD_NOT_BE_UPLOADED_TRY_AGAIN);
__weak __typeof(self.delegate) weakDelegate = self.delegate;
- BOOL showingAccountPicker = _showingAccountPicker;
[self.delegate
showTryAgainOrCancelAlertWithTitle:title
message:message
@@ -463,19 +447,10 @@
tryAgainAction:tryAgain
cancelTitle:cancelTitle
cancelAction:^{
- if (showingAccountPicker) {
- // Do nothing if the account picker is
- // presented. The alert will be dismissed and
- // the account picker will remain presented to
- // let the user pick another account if they
- // want to.
- return;
- }
-
base::UmaHistogramEnumeration(
kSaveToPhotosActionsHistogram,
SaveToPhotosActions::
- kFailureUserCancelledWithAlert);
+ kFailureUserCancelledWithTryAgainAlert);
[weakDelegate hideSaveToPhotos];
}];
}
diff --git a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator_unittest.mm b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator_unittest.mm
index f313c0d..a0453d2 100644
--- a/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator_unittest.mm
+++ b/ios/chrome/browser/ui/save_to_photos/save_to_photos_mediator_unittest.mm
@@ -385,17 +385,7 @@
EXPECT_NSEQ(GetTestPhotosService()->GetImageData(), GetFakeImageData());
EXPECT_EQ(GetTestPhotosService()->GetIdentity(), fake_identity_);
- // Expect that the account picker is hidden once the PhotosService is done
- // uploading.
- OCMExpect([mock_save_to_photos_mediator_delegate hideAccountPicker]);
-
- // Run until the PhotosService finishes to upload the image and check that the
- // account picker was hidden.
- task_environment_.RunUntilQuit();
- EXPECT_OCMOCK_VERIFY(mock_save_to_photos_mediator_delegate);
-
- // Expect that the success snackbar is shown once the account picker is
- // hidden.
+ // Expect that the success snackbar is shown once image is uploaded.
NSString* expected_message = l10n_util::GetNSStringF(
IDS_IOS_SAVE_TO_PHOTOS_SNACKBAR_IMAGE_SAVED_MESSAGE,
base::SysNSStringToUTF16(fake_identity_.userEmail));
@@ -407,7 +397,8 @@
messageAction:[OCMArg isNotNil]
completionAction:[OCMArg isNotNil]]);
- [mediator accountPickerWasHidden];
+ // Run until the PhotosService finishes to upload the image.
+ task_environment_.RunUntilQuit();
// Verify that the success snackbar has been shown.
EXPECT_OCMOCK_VERIFY(mock_save_to_photos_mediator_delegate);
diff --git a/tools/metrics/histograms/metadata/ios/enums.xml b/tools/metrics/histograms/metadata/ios/enums.xml
index 4e7280ac..6f3f1d0e 100644
--- a/tools/metrics/histograms/metadata/ios/enums.xml
+++ b/tools/metrics/histograms/metadata/ios/enums.xml
@@ -675,6 +675,9 @@
label="Success - StoreKit opened and app not installed (maybe dismissed
before full installation)"/>
<int value="8" label="Success - StoreKit opened and app installed"/>
+ <int value="9" label="Failure - User out-of-storage and DID manage storage"/>
+ <int value="10"
+ label="Failure - User out-of-storage and did NOT manage storage"/>
</enum>
<enum name="IOSSearchExtensionAction">