Add session restore infobar setting change pref and metrics [chromium/src : main]
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 1 comment![]( "Open in Gerrit")
<histogram name="Session.Restore.SettingChanged.TurnOffFromRestart"
enum="Boolean" expires_after="2026-09-01">
<owner>musa...@chromium.org</owner>
<owner>koreta...@chromium.org</owner>
<summary>
Records whether the user changed the session restore setting after being
shown an infobar to turn off session restore from browser restart.
</summary>
</histogram>
<histogram name="Session.Restore.SettingChanged.TurnOffFromSession"
enum="Boolean" expires_after="2026-09-01">
<owner>musa...@chromium.org</owner>
<owner>koreta...@chromium.org</owner>
<summary>
Records whether the user changed the session restore setting after being
shown an infobar to turn off session restore from a restored session.
</summary>
</histogram>
<histogram name="Session.Restore.SettingChanged.TurnOnSessionRestore"
enum="Boolean" expires_after="2026-09-01">
<owner>musa...@chromium.org</owner>
<owner>koreta...@chromium.org</owner>
<summary>
Records whether the user changed the session restore setting after being
shown an infobar to turn on session restore.
</summary>
</histogram>Muhammad SalmaanWe can merge those 3 metrics together in 1 by using Patterned Histograms.
See: https://chromium.googlesource.com/chromium/src/+/lkgr/tools/metrics/histograms/README.md
Done
Related details
- Daniel Soromou
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
// When this param is true, the session restore preference will have
// continue where you left off as default behavior.
extern const base::FeatureParam<bool> kSetDefaultToContinueSession;Should we use BASE_DECLARE_FEATURE_PARAM or should we document why it's not used here?
Related details
- Darryl James
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 1 comment![]( "Open in Gerrit")
// When this param is true, the session restore preference will have
// continue where you left off as default behavior.
extern const base::FeatureParam<bool> kSetDefaultToContinueSession;Should we use BASE_DECLARE_FEATURE_PARAM or should we document why it's not used here?
I discussed it with Ian that its better to have a mode that when its enabled it sets the session restore preference to restore session.Having two feature params could is not needed in this case.This would be similar to the page action when we individually set them to true. The same is happening here.
Related details
- Daniel Soromou
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Soromou (Gerrit)
Daniel Soromou added 1 comment![]( "Open in Gerrit")
// When this param is true, the session restore preference will have
// continue where you left off as default behavior.
extern const base::FeatureParam<bool> kSetDefaultToContinueSession;Muhammad SalmaanShould we use BASE_DECLARE_FEATURE_PARAM or should we document why it's not used here?
I discussed it with Ian that its better to have a mode that when its enabled it sets the session restore preference to restore session.Having two feature params could is not needed in this case.This would be similar to the page action when we individually set them to true. The same is happening here.
I am talking about they way to declare this. We have 2 ways to declare feature params to my knowledge.
(1) using BASE_DECLARE_FEATURE_PARAM: this is optimized for the case where the param will be queried multiple time. All the params in this file using that partern.
(2) using extern like it's done currently.
So my question was should we also use `BASE_DECLARE_FEATURE_PARAM` here like for the other feature params? or should be explain why we are not using that approach here?
Related details
- Darryl James
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 1 comment![]( "Open in Gerrit")
// When this param is true, the session restore preference will have
// continue where you left off as default behavior.
extern const base::FeatureParam<bool> kSetDefaultToContinueSession;Muhammad SalmaanShould we use BASE_DECLARE_FEATURE_PARAM or should we document why it's not used here?
Daniel SoromouI discussed it with Ian that its better to have a mode that when its enabled it sets the session restore preference to restore session.Having two feature params could is not needed in this case.This would be similar to the page action when we individually set them to true. The same is happening here.
I am talking about they way to declare this. We have 2 ways to declare feature params to my knowledge.
(1) using BASE_DECLARE_FEATURE_PARAM: this is optimized for the case where the param will be queried multiple time. All the params in this file using that partern.
(2) using extern like it's done currently.
So my question was should we also use `BASE_DECLARE_FEATURE_PARAM` here like for the other feature params? or should be explain why we are not using that approach here?
Changed to using BASE_DECLARE_FEATURE_PARAM as others in the file are using it too.
Related details
- Daniel Soromou
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 2 comments![]( "Open in Gerrit")
if (WasSessionRestorePrefChanged(profile.GetPrefs()) ||optional: This can probably have more generic naming such as `ManualUserActionTaken` or `UserInteractedWithSessionRestorePref`
How it is now is also good 👍
pref_change_registrar_.RemoveAll();Should this code be implemented within the infobar itself?
My understanding is that this listener function can outlive the infobar itself. That is, once we have shown the infobar we listen to the kRestoreOnStartup pref and continue to listen to it throughout the lifetime of the browser until the pref is changed.
I am thinking we want to tie this to the lifetime of the infobar instead (either in the infobar itself of by listening to the infobar's destruction in this class).
Related details
- Alex Ilin
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 2 comments![]( "Open in Gerrit")
optional: This can probably have more generic naming such as `ManualUserActionTaken` or `UserInteractedWithSessionRestorePref`
How it is now is also good 👍
Done
Should this code be implemented within the infobar itself?
My understanding is that this listener function can outlive the infobar itself. That is, once we have shown the infobar we listen to the kRestoreOnStartup pref and continue to listen to it throughout the lifetime of the browser until the pref is changed.
I am thinking we want to tie this to the lifetime of the infobar instead (either in the infobar itself of by listening to the infobar's destruction in this class).
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 1 comment![]( "Open in Gerrit")
session_restore_infobar_controller_ = std::make_unique<
session_restore_infobar::SessionRestoreInfobarController>();Muhammad SalmaanConsider making this Unowned User Data
Done
Related details
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 4 comments![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
4 comments
auto session_restore_infobar_controller =`auto*` to prevent compiler warnings
void SessionRestoreInfobarController::MaybeShowInfoBar(
Profile& profile,
bool was_restarted,
bool is_post_crash_launch) {
if (features::kSetDefaultToContinueSession.Get()) {
SessionStartupPref::SetStartupPref(
&profile, SessionStartupPref(SessionStartupPref::LAST));
}
if (UserInteractedWithSessionRestorePref(profile.GetPrefs()) ||
InfoBarShownMaxTimes(profile.GetPrefs())) {
return;
}
model_ = std::make_unique<SessionRestoreInfobarModel>(profile, was_restarted,
is_post_crash_launch);
if (!model_->ShouldShowOnStartup()) {
return;
}
if (GetInfobarMessageType() ==
SessionRestoreInfoBarDelegate::InfobarMessageType::kNone) {
return;
}
SessionRestoreInfoBarManager::GetInstance()->ShowInfoBar(
profile, GetInfobarMessageType());
IncrementInfoBarShownCount(profile.GetPrefs());
}
super optional: This function should go below the destructor to match the definition order in the header file 👍
static void RecordSettingChanged(question: Why is this one static? It is to get around errors for a callback or something similar?
Related details
- Alex Ilin
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin added 3 comments![]( "Open in Gerrit")
bool setting_changed,I don't see it ever being recorded with `false` value. Is this working as intended?
Records whether the user changed the session restore setting after being
shown an infobar of type {SessionRestoreInfobarType}.The description needs to be more clear about what "after being shown an infobar" means. Will this be recorded if the user changes the setting 5 minutes after seeing the infobar? 10 days after seeing the infobar?
I guess this wants to say "changed the session restore setting while being shown an infobar".
shown an infobar of type {SessionRestoreInfobarType}.The UMA dashboard will replace the `{SessionRestoreInfobarType}` placeholder with the summary provided below, so you'll get the following string:
```
Records whether the user changed the session restore setting after being
shown an infobar of type turn off session restore from browser restart.
```
This string is a little bit hard to read.
I have the following suggestion that should be combined with a slight change of "summary" strings.
```suggestion
shown the session restore infobar that informs the user about {SessionRestoreInfobarType}.
```
```
<token key="SessionRestoreInfobarType">
<variant name="TurnOffFromRestart"
summary="turning off session restore from browser restart"/>
<variant name="TurnOffFromSession"
summary="turning off session restore from a restored session"/>
<variant name="TurnOnSessionRestore" summary="turning on session restore"/>
</token>
```
Related details
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 6 comments![]( "Open in Gerrit")
`auto*` to prevent compiler warnings
Done
void SessionRestoreInfobarController::MaybeShowInfoBar(
Profile& profile,
bool was_restarted,
bool is_post_crash_launch) {
if (features::kSetDefaultToContinueSession.Get()) {
SessionStartupPref::SetStartupPref(
&profile, SessionStartupPref(SessionStartupPref::LAST));
}
if (UserInteractedWithSessionRestorePref(profile.GetPrefs()) ||
InfoBarShownMaxTimes(profile.GetPrefs())) {
return;
}
model_ = std::make_unique<SessionRestoreInfobarModel>(profile, was_restarted,
is_post_crash_launch);
if (!model_->ShouldShowOnStartup()) {
return;
}
if (GetInfobarMessageType() ==
SessionRestoreInfoBarDelegate::InfobarMessageType::kNone) {
return;
}
SessionRestoreInfoBarManager::GetInstance()->ShowInfoBar(
profile, GetInfobarMessageType());
IncrementInfoBarShownCount(profile.GetPrefs());
}
super optional: This function should go below the destructor to match the definition order in the header file 👍
Done
question: Why is this one static? It is to get around errors for a callback or something similar?
Had reverted code, used in the controller for recording metrics for setting to false when user did not change the setting for the infobar when it was a maximum of 3 times and no action was taken.
I don't see it ever being recorded with `false` value. Is this working as intended?
Added it in, I reverted it by mistake.
Records whether the user changed the session restore setting after being
shown an infobar of type {SessionRestoreInfobarType}.The description needs to be more clear about what "after being shown an infobar" means. Will this be recorded if the user changes the setting 5 minutes after seeing the infobar? 10 days after seeing the infobar?
I guess this wants to say "changed the session restore setting while being shown an infobar".
Done
The UMA dashboard will replace the `{SessionRestoreInfobarType}` placeholder with the summary provided below, so you'll get the following string:
```
Records whether the user changed the session restore setting after being
shown an infobar of type turn off session restore from browser restart.
```This string is a little bit hard to read.
I have the following suggestion that should be combined with a slight change of "summary" strings.
```suggestion
shown the session restore infobar that informs the user about {SessionRestoreInfobarType}.
``````
<token key="SessionRestoreInfobarType">
<variant name="TurnOffFromRestart"
summary="turning off session restore from browser restart"/>
<variant name="TurnOffFromSession"
summary="turning off session restore from a restored session"/>
<variant name="TurnOnSessionRestore" summary="turning on session restore"/>
</token>
```
- Alex Ilin
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
- Alex Ilin
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin added 6 comments![]( "Open in Gerrit")
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());It'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
shown an infobar of type {SessionRestoreInfobarType}.Muhammad SalmaanThe UMA dashboard will replace the `{SessionRestoreInfobarType}` placeholder with the summary provided below, so you'll get the following string:
```
Records whether the user changed the session restore setting after being
shown an infobar of type turn off session restore from browser restart.
```This string is a little bit hard to read.
I have the following suggestion that should be combined with a slight change of "summary" strings.
```suggestion
shown the session restore infobar that informs the user about {SessionRestoreInfobarType}.
``````
<token key="SessionRestoreInfobarType">
<variant name="TurnOffFromRestart"
summary="turning off session restore from browser restart"/>
<variant name="TurnOffFromSession"
summary="turning off session restore from a restored session"/>
<variant name="TurnOnSessionRestore" summary="turning on session restore"/>
</token>
```
Done
Only partially done, I highlighted below the required edits to the summary values.
Records whether the user changed the session restore setting after being
shown an infobar of type {SessionRestoreInfobarType}.Muhammad SalmaanThe description needs to be more clear about what "after being shown an infobar" means. Will this be recorded if the user changes the setting 5 minutes after seeing the infobar? 10 days after seeing the infobar?
I guess this wants to say "changed the session restore setting while being shown an infobar".
Done
Not done, my comment still stands
Please explain explain the recording logic in details as required by guidelines [1].
Examining your logic, it's more subtle than my initial guess, especially for recording the `false` value.
False value is recorded upon a blocked attempt to show the infobar because the max limit is reached.
There is also a case, when the histogram isn't recorded at all. Namely, if the user changes the pref value while the infobar is not showing, neither `true` or `false` will be recorded (is this intentional?).
It's important to document all these special cases so that you and other engineers can make sense of the recorded values later [2].
[1] https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#state-when-it-is-recorded
[2]
https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#understandable-to-everyone
summary="turn off session restore from browser restart"/>```suggestion
summary="turning off session restore from browser restart"/>
```
summary="turn off session restore from a restored session"/>```suggestion
summary="turning off session restore from a restored session"/>
```
<variant name="TurnOnSessionRestore" summary="turn on session restore"/>```suggestion
<variant name="TurnOnSessionRestore" summary="turning on session restore"/>
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 5 comments![]( "Open in Gerrit")
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());It'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
This will hit it only once when for the specific profile the infobar has been shown 3 times. So basically, the infobar will be shown 3 times and then on the fourth try this if statement will be true and record false for the setting change.
Records whether the user changed the session restore setting after being
shown an infobar of type {SessionRestoreInfobarType}.Muhammad SalmaanThe description needs to be more clear about what "after being shown an infobar" means. Will this be recorded if the user changes the setting 5 minutes after seeing the infobar? 10 days after seeing the infobar?
I guess this wants to say "changed the session restore setting while being shown an infobar".
Alex IlinDone
Not done, my comment still stands
Please explain explain the recording logic in details as required by guidelines [1].
Examining your logic, it's more subtle than my initial guess, especially for recording the `false` value.
False value is recorded upon a blocked attempt to show the infobar because the max limit is reached.
There is also a case, when the histogram isn't recorded at all. Namely, if the user changes the pref value while the infobar is not showing, neither `true` or `false` will be recorded (is this intentional?).
It's important to document all these special cases so that you and other engineers can make sense of the recorded values later [2].
[1] https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#state-when-it-is-recorded
[2]
https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#understandable-to-everyone
I have described it better, let me know if this one on the current change sounds good.
summary="turn off session restore from browser restart"/>```suggestion
summary="turning off session restore from browser restart"/>
```
Done
summary="turn off session restore from a restored session"/>```suggestion
summary="turning off session restore from a restored session"/>
```
Done
<variant name="TurnOnSessionRestore" summary="turn on session restore"/>```suggestion
<variant name="TurnOnSessionRestore" summary="turning on session restore"/>
```
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin added 3 comments![]( "Open in Gerrit")
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());Muhammad SalmaanIt'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
This will hit it only once when for the specific profile the infobar has been shown 3 times. So basically, the infobar will be shown 3 times and then on the fourth try this if statement will be true and record false for the setting change.
My concern is about what's going to happen when `MaybeShowInfoBar()` is called for the fifth time.
Could you point me out at a condition that blocks this call for the 5th, 6th, and so on times?
Even better, it'd be great to write a test verifying that a `false` value will be recorded only once.
Records whether the user changed the session restore setting after being
shown an infobar of type {SessionRestoreInfobarType}.Muhammad SalmaanThe description needs to be more clear about what "after being shown an infobar" means. Will this be recorded if the user changes the setting 5 minutes after seeing the infobar? 10 days after seeing the infobar?
I guess this wants to say "changed the session restore setting while being shown an infobar".
Alex IlinDone
Muhammad SalmaanNot done, my comment still stands
Please explain explain the recording logic in details as required by guidelines [1].
Examining your logic, it's more subtle than my initial guess, especially for recording the `false` value.
False value is recorded upon a blocked attempt to show the infobar because the max limit is reached.
There is also a case, when the histogram isn't recorded at all. Namely, if the user changes the pref value while the infobar is not showing, neither `true` or `false` will be recorded (is this intentional?).
It's important to document all these special cases so that you and other engineers can make sense of the recorded values later [2].
[1] https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#state-when-it-is-recorded
[2]
https://chromium.googlesource.com/chromium/src/+/HEAD/tools/metrics/histograms/README.md#understandable-to-everyone
I have described it better, let me know if this one on the current change sounds good.
The extended version looks pretty good, thanks!
while the infobar is shown. A `false` value is recorded if the user
dismisses the infobar without changing the setting, or the user has ignored
the infobar and it has been shown a max of 3 times.Do we need both of these conditions? I liked the previous version more, when a single profile would record `true` or `false` at most once.
This gave you a clear signal whether the infobar encouraged the user to take an action after 3 attempts, or not.
If you want to record `false` on every infobar dismiss, I'd encourage you to split your histogram into two, or convert it to an enum histogram, so that you can distinguish an intermediate `false` from a final `false`.
Related details
- Darryl James
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 2 comments![]( "Open in Gerrit")
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());Muhammad SalmaanIt'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
Alex IlinThis will hit it only once when for the specific profile the infobar has been shown 3 times. So basically, the infobar will be shown 3 times and then on the fourth try this if statement will be true and record false for the setting change.
My concern is about what's going to happen when `MaybeShowInfoBar()` is called for the fifth time.
Could you point me out at a condition that blocks this call for the 5th, 6th, and so on times?
Even better, it'd be great to write a test verifying that a `false` value will be recorded only once.
I added a check in the destructor for the delegate if no action is taken for the infobar and its the last time its being displayed to record the metric.
while the infobar is shown. A `false` value is recorded if the user
dismisses the infobar without changing the setting, or the user has ignored
the infobar and it has been shown a max of 3 times.Do we need both of these conditions? I liked the previous version more, when a single profile would record `true` or `false` at most once.
This gave you a clear signal whether the infobar encouraged the user to take an action after 3 attempts, or not.
If you want to record `false` on every infobar dismiss, I'd encourage you to split your histogram into two, or convert it to an enum histogram, so that you can distinguish an intermediate `false` from a final `false`.
if a user dismisses an infobar once, it will not appear again. so the false is only ever recorded once for if the infobar was dismissed and the other flow in which it is recorded false is when the infobar has been ignored ( meaning no action was taken for the 3 times the infobar was shown.).For a profile, False is only recorded once).
I added a check in the destructor for the delegate if no action is taken for the infobar and its the last time its being displayed to record the metric.
Related details
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 1 comment![]( "Open in Gerrit")
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());Muhammad SalmaanIt'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
Alex IlinThis will hit it only once when for the specific profile the infobar has been shown 3 times. So basically, the infobar will be shown 3 times and then on the fourth try this if statement will be true and record false for the setting change.
Muhammad SalmaanMy concern is about what's going to happen when `MaybeShowInfoBar()` is called for the fifth time.
Could you point me out at a condition that blocks this call for the 5th, 6th, and so on times?
Even better, it'd be great to write a test verifying that a `false` value will be recorded only once.
I added a check in the destructor for the delegate if no action is taken for the infobar and its the last time its being displayed to record the metric.
Done
Related details
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James added 2 comments![]( "Open in Gerrit")
model_ = std::make_unique<SessionRestoreInfobarModel>(profile, was_restarted,question: Should model creation be deferred until after the early returns?
profile_.get().GetPrefs()->SetBoolean(prefs::kSessionRestorePrefChanged,
true);I was thinking about this line for a while, did we want the behavior to permanently dismiss the infobar if the user dismissed it once? Even if they haven't changed the session restore preference?
My expectation here would be to allow users to dismiss the infobar manually up to 3 times when it appears. The only cases where the infobar would stop appearing is if the pref is manually changed by the user in the settings page, or if the infobar has already been seen 3 times.
Dismissing it should just increment the count in my opinion but maybe this is intended? Is it?
Related details
- Alex Ilin
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 2 comments![]( "Open in Gerrit")
model_ = std::make_unique<SessionRestoreInfobarModel>(profile, was_restarted,question: Should model creation be deferred until after the early returns?
Fix applied.
profile_.get().GetPrefs()->SetBoolean(prefs::kSessionRestorePrefChanged,
true);I was thinking about this line for a while, did we want the behavior to permanently dismiss the infobar if the user dismissed it once? Even if they haven't changed the session restore preference?
My expectation here would be to allow users to dismiss the infobar manually up to 3 times when it appears. The only cases where the infobar would stop appearing is if the pref is manually changed by the user in the settings page, or if the infobar has already been seen 3 times.
Dismissing it should just increment the count in my opinion but maybe this is intended? Is it?
- Alex Ilin
- Darryl James
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darryl James (Gerrit)
Darryl James voted and added 1 comment![]( "Open in Gerrit")
Votes added by Darryl James
| Code-Review | +1 |
- Alex Ilin
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Ilin (Gerrit)
Alex Ilin voted and added 5 comments![]( "Open in Gerrit")
Votes added by Alex Ilin
| Code-Review | +1 |
5 comments
LGTM
SessionRestoreInfoBarDelegate::RecordSettingChanged(
false, GetInfobarMessageType());Muhammad SalmaanIt'd be better to record this histogram only once when the max allowed limit is hit. With the way it's written now, the histogram will be recorded on every startup [1] until the user changes the pref value to trigger an earlier return on L50.
This will for sure over-inflate the `false` count, making the histogram less useful.
Alex IlinThis will hit it only once when for the specific profile the infobar has been shown 3 times. So basically, the infobar will be shown 3 times and then on the fourth try this if statement will be true and record false for the setting change.
Muhammad SalmaanMy concern is about what's going to happen when `MaybeShowInfoBar()` is called for the fifth time.
Could you point me out at a condition that blocks this call for the 5th, 6th, and so on times?
Even better, it'd be great to write a test verifying that a `false` value will be recorded only once.
Muhammad SalmaanI added a check in the destructor for the delegate if no action is taken for the infobar and its the last time its being displayed to record the metric.
Done
Thanks, I like this solution much better!
prefs::kSessionRestoreInfoBarTimesShown) == 3) {nit: please use a named constant here
1. A named constant makes the meaning of "3" clearer
2. It avoids getting two values that mean the same thing diverged over time
```suggestion
prefs::kSessionRestoreInfoBarTimesShown) ==
kSessionRestoreInfoBarMaxTimesToShow) {
```
dismisses the infobar without changing the setting, or the user has ignorednit:
```suggestion
dismisses the infobar without changing the setting (which stops the infobar from being shown again), or the user has ignored
```
while the infobar is shown. A `false` value is recorded if the user
dismisses the infobar without changing the setting, or the user has ignored
the infobar and it has been shown a max of 3 times.Muhammad SalmaanDo we need both of these conditions? I liked the previous version more, when a single profile would record `true` or `false` at most once.
This gave you a clear signal whether the infobar encouraged the user to take an action after 3 attempts, or not.
If you want to record `false` on every infobar dismiss, I'd encourage you to split your histogram into two, or convert it to an enum histogram, so that you can distinguish an intermediate `false` from a final `false`.
if a user dismisses an infobar once, it will not appear again. so the false is only ever recorded once for if the infobar was dismissed and the other flow in which it is recorded false is when the infobar has been ignored ( meaning no action was taken for the 3 times the infobar was shown.).For a profile, False is only recorded once).
I added a check in the destructor for the delegate if no action is taken for the infobar and its the last time its being displayed to record the metric.
Alex Ilin
Ah, that's a piece of information that was missing, thanks for clarifying!
Related details
- Muhammad Salmaan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan added 2 comments![]( "Open in Gerrit")
nit: please use a named constant here
1. A named constant makes the meaning of "3" clearer
2. It avoids getting two values that mean the same thing diverged over time```suggestion
prefs::kSessionRestoreInfoBarTimesShown) ==
kSessionRestoreInfoBarMaxTimesToShow) {
```
Fix applied.
dismisses the infobar without changing the setting, or the user has ignorednit:
```suggestion
dismisses the infobar without changing the setting (which stops the infobar from being shown again), or the user has ignored
```
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Muhammad Salmaan (Gerrit)
Muhammad Salmaan voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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 with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
16 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_delegate.cc
Insertions: 2, Deletions: 1.
@@ -87,7 +87,8 @@
RecordInfoBarAction(message_type_,
SessionRestoreInfoBarDelegate::InfobarAction::kIgnored);
if (profile_->GetPrefs()->GetInteger(
- prefs::kSessionRestoreInfoBarTimesShown) == 3) {
+ prefs::kSessionRestoreInfoBarTimesShown) ==
+ kSessionRestoreInfoBarMaxTimesToShow) {
RecordSettingChanged(false, message_type_);
}
}
```
```
The name of the file: tools/metrics/histograms/metadata/session/histograms.xml
Insertions: 3, Deletions: 2.
@@ -122,8 +122,9 @@
A `true` value is recorded if the user changes the session restore setting
while the infobar is shown. A `false` value is recorded if the user
- dismisses the infobar without changing the setting, or the user has ignored
- the infobar and it has been shown a max of 3 times.
+ dismisses the infobar without changing the setting (which stops the infobar
+ from being shown again), or the user has ignored the infobar and it has been
+ shown a max of 3 times.
This histogram is not recorded if the user changes the setting at any other
time (i.e. when the session restore infobar is not showing).
```
Change information
Add session restore infobar setting change pref and metrics
This cl adds the following: Adds a mode to change the default session
restore preference to continue where you left off. Adds a check to see
while the infobar is displayed, if the setting has changed, and if it
has, the infobar will not be displayed starting from the next browser
session. Adds metric to record for if the user changed the session
restore preference while the infobar was present and records the type of
infobar present as well while the change took place.
- M chrome/browser/ui/browser_ui_prefs.cc
- M chrome/browser/ui/browser_window/internal/BUILD.gn
- M chrome/browser/ui/browser_window/internal/browser_window_features.cc
- M chrome/browser/ui/browser_window/public/browser_window_features.h
- M chrome/browser/ui/startup/infobar_utils.cc
- M chrome/browser/ui/ui_features.cc
- M chrome/browser/ui/ui_features.h
- M chrome/browser/ui/views/session_restore_infobar/BUILD.gn
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_controller.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_controller.h
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_delegate.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_delegate.h
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_delegate_unittest.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_interactive_uitest.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_manager.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_model.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_model.h
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_prefs.cc
- M chrome/browser/ui/views/session_restore_infobar/session_restore_infobar_prefs.h
- M chrome/common/pref_names.h
- M tools/metrics/histograms/metadata/session/histograms.xml
Code-Review: +1 by Darryl James, +1 by Alex Ilin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |