Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mojom implementation of window.h5vcc.accessibility #5214

Merged
merged 2 commits into from
Mar 31, 2025
Merged

Mojom implementation of window.h5vcc.accessibility #5214

merged 2 commits into from
Mar 31, 2025

Conversation

haozheng-cobalt
Copy link
Contributor

@haozheng-cobalt haozheng-cobalt commented Mar 27, 2025 •
edited
Loading

Breaks circular dependency between H5vccAccessibilityImpl and CobaltTextToSpeechHelper through an observer interface pattern implemented in the interface TextToSpeechObserver.

H5vccAccessibilityImpl implements TextToSpeechObserver to receive state changes, CobaltTextToSpeechHelper maintains observer list without concrete H5vccAccessibilityImpl dependencies.
State queries flow: H5vccAccessibilityImpl → CobaltTextToSpeechHelper.
Notifications flow: CobaltTextToSpeechHelper → TextToSpeechObserver (H5vccAccessibilityImpl).

Test:
https://paste.googleplex.com/5925041131487232
Tested with Kabuki's loader for both APIs, see evidence in the ticket.

b/391708407

@haozheng-cobalt haozheng-cobalt self-assigned this Mar 27, 2025
@haozheng-cobalt haozheng-cobalt requested a review from a team as a code owner March 27, 2025 22:23
@haozheng-cobalt haozheng-cobalt force-pushed the tts-wiring branch 2 times, most recently from 69b1f5f to d406bf8 Compare March 27, 2025 23:40
hlwarriner
hlwarriner requested changes Mar 28, 2025
View reviewed changes
Copy link
Contributor

@hlwarriner hlwarriner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a first pass - mostly LGTM but left a few minor comments.

@hlwarriner
Copy link
Contributor

…kend

b/391708407

nit: please keep the subject line of the commit message to 50 characters, if you can: https://cbea.ms/git-commit/#limit-50.

@haozheng-cobalt haozheng-cobalt marked this pull request as draft March 28, 2025 15:49
@haozheng-cobalt haozheng-cobalt marked this pull request as ready for review March 28, 2025 19:50
@haozheng-cobalt haozheng-cobalt force-pushed the tts-wiring branch 3 times, most recently from 7f2f21e to 22a1573 Compare March 28, 2025 20:16
@haozheng-cobalt
Copy link
Contributor Author

haozheng-cobalt commented Mar 28, 2025 •
edited
Loading

Builds break, because

ERROR at //starboard/android/shared/text_to_speech_helper.h:22:11: Include not allowed.
#include "cobalt/browser/h5vcc_accessibility/text_to_speech_observer.h"
          ^-----------------------------------------------------------
It is not in any dependency of
  //starboard/android/shared:starboard_platform

Will relocate the dependency.

Still waiting for Kabuki's to provide a loader to test window.h5vccAccessibility.ontexttospeechchange,
Verify the window.h5vccAccessibility.textToSpeech works.

@haozheng-cobalt haozheng-cobalt force-pushed the tts-wiring branch 5 times, most recently from 4e2ca53 to 0654373 Compare March 31, 2025 03:28
@haozheng-cobalt haozheng-cobalt changed the title Implement the bindings between window.h5vcc.accessibility and the bac… Mojom implementation of window.h5vcc.accessibility Mar 31, 2025
andrewsavage1
andrewsavage1 requested changes Mar 31, 2025
View reviewed changes
@haozheng-cobalt haozheng-cobalt force-pushed the tts-wiring branch 5 times, most recently from fd4abef to 9b922e6 Compare March 31, 2025 19:49
hlwarriner
hlwarriner approved these changes Mar 31, 2025
View reviewed changes
Copy link
Contributor

@hlwarriner hlwarriner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside for a couple nits around the todo comments. Please let Andrew approve as well before merging, though.

Thanks for figuring this out!

@hlwarriner
Copy link
Contributor

Please also just double check that this all builds correctly:

$ cobalt/build/gn.py -p linux-x64x11-no-starboard -C devel
$ autoninja -C out/linux-x64x11-no-starboard_devel blink_wpt_tests dump_syms minidump_stackwalk content_shell

@haozheng-cobalt
Copy link
Contributor Author

Please also just double check that this all builds correctly:

$ cobalt/build/gn.py -p linux-x64x11-no-starboard -C devel
$ autoninja -C out/linux-x64x11-no-starboard_devel blink_wpt_tests dump_syms minidump_stackwalk content_shell

Thanks Holden, I made sure that linux build passes as well.

@haozheng-cobalt haozheng-cobalt enabled auto-merge (squash) March 31, 2025 21:05
@haozheng-cobalt haozheng-cobalt merged commit bf79023 into youtube:main Mar 31, 2025
151 checks passed
hlwarriner added a commit to hlwarriner/cobalt that referenced this pull request Apr 2, 2025
@hlwarriner
Run TTS cb with stub value for Starboard platforms
51602f3 
The IsTextToSpeechEnabledSync Mojo method is not yet implemented for
Starboard platforms but we should at least run the callback with a stub
value. We are currently seeing a crash on the main branch because the
callback is getting destroyed without first being run or its binding
getting closed. Thanks joeltine for flagging.

This is a fix-forward from youtube#5214.

b/391708407
@hlwarriner hlwarriner mentioned this pull request Apr 2, 2025
Merged
hlwarriner added a commit that referenced this pull request Apr 2, 2025
@hlwarriner
Run TTS cb with stub value for Starboard platforms (#5279)
74cbb0a 
The IsTextToSpeechEnabledSync Mojo method is not yet implemented for
Starboard platforms but we should at least run the callback with a stub
value. We are currently seeing a crash on the main branch because the
callback is getting destroyed without first being run or its binding
getting closed. Thanks joeltine for flagging.

This is a fix-forward from #5214.

b/391708407
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hlwarriner hlwarriner hlwarriner approved these changes

@andrewsavage1 andrewsavage1 andrewsavage1 approved these changes

@yell0wd0g yell0wd0g Awaiting requested review from yell0wd0g yell0wd0g is a code owner automatically assigned from youtube/cobalt-3p-repository-owners

Assignees

@haozheng-cobalt haozheng-cobalt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@haozheng-cobalt @hlwarriner @andrewsavage1