Voice Typing: Add a Toast UI notification for dictation errors [chromium/src : main]
Amya Singhal (Gerrit)
Amya Singhal abandoned this change.
Amya Singhal abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 4 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(DictationKeyedServiceBrowserTest, ShowsToastOnError) {Along with moving the impl to session_ui the test is probably better suited in session_ui_impl_browsertest.cc where you can use [Kombucha](https://chromium.googlesource.com/chromium/src/+/HEAD/chrome/test/interaction/README.md) testing which is well suited for UI ([example](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_controller_interactive_uitest.cc;l=158;drc=a76e5af6a1dee3620740b0d66a669f615a22e56a)).
provider->OnStreamStateChanged(StreamProvider::StreamState::kFailed);In session_ui tests use `ExcentionAPISetStreamState` to inject a failure (that'll exercise the extension API as well).
(attached_stream_provider_.get() == &stream_provider) &&Bit debatable but I think we might want to show an error if a background finalizing stream encounters an error.
I think the API should be (see my other comment about moving the handler to SessionUi) something like:
```
ui->OnError(stream_type)
```
Where `stream_type` is an enum `kAttached` or `kFinalizing` depending on which value it's in. We should also check `old_state` and ignore if the stream's old state is `kComplete`.
And no need to PostTask here - in SessionUiImpl you can call `UiRequestEndSession` which already posts. Avoid ending the session if the failing stream is finalizing.
// Called when the session encounters a fatal error.
virtual void OnSessionError() = 0;Showing an error is a responsibility of the SessionUi rather than the DictationKeyedService/SessionControllerDelegate. Lets move it to SessionUiImpl and the corresponding interface.
Related details
- Amya Singhal
- Kevin McNee
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. |
Kevin McNee (Gerrit)
Kevin McNee added 3 comments![]( "Open in Gerrit")
EXPECT_TRUE(base::test::RunUntil(Relatedly, if we synchronize on the end of the session, then we may not need to poll for this.
But both of these points are probably moot if this is rewritten as a ui test per bokan's comment.
toast_registry_->RegisterToast(Let's gate this on our feature flag.
Related details
- Amya Singhal
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. |
Amya Singhal (Gerrit)
Amya Singhal added 7 comments![]( "Open in Gerrit")
IN_PROC_BROWSER_TEST_F(DictationKeyedServiceBrowserTest, ShowsToastOnError) {Along with moving the impl to session_ui the test is probably better suited in session_ui_impl_browsertest.cc where you can use [Kombucha](https://chromium.googlesource.com/chromium/src/+/HEAD/chrome/test/interaction/README.md) testing which is well suited for UI ([example](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/send_tab_to_self/send_tab_to_self_bubble_controller_interactive_uitest.cc;l=158;drc=a76e5af6a1dee3620740b0d66a669f615a22e56a)).
done!
base::RunLoop run_loop;Amya SinghalThis is never run.
removed!
provider->OnStreamStateChanged(StreamProvider::StreamState::kFailed);In session_ui tests use `ExcentionAPISetStreamState` to inject a failure (that'll exercise the extension API as well).
done, thank you!
Relatedly, if we synchronize on the end of the session, then we may not need to poll for this.
But both of these points are probably moot if this is rewritten as a ui test per bokan's comment.
done!
Bit debatable but I think we might want to show an error if a background finalizing stream encounters an error.
I think the API should be (see my other comment about moving the handler to SessionUi) something like:
```
ui->OnError(stream_type)
```Where `stream_type` is an enum `kAttached` or `kFinalizing` depending on which value it's in. We should also check `old_state` and ignore if the stream's old state is `kComplete`.
And no need to PostTask here - in SessionUiImpl you can call `UiRequestEndSession` which already posts. Avoid ending the session if the failing stream is finalizing.
done, introduced ```ui->OnError(stream_type)```
// Called when the session encounters a fatal error.
virtual void OnSessionError() = 0;Showing an error is a responsibility of the SessionUi rather than the DictationKeyedService/SessionControllerDelegate. Lets move it to SessionUiImpl and the corresponding interface.
done!
Let's gate this on our feature flag.
- David Bokan
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 5 comments![]( "Open in Gerrit")
const bool is_attached =
attached_stream_provider_.get() == &stream_provider;
const bool is_failure =
stream_provider.GetState() == StreamProvider::StreamState::kFailed;Given my below comment these won't be useful when you move it. One nit you can do to improve readability is add `using StreamState = StreamProvider::StreamState;` at the top of the method body to at least remove some of the qualifiers
if (is_failure && old_state != StreamProvider::StreamState::kComplete) {
const SessionUi::StreamType stream_type =
is_attached ? SessionUi::StreamType::kAttached
: SessionUi::StreamType::kFinalizing;
if (ui_) {
ui_->OnError(stream_type);
}
}This is conceptually unrelated to what's being done in the outer `if` block so move it out.
Also, we should do this after movig to the failure state below so that, if the stream is attached, the UI sees a consistent controller state if it calls GetState.
SessionUiImpl::SessionUiImpl(base::WeakPtr<BrowserWindowInterface> window,Leave this as a ref, when you need to store it as a WeakPtr get one via GetWeakPtr. That said, the comment in [the header](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_window/public/browser_window_interface.h;l=205;drc=1a9fa755794756545ee0fb5d941a8527d56c37be) suggests avoiding this pattern.
Lets register a window close callback and _synchronously_ (because otherwise we'd be racing window deletion) notify the controller from the callback that the UI needs to be deleted. (e.g. add a UiDidClose() method on SessionUiDelegate). That way we can store window_ as a raw_ptr with the understanding that it will always be deleted before the window.
Check([this]() {
ToastController* toast_controller =
browser()->GetFeatures().toast_controller();
return toast_controller && !toast_controller->IsShowingToast();
}),nit: add a helper `IsShowingDictationErrorToast(bool)` to the test harness for this and the check that it's showing below. (I think it's fine for this check to just ensure no DictationError toast is showing.)
Related details
- Amya Singhal
- Kevin McNee
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. |
Amya Singhal (Gerrit)
Amya Singhal added 4 comments![]( "Open in Gerrit")
const bool is_attached =
attached_stream_provider_.get() == &stream_provider;
const bool is_failure =
stream_provider.GetState() == StreamProvider::StreamState::kFailed;Given my below comment these won't be useful when you move it. One nit you can do to improve readability is add `using StreamState = StreamProvider::StreamState;` at the top of the method body to at least remove some of the qualifiers
great idea, done!
if (is_failure && old_state != StreamProvider::StreamState::kComplete) {
const SessionUi::StreamType stream_type =
is_attached ? SessionUi::StreamType::kAttached
: SessionUi::StreamType::kFinalizing;
if (ui_) {
ui_->OnError(stream_type);
}
}This is conceptually unrelated to what's being done in the outer `if` block so move it out.
Also, we should do this after movig to the failure state below so that, if the stream is attached, the UI sees a consistent controller state if it calls GetState.
done!
SessionUiImpl::SessionUiImpl(base::WeakPtr<BrowserWindowInterface> window,Leave this as a ref, when you need to store it as a WeakPtr get one via GetWeakPtr. That said, the comment in [the header](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/browser_window/public/browser_window_interface.h;l=205;drc=1a9fa755794756545ee0fb5d941a8527d56c37be) suggests avoiding this pattern.
Lets register a window close callback and _synchronously_ (because otherwise we'd be racing window deletion) notify the controller from the callback that the UI needs to be deleted. (e.g. add a UiDidClose() method on SessionUiDelegate). That way we can store window_ as a raw_ptr with the understanding that it will always be deleted before the window.
done!
Check([this]() {
ToastController* toast_controller =
browser()->GetFeatures().toast_controller();
return toast_controller && !toast_controller->IsShowingToast();
}),nit: add a helper `IsShowingDictationErrorToast(bool)` to the test harness for this and the check that it's showing below. (I think it's fine for this check to just ensure no DictationError toast is showing.)
- David Bokan
- Kevin McNee
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted and added 9 comments![]( "Open in Gerrit")
Votes added by David Bokan
| Code-Review | +1 |
9 comments
Thank you! LGTM % a few minor nitpicks with naming/comments (partly my own choices from last round...)
delegate_->EndSession();We usually add a scary comment after a line like this because this call deletes `this`, e.g.:
```
// WARNING: `this` is deleted, do not add code below here.
```
Also, add a comment above explaining that we're intentionally ending the session synchronously in this path to avoid dangling pointers to deleted UI components.
if (is_failure && old_state != StreamState::kComplete) {Please add a test case for both these scenarios - failure state after non-complete state and failure state after complete state. Ensure only the former calls SessionUi::OnError.
You can do this from the session controller unit test where you can mock the Provider state and observe calls on the MockSessionUi.
// Called when a stream encounters an error.nit: when a _live_ stream encounters an error
virtual void UiDidClose() = 0;Sorry - now that I see this I think "UiDidClose" is a bit ambiguous (since Ui is more obviously referring to SessionUi). I think HostWindowDidClose would be cleaerer.
const raw_ptr<BrowserWindowInterface> window_;Make this a raw_ref since it should never be null
Also add a comment that it's a raw_ref because we observe window close events and synchronously end the session which owns `this` so SessionUi will never outlive the window. (and maybe a short comment for the below member that `// owns this`
if (window_) {Remove, this should never be null now
auto IsShowingDictationErrorToast(bool showing) {Sorry (again - it's hard to visualize until I see the code :/)
Lets make this CheckShowingDictationErrorToast to make it clearer this is a check
if (!toast_controller) {
return !showing;
}In what case do we not have a toast controller? I think this should be a CHECK (i.e. the tests aren't relevant if there's no toast system)
Related details
- Amya Singhal
- Kevin McNee
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. |