[iOS] Track user background selection outcome and small refactoring [chromium/src : main]
Robbie Gibson (Gerrit)
Robbie Gibson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Robbie Gibson
| Code-Review | +1 |
2 comments
self.backgroundSelectionOutcome = BackgroundSelectionOutcome::kApplied;This will also be set if the user selects a recently used background and then dismisses the menu. I think that's fine? But just wanted to confirm.
Related details
- Cheick Cisse
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cheick Cisse (Gerrit)
Cheick Cisse added 1 comment![]( "Open in Gerrit")
self.backgroundSelectionOutcome = BackgroundSelectionOutcome::kApplied;This will also be set if the user selects a recently used background and then dismisses the menu. I think that's fine? But just wanted to confirm.
I think so? It would be selection applied.
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robbie Gibson (Gerrit)
Robbie Gibson added 3 comments![]( "Open in Gerrit")
HomeCustomizationBackgroundPickerActionSheetSoordinator becausenit spelling "Soordinator"
HomeCustomizationBackgroundPhotoPickerCoordinator updatesI'm a little confused here. I think the photo framing flow doesn't use the `HomeCustomizationBackgroundConfigurationMediator` at all, just the `HomeCustomizationBackgroundPhotoFramingMediator`, so would it be easier to track the photo flow stuff there?
BackgroundSelectionOutcome::kCanceledAfterSelection;Here, for example, `kCanceledAfterSelection` has a slightly different meaning for the photo flow vs color or gallery flows, right?
For the photo flow here, it means the user chose a photo from their library, but then cancelled. Vs color or gallery where they did select a color/image, saw how it looked on the background and then cancelled.
I guess in some sense they're similar, although to me it feels slightly different.
Related details
- Cheick Cisse
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cheick Cisse (Gerrit)
Cheick Cisse added 2 comments![]( "Open in Gerrit")
HomeCustomizationBackgroundPhotoPickerCoordinator updatesI'm a little confused here. I think the photo framing flow doesn't use the `HomeCustomizationBackgroundConfigurationMediator` at all, just the `HomeCustomizationBackgroundPhotoFramingMediator`, so would it be easier to track the photo flow stuff there?
Yeah it's a bit confusing I agree. The photo framing actually does interclty interact with HomeCustomizationBackgroundConfigurationMediator. HomeCustomizationBackgroundPhotoFramingMediator -> discardBackground -> _backgroundService->RestoreCurrentTheme(); -> which calls [1] (1: HomeCustomizationBackgroundConfigurationMediator from HomeCustomizationCoordinator) and also (2: HomeCustomizationBackgroundConfigurationMediator from HomeCustomizationBackgroundPickerActionSheetCoordinator). So we know if themeHasChanged is true or false. With this value, we also know if the user outcome is kCanceledAfterSelection, kCanceled, or kApplied.
BackgroundSelectionOutcome::kCanceledAfterSelection;Here, for example, `kCanceledAfterSelection` has a slightly different meaning for the photo flow vs color or gallery flows, right?
For the photo flow here, it means the user chose a photo from their library, but then cancelled. Vs color or gallery where they did select a color/image, saw how it looked on the background and then cancelled.
I guess in some sense they're similar, although to me it feels slightly different.
Yeah I see what you mean but yeah to me it's very similar. The user selected something (color, preset image or own image) and then canceled or applied or the user just canceled. Maybe we could change the name of enums? I think have all of theme with the same enum makes sense to me. Unless you feel strongly and in that case I can make another enum specifically for user uploaded images.
Related details
- Robbie Gibson
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robbie Gibson (Gerrit)
Robbie Gibson added 2 comments![]( "Open in Gerrit")
HomeCustomizationBackgroundPhotoPickerCoordinator updatesCheick CisseI'm a little confused here. I think the photo framing flow doesn't use the `HomeCustomizationBackgroundConfigurationMediator` at all, just the `HomeCustomizationBackgroundPhotoFramingMediator`, so would it be easier to track the photo flow stuff there?
Yeah it's a bit confusing I agree. The photo framing actually does interclty interact with HomeCustomizationBackgroundConfigurationMediator. HomeCustomizationBackgroundPhotoFramingMediator -> discardBackground -> _backgroundService->RestoreCurrentTheme(); -> which calls [1] (1: HomeCustomizationBackgroundConfigurationMediator from HomeCustomizationCoordinator) and also (2: HomeCustomizationBackgroundConfigurationMediator from HomeCustomizationBackgroundPickerActionSheetCoordinator). So we know if themeHasChanged is true or false. With this value, we also know if the user outcome is kCanceledAfterSelection, kCanceled, or kApplied.
Ah yeah, that's true, although tbh I'm not 100% sure if it's good that the 2 `HomeCustomizationBackgroundConfigurationMediator` are constantly observing the service (this is also what caused the earlier issue where both `HomeCustomizationBackgroundConfigurationMediator` see that the background has changed when you pick a color/gallery option).
But I don't really have a very well-thought-out replacement idea, so I guess it's fine for now.
BackgroundSelectionOutcome::kCanceledAfterSelection;Cheick CisseHere, for example, `kCanceledAfterSelection` has a slightly different meaning for the photo flow vs color or gallery flows, right?
For the photo flow here, it means the user chose a photo from their library, but then cancelled. Vs color or gallery where they did select a color/image, saw how it looked on the background and then cancelled.
I guess in some sense they're similar, although to me it feels slightly different.
Yeah I see what you mean but yeah to me it's very similar. The user selected something (color, preset image or own image) and then canceled or applied or the user just canceled. Maybe we could change the name of enums? I think have all of theme with the same enum makes sense to me. Unless you feel strongly and in that case I can make another enum specifically for user uploaded images.
Yeah, I guess that it's close enough to let us reuse the same enum.
Related details
- Cheick Cisse
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cheick Cisse (Gerrit)
Cheick Cisse added 1 comment![]( "Open in Gerrit")
HomeCustomizationBackgroundPickerActionSheetSoordinator becauseCheick Cissenit spelling "Soordinator"
Done
Related details
- Robbie Gibson
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Robbie Gibson (Gerrit)
Robbie Gibson voted Code-Review+1![]( "Open in Gerrit")
- Cheick Cisse
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cheick Cisse (Gerrit)
Cheick Cisse voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[iOS] Track user background selection outcome and small refactoring
This CL tracks the user's background selection outcome, which can be one
of the following:
- Selection applied
- Selection canceled
- No selection
This will be used for metrics.
In addition, this CL removes the call to SaveCurrentTheme, as this is already
handled in the HomeCustomizationCoordinator.
For user uploaded images, we have to set the backgroundSelectionOutcome
of the mediator in the
HomeCustomizationBackgroundPickerActionSheetCoordinator because
HomeCustomizationBackgroundPhotoPickerCoordinator updates
a different mediator. They are both
HomeCustomizationBackgroundConfigurationMediator but two different
objects.
- M ios/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.h
- M ios/chrome/browser/home_customization/coordinator/home_customization_background_configuration_mediator.mm
- M ios/chrome/browser/home_customization/coordinator/home_customization_background_picker_action_sheet_coordinator.mm
- M ios/chrome/browser/home_customization/utils/home_customization_constants.h
Code-Review: +1 by Robbie Gibson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |