Add suggestion interaction callbacks in FilterTabController. [chromium/src : main]
Aditya Gupta (Gerrit)
Aditya Gupta has uploaded the change for review![]( "Open in Gerrit")
Commit message
Add suggestion interaction callbacks in FilterTabController.
This CL adds SuggestionInteractionCallbacks in MultistepFilterUiDelegate and passes the SuggestionInteractionCallbacks to FilterUiController in OnSuggestionGenerated method. This decouples MultistepFilterService depedency from FilterUiController and makes FilterTabController the source of truth for all future metrics tracking.
Change diff
Change information
- M chrome/browser/multistep_filter/BUILD.gn
- M chrome/browser/multistep_filter/chrome_filter_navigation_observer_unittest.cc
- M chrome/browser/multistep_filter/multistep_filter_ui_delegate_impl.cc
- M chrome/browser/multistep_filter/multistep_filter_ui_delegate_impl.h
- M chrome/browser/multistep_filter/multistep_filter_ui_delegate_impl_unittest.cc
- M chrome/browser/multistep_filter/ui/filter_ui_controller.cc
- M chrome/browser/multistep_filter/ui/filter_ui_controller.h
- M chrome/browser/multistep_filter/ui/filter_ui_controller_test_api.h
- M chrome/browser/multistep_filter/ui/filter_ui_controller_unittest.cc
- M components/multistep_filter/content/content_filter_navigation_observer_unittest.cc
- M components/multistep_filter/core/filter_tab_controller.cc
- M components/multistep_filter/core/filter_tab_controller.h
- M components/multistep_filter/core/filter_tab_controller_unittest.cc
- M components/multistep_filter/core/multistep_filter_ui_delegate.h
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré added 11 comments![]( "Open in Gerrit")
OnSuggestionGenerated(testing::Optional(suggestion), testing::_));```suggestion
OnSuggestionGenerated(testing::Optional(suggestion), _));
```
~MultistepFilterUiDelegateImpl() override = default;The `= default` should go back to the .cc file.
// Callbacks to notify the core of user interactions with this suggestion.about
case SuggestionViewState::kInactive: {here and below: these {} are not needed, right?
if (suggestion_state_->callbacks.on_suggestion_shown) {
std::move(suggestion_state_->callbacks.on_suggestion_shown).Run();
}in the `case` below you first do the logging and then the callback. Is there a reason not to have a consistent order?
// Called when the user interacts with a suggestion."the suggestion UI."?
virtual void OnSuggestionGenerated(I think that if you rename this to "ShowSuggestions" the whole concept with the callbacks becomes a bit more obvious.
base::OnceCallback<void(SuggestionUserDecision)> on_suggestion_interaction;base::RepeatedCallback?
base::OnceCallback<void(SuggestionUserDecision)> on_suggestion_interaction;on_user_iteraction?
base::OnceClosure on_suggestion_reopened;Is this a onetime callback or can this happen multiple times, which would mean 1) `base::RepeatedCallback` and 2) no use of `std::move(...)`?
struct SuggestionInteractionCallbacks {SuggestionUiCallbacks?
Related details
- Aditya Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré added 2 comments![]( "Open in Gerrit")
.WillOnce(
[&captured_callbacks](
std::optional<UrlFilterSuggestion> suggestion,
MultistepFilterUiDelegate::SuggestionInteractionCallbacks
callbacks) { captured_callbacks = std::move(callbacks); });How about this:
```suggestion
.WillOnce(SaveArgByMove<1>(&captured_callbacks));
```
Maybe you could also use a TestFuture... something like the following (I am not sure if I got the syntax right).
```
base::TestFuture<void, std::optional<UrlFilterSuggestion>,
MultistepFilterUiDelegate::SuggestionInteractionCallbacks> future.
EXPECT_CALL(*mock_delegate_,
OnSuggestionGenerated(std::optional(expected_suggestion), _))
.WillOnce(InvokeFuture(future));
...
std::move(future.Get<2>().on_suggestion_shown).Run();
```
std::move(captured_callbacks.on_suggestion_shown).Run();This crashes the entire test process in case the callback did not happen.
Related details
- Aditya Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Aditya Gupta (Gerrit)
Aditya Gupta voted and added 13 comments![]( "Open in Gerrit")
Votes added by Aditya Gupta
| Commit-Queue | +1 |
13 comments
OnSuggestionGenerated(testing::Optional(suggestion), testing::_));Aditya Gupta```suggestion
OnSuggestionGenerated(testing::Optional(suggestion), _));
```
Done
~MultistepFilterUiDelegateImpl() override = default;The `= default` should go back to the .cc file.
Done
// Callbacks to notify the core of user interactions with this suggestion.Aditya Guptaabout
Done
case SuggestionViewState::kInactive: {here and below: these {} are not needed, right?
Done
if (suggestion_state_->callbacks.on_suggestion_shown) {
std::move(suggestion_state_->callbacks.on_suggestion_shown).Run();
}in the `case` below you first do the logging and then the callback. Is there a reason not to have a consistent order?
Done
// Called when the user interacts with a suggestion.Aditya Gupta"the suggestion UI."?
Done
.WillOnce(
[&captured_callbacks](
std::optional<UrlFilterSuggestion> suggestion,
MultistepFilterUiDelegate::SuggestionInteractionCallbacks
callbacks) { captured_callbacks = std::move(callbacks); });How about this:
```suggestion
.WillOnce(SaveArgByMove<1>(&captured_callbacks));
```Maybe you could also use a TestFuture... something like the following (I am not sure if I got the syntax right).
```
base::TestFuture<void, std::optional<UrlFilterSuggestion>,
MultistepFilterUiDelegate::SuggestionInteractionCallbacks> future.
EXPECT_CALL(*mock_delegate_,
OnSuggestionGenerated(std::optional(expected_suggestion), _))
.WillOnce(InvokeFuture(future));...
std::move(future.Get<2>().on_suggestion_shown).Run();
```
Done
std::move(captured_callbacks.on_suggestion_shown).Run();This crashes the entire test process in case the callback did not happen.
Added ASSERT_FALSE(captured_callbacks.on_suggestion_shown.is_null()); before triggering the callback,
virtual void OnSuggestionGenerated(I think that if you rename this to "ShowSuggestions" the whole concept with the callbacks becomes a bit more obvious.
I will do that in a separate CL else this CL will add a lot of noise of the renaming.
base::OnceCallback<void(SuggestionUserDecision)> on_suggestion_interaction;Aditya Guptaon_user_iteraction?
Done
base::OnceCallback<void(SuggestionUserDecision)> on_suggestion_interaction;base::RepeatedCallback?
Same as above but if you think this is not correct I am happy to set up some time to go over this.
base::OnceClosure on_suggestion_reopened;Is this a onetime callback or can this happen multiple times, which would mean 1) `base::RepeatedCallback` and 2) no use of `std::move(...)`?
No since we only want to record the metrics once, I choose to make them all OnceCallback, verified that if the callback is null (after making the first call), the check is in place to not call it again. This way I could be confident that the callbacks will be only triggered once for a suggesiton.
struct SuggestionInteractionCallbacks {- Dominic Battré
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré voted and added 6 comments![]( "Open in Gerrit")
Votes added by Dominic Battré
| Code-Review | +1 |
6 comments
std::move(captured_callbacks.on_suggestion_shown).Run();Aditya GuptaThis crashes the entire test process in case the callback did not happen.
Added ASSERT_FALSE(captured_callbacks.on_suggestion_shown.is_null()); before triggering the callback,
Acknowledged
base::OnceCallback<void(SuggestionUserDecision)> on_user_interaction;// Called with the final interaction by the user. This can mean for example accepting a suggestion or to ignore it. Unless the user accepts the suggestion or opens settings, the reporting happens when the tab is closed.
please verify this is correct or fix it.
base::OnceClosure on_suggestion_reopened;// Called the first time a suggestion UI is shown after being dismissed/timing out before.
base::OnceClosure on_suggestion_shown;// Called the first time a suggestion UI is shown after pageload.
base::OnceCallback<void(SuggestionUserDecision)> on_suggestion_interaction;base::RepeatedCallback?
Same as above but if you think this is not correct I am happy to set up some time to go over this.
Acknowledged
base::OnceClosure on_suggestion_reopened;Aditya GuptaIs this a onetime callback or can this happen multiple times, which would mean 1) `base::RepeatedCallback` and 2) no use of `std::move(...)`?
No since we only want to record the metrics once, I choose to make them all OnceCallback, verified that if the callback is null (after making the first call), the check is in place to not call it again. This way I could be confident that the callbacks will be only triggered once for a suggesiton.
- Aditya Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dominic Battré
| Code-Review | +1 |
1 comment
// 2. Verify on_suggestion_interaction callback.on_user_interaction
Related details
- Aditya Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominic Battré (Gerrit)
Dominic Battré added 1 comment![]( "Open in Gerrit")
OnSuggestionGenerated(testing::Optional(suggestion), testing::_));delete
Related details
- Aditya Gupta
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |