This change to the ownership model for the delegate passed into
`SetEventDelegate` requires us to separate the controller from the
delegate, and have them share a weak relationship.
Chromium changes:
https://chromium.googlesource.com/chromium/src/+/50d43c7bcf04f5fac7cdc7ad26732f3d94b5bd43
commit 50d43c7bcf04f5fac7cdc7ad26732f3d94b5bd43
Author: Di Wu <diwux@google.com>
Date: Sun Jul 27 20:29:49 2025 -0700
Refactor TtsUtterance to always own its event delegate
This change refactors the ownership model for UtteranceEventDelegate to
establish a single, clear model where TtsUtterance always takes
ownership of its delegate via std::unique_ptr. This simplifies the code,
addresses code review feedback, and fixes underlying memory management
issues that caused test failures.
Problem:
The previous memory management for UtteranceEventDelegate was fragile.
Some delegates used a "delete this" pattern, which is error-prone and
led to memory leaks in browser tests.
An initial refactoring moved to a std::unique_ptr model but introduced a
NonOwnedUtteranceEventDelegate wrapper to handle cases where the
delegate's lifetime was managed externally (e.g., TtsSpeakFunction,
SettingsWithTtsPreviewHandler). Code reviewers pointed out that this
wrapper complicated the ownership model and that a single, consistent
ownership pattern would be preferable.
Solution:
This commit fully adopts the single-ownership model and removes the
NonOwnedUtteranceEventDelegate wrapper.
1. TtsUtterance always owns its delegate: TtsUtterance::SetEventDelegate
now exclusively takes a std::unique_ptr<UtteranceEventDelegate>,
ensuring the delegate is automatically destroyed with the utterance.
2. Refactored externally-managed delegates: The two cases that
previously required a non-owned wrapper have been refactored to use
dedicated, owned delegate classes:
2.1 TtsSpeakFunction: Now uses a new, private TtsExtensionEventHandler
class that implements UtteranceEventDelegate. This handler is owned by
the TtsUtterance and holds the `extension_id` to dispatch events. This
removes the need for a reference back to the `TtsSpeakFunction`,
simplifying lifetime management.
2.2 SettingsWithTtsPreviewHandler: Now uses a new
TtsPreviewEventDelegate class. This handler is owned by the TtsUtterance
and holds a base::WeakPtr to the SettingsWithTtsPreviewHandler to safely
make callbacks.
3. Simplified delegate cleanup: With the removal of the non-owned
wrapper, the GetType() virtual method on UtteranceEventDelegate and the
corresponding logic in TtsControllerImpl are no longer needed and have
been removed.
This refactoring makes the TTS delegate ownership model clear,
consistent, and safe, resolving memory leaks and directly addressing
code review feedback for a simpler design.
Bug: b:281717553, b:251732518, 431531726, 432151731
Test: Run and symbolize the affected two tests with asan and lsan enabled. Then run with MiraclePtr check on the SelectToSpeakTest.FullscreenMagnifierFollowsTextBoundsWhenPrefOn test.
Change-Id: Ia7524c0a859ee2f7c1f208412215c6e6b1b4e512
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6733896
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Di Wu <diwux@google.com>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1492580}
This change replaces all uses of `absl::optional` with `std::*` variants
for optional. This is in line with upstream recent changes making
`absl::optional` a `typedef` to the `std` type.
This change has been done using an automated script:
#!/bin/bash
function replace {
echo "Replacing $1 by $2"
git grep -l "$1" \
| cut -f1 -d: \
| sort \
| uniq \
| grep \
-e "\.h" \
-e "\.cc" \
-e "\.mm" \
-e "\.py" \
| xargs sed -i "s/$1/$2/g"
}
function delete_line_with {
echo "Deleting lines with $1"
git grep -l "$1" \
| cut -f1 -d: \
| sort \
| uniq \
| grep \
-e "\.h" \
-e "\.cc" \
-e "\.mm" \
-e "\.py" \
| xargs sed -i "/$1/d"
}
function add_header {
echo "Adding header $1"
git diff --name-only HEAD \
| xargs ../tools/add_header.py --header "$1"
}
replace "absl::make_optional" "std::make_optional"
replace "absl::optional" "std::optional"
replace "absl::nullopt" "std::nullopt"
replace "absl::in_place" "std::in_place"
replace "absl::in_place_t" "std::in_place_t"
add_header "<optional>"
delete_line_with "\"third_party\/abseil-cpp\/absl\/types\/optional.h\""
git cl format
Chromium change:
https://chromium.googlesource.com/chromium/src/+/d9d21aa16829a7d471a4f3b3a493a31170ed8271
commit d9d21aa16829a7d471a4f3b3a493a31170ed8271
Author: David Benjamin <davidben@chromium.org>
Date: Mon Oct 2 23:29:57 2023 +0000
Make absl::optional a typedef for std::optional
This only changes the types around. It doesn't rewrite existing uses
to std::optional, which we can do incrementally.
absl::optional to std::optional seems to have two visible impacts.
First, the field order is different (bool first vs bool last).
std::optional's order (bool last) seems to be better overall, decreasing
binary size. Second, absl::optional's assertions crash with
__builtin_trap, while std::optional calls __libcpp_verbose_abort which
calls base::ImmediateCrash. __builtin_trap permits the compiler to
combine crash sites within a function but leads to worse crash
debugging. In base::ImmediateCrash, we'd made a conscious decision to
prefer debuggability and pay some binary size for it. The net size
increase brings our optional type in line with that preference.
For more details see the discussion and document below:
https://groups.google.com/a/chromium.org/g/cxx/c/XG3G85_ZF1k/m/_QN8adIJBQAJhttps://docs.google.com/document/d/1AW7q9HCLOk738OCj8Z2U_AKVUC0YIFZWuyRvv09XTHk/edit
Binary-Size: See discussion above.
Fuchsia-Binary-Size: See discussion above.
Bug: 1373619
* TTS fixes.
Added reading word highlight.
Added state for inline play/pause button.
Added description in the TTS list.
Fixed paragraph highlight style.
Pause TTS when changing toolbar state.
Fixed colors to use leo::colors.
Added progress animation for underline.
Claudio DeSouza