[iOS] Conditionally hide Safety Check module in Magic Stack [chromium/src : main]
Benjamin Williams (Gerrit)
Benjamin Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Williams
| Commit-Queue | +1 |
1 comment
Hi folks,
- thegreenfrog@ - Need overall review
- gambard@/ajuma@ - Need OWNERS review (overall review also appreciated if your time permits)
Thanks!
Related details
- Ali Juma
- Chris Lu
- Gauthier Ambard
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard added 5 comments![]( "Open in Gerrit")
bool IsSafetyCheckModuleHiddenWithoutIssues();Nit: the method name is confusing. When reading it in the code I was expecting this to return whether the module is currently hidden or not, not a bool.
Did you consider replacing "Is" by "Should" (and adapting the rest) or something similar?
// LINT.ThenChange(/tools/metrics/histograms/metadata/ios/enums.xml:IOSSafetyCheckHiddenReason)If the linter error expected? Should the path starts with a `/`?
UMA_HISTOGRAM_ENUMERATION(HEre and below, why using a macro and not a function?
(previousIssuesCount == 0) && (previousIssuesCount == issuesCount);I am not sure to understand the logic. The module is hidden if there was 0 issues previously and there is currently 0 issues (and it is not the default value)?
So, if we used to have issues and now we don't, the module is shown?
And why checking the default value of the pref?
- (void)logIssuesCount:(NSUInteger)issuesCountNit, optional: "log" is associated with UMA in my mind. I would rather have something like `updateIssueCountPref: withCount:` or something like that.
Related details
- Ali Juma
- Benjamin Williams
- Chris Lu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams added 5 comments![]( "Open in Gerrit")
Nit: the method name is confusing. When reading it in the code I was expecting this to return whether the module is currently hidden or not, not a bool.
Did you consider replacing "Is" by "Should" (and adapting the rest) or something similar?
Great point. Done.
// LINT.ThenChange(/tools/metrics/histograms/metadata/ios/enums.xml:IOSSafetyCheckHiddenReason)If the linter error expected? Should the path starts with a `/`?
Done
HEre and below, why using a macro and not a function?
Thanks for catching that! I was initially following the file's existing use of macros for histograms. I've now updated everything to `base::UmaHistogramEnumeration()`.
(previousIssuesCount == 0) && (previousIssuesCount == issuesCount);I am not sure to understand the logic. The module is hidden if there was 0 issues previously and there is currently 0 issues (and it is not the default value)?
So, if we used to have issues and now we don't, the module is shown?
And why checking the default value of the pref?
Overall, yes, that's correct! This is new business logic from Product. The Safety Check module will now only be visible in the Magic Stack if there are issues to display. If both the previous and current state is 'All Safe' (i.e. zero issues), the module will be hidden.
"So, if we used to have issues and now we don't, the module is shown?"
>> Yes, that's right, because Product wants users to see the 'All Safe' state maximally once.
"And why checking the default value of the pref?"
>> Because the Pref being zero typically means the 'All Safe' state. However, not when it's the default value, that just means it hasn't been set before.
If my answers are satisfactory, please feel free to mark this thread as complete. Thanks Gauthier for raising this!
Also, I've updated the CL description, and added some new comments, to make this business logic more clear.
Nit, optional: "log" is associated with UMA in my mind. I would rather have something like `updateIssueCountPref: withCount:` or something like that.
- Ali Juma
- Chris Lu
- Gauthier Ambard
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ali Juma (Gerrit)
Ali Juma voted and added 1 comment![]( "Open in Gerrit")
- Benjamin Williams
- Chris Lu
- Gauthier Ambard
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams added 1 comment![]( "Open in Gerrit")
// LINT.ThenChange(/tools/metrics/histograms/metadata/ios/enums.xml:IOSSafetyCheckHiddenReason)Benjamin WilliamsIf the linter error expected? Should the path starts with a `/`?
Done
- Chris Lu
- Gauthier Ambard
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gauthier Ambard (Gerrit)
Gauthier Ambard voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gauthier Ambard
| Code-Review | +1 |
3 comments
// LINT.ThenChange(//tools/metrics/histograms/metadata/ios/enums.xml:IOSSafetyCheckHiddenReason)Sorry I wasn't clear, my suggestion was to remove the leading "/". (Feel free to land if it doesn't work)
(previousIssuesCount == 0) && (previousIssuesCount == issuesCount);Benjamin WilliamsI am not sure to understand the logic. The module is hidden if there was 0 issues previously and there is currently 0 issues (and it is not the default value)?
So, if we used to have issues and now we don't, the module is shown?
And why checking the default value of the pref?
Overall, yes, that's correct! This is new business logic from Product. The Safety Check module will now only be visible in the Magic Stack if there are issues to display. If both the previous and current state is 'All Safe' (i.e. zero issues), the module will be hidden.
"So, if we used to have issues and now we don't, the module is shown?"
>> Yes, that's right, because Product wants users to see the 'All Safe' state maximally once."And why checking the default value of the pref?"
>> Because the Pref being zero typically means the 'All Safe' state. However, not when it's the default value, that just means it hasn't been set before.If my answers are satisfactory, please feel free to mark this thread as complete. Thanks Gauthier for raising this!
Also, I've updated the CL description, and added some new comments, to make this business logic more clear.
- Ali Juma
- Benjamin Williams
- Chris Lu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Williams
| Commit-Queue | +2 |
1 comment
// LINT.ThenChange(//tools/metrics/histograms/metadata/ios/enums.xml:IOSSafetyCheckHiddenReason)Sorry I wasn't clear, my suggestion was to remove the leading "/". (Feel free to land if it doesn't work)
- Ali Juma
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Lu (Gerrit)
Chris Lu added 2 comments![]( "Open in Gerrit")
I think we need to now update the EGTest to ensure the safety check card is added to the MagicStack
if (!IsSafetyCheckMagicStackEnabled() ||as an aside, can we do feature flag cleanup?
Related details
- Ali Juma
- Benjamin Williams
- Chris Lu
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Williams
| Commit-Queue | +2 |
1 comment
if (!IsSafetyCheckMagicStackEnabled() ||as an aside, can we do feature flag cleanup?
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams added 1 comment![]( "Open in Gerrit")
if (!IsSafetyCheckMagicStackEnabled() ||Benjamin Williamsas an aside, can we do feature flag cleanup?
Done
This will be handled in a separate CL.
Benjamin Williams (Gerrit)
Benjamin Williams voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams added 1 comment![]( "Open in Gerrit")
Hi team, please ignore this latest patchset. SafetyCheckView EG Test is failing on ios-simulator-full-configs. I'm investigating why the module isn't showing up (which causes the test to fail). Added some temporary logs to help with this. Will update once resolved.
Benjamin Williams (Gerrit)
Benjamin Williams added 1 comment![]( "Open in Gerrit")
Hi everyone, thanks for your patience while I resolved the module visibility issue. The problem was that the new code was checking if the `kHomeCustomizationMagicStackSafetyCheckIssuesCount` Pref was set to its default value, initially aimed at distinguishing between intentional and default zero values. However, this check proved fragile and unnecessary. The fix is to hide the module only if both previous and current issue counts are zero; any change will unhide it. I've removed the default value check, and all tests should pass, with submission working as expected. Thanks again for your patience!
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams removed a vote from this change![]( "Open in Gerrit")
Removed Commit-Queue+2 by Benjamin Williams <bwwil...@google.com>
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Williams (Gerrit)
Benjamin Williams voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Williams
| Commit-Queue | +2 |
1 comment
Fingers crossed this fixes the module visibility issue for SafetyCheckView EG test.
Related details
- Ali Juma
- Chris Lu
- Chris Lu
Code-Coverage
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: ios/chrome/browser/ui/content_suggestions/magic_stack/magic_stack_ranking_model.mm
Insertions: 3, Deletions: 7.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/ui/content_suggestions/safety_check/safety_check_view_egtest.mm
Insertions: 12, Deletions: 8.
The diff is too large to show. Please review the diff.
```
```
The name of the file: ios/chrome/browser/ui/content_suggestions/content_suggestions_metrics_constants.h
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
[iOS] Conditionally hide Safety Check module in Magic Stack
This change optimizes the visibility of the Safety Check module within
the Magic Stack by dynamically displaying it only when new issues arise.
This approach prevents unnecessary visual clutter and ensures users are
promptly informed about potential safety concerns without bloating the
Magic Stack. The module will be hidden if the user currently has no
Safety Check issues and the previous Safety Check state also displayed
no issues. As soon as new issues are detected, the module will be
unhidden and re-included in the Magic Stack.
This CL introduces a new local-state Pref to store the previous issues
count, adds logging to a new histogram to better understand why the
Safety Check module may not be displayed in the Magic Stack, and places
all of this functionality behind a Finch killswitch for enhanced safety
and control.
- M ios/chrome/browser/shared/model/prefs/browser_prefs.mm
- M ios/chrome/browser/shared/model/prefs/pref_names.h
- M ios/chrome/browser/shared/public/features/features.h
- M ios/chrome/browser/shared/public/features/features.mm
- M ios/chrome/browser/ui/content_suggestions/content_suggestions_metrics_constants.h
- M ios/chrome/browser/ui/content_suggestions/content_suggestions_metrics_constants.mm
- M ios/chrome/browser/ui/content_suggestions/magic_stack/magic_stack_ranking_model.mm
- M ios/chrome/browser/ui/content_suggestions/magic_stack/magic_stack_ranking_model_unittest.mm
- M ios/chrome/browser/ui/content_suggestions/safety_check/BUILD.gn
- M ios/chrome/browser/ui/content_suggestions/safety_check/safety_check_magic_stack_mediator.mm
- M ios/chrome/browser/ui/content_suggestions/safety_check/safety_check_view_egtest.mm
- M tools/metrics/histograms/metadata/ios/enums.xml
- M tools/metrics/histograms/metadata/ios/histograms.xml
Code-Review: +1 by Gauthier Ambard
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |