cobalt icon indicating copy to clipboard operation
cobalt copied to clipboard

Move Accessibility API from Starboard to Extension

Open iuriionishchenko opened this issue 1 year ago • 4 comments

b/299639708

Change-Id: Ida75ca986b88ae444684321ae609e1aae5588bf5

iuriionishchenko avatar Feb 13 '24 14:02 iuriionishchenko

Codecov Report

Attention: Patch coverage is 1.47059% with 67 lines in your changes missing coverage. Please review.

Project coverage is 58.83%. Comparing base (84bd543) to head (fae7e35). Report is 8 commits behind head on main.

:exclamation: Current head fae7e35 differs from pull request most recent head edd03eb

Please upload reports for the commit edd03eb to get more accurate results.

Files Patch % Lines
cobalt/dom/captions/system_caption_settings.cc 0.00% 43 Missing :warning:
cobalt/h5vcc/h5vcc_accessibility.cc 0.00% 24 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2426      +/-   ##
==========================================
+ Coverage   58.74%   58.83%   +0.09%     
==========================================
  Files        1781     1905     +124     
  Lines       85283    93350    +8067     
==========================================
+ Hits        50099    54923    +4824     
- Misses      35184    38427    +3243     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Feb 13 '24 15:02 codecov[bot]

@hlwarriner could you please help review this ? I think we may need some bracketing of #ifdef STARBOARD 16 in the old headers, on an off chance someone somewhere included them.

kaidokert avatar Feb 22 '24 04:02 kaidokert

@hlwarriner could you please help review this ? I think we may need some bracketing of #ifdef STARBOARD 16 in the old headers, on an off chance someone somewhere included them.

Sure. Yeah I think this needs SB_API_VERSION guards, similar to how they were used in https://github.com/youtube/cobalt/pull/1930.

hlwarriner avatar Feb 22 '24 16:02 hlwarriner

@hlwarriner could you please help review this ? I think we may need some bracketing of #ifdef STARBOARD 16 in the old headers, on an off chance someone somewhere included them.

Sure. Yeah I think this needs SB_API_VERSION guards, similar to how they were used in #1930.

I have added SB_API_VERSION guards.

iuriionishchenko avatar Mar 14 '24 11:03 iuriionishchenko

@kaidokert, should someone with domain knowledge about the accessibility API take a quick look, too? I only focused on the mechanics of changing the Starboard API to an extension.

@johnxwork, can you have a quick look ?

kaidokert avatar May 17 '24 00:05 kaidokert

Further - Android code still references starboard/accessibility.h and accessibility_internal.h here - this client code needs to be converted too, otherwise we can't land this change.

kaidokert avatar Jun 15 '24 00:06 kaidokert

Internal CL 285260 at go/cobalt-cl/285260

kaidokert avatar Jun 19 '24 04:06 kaidokert

I've added the Android conversion. Tests are still to be followed up on, but otherwise ready.

Internal CL needs to be merged together with this

I am working on unit test and have some issue with debugging them. When I fix it I will update this PR to add the unit tests. Also I have a question. There is a comment, as I remember, in the CL to rename StarboardExtensionAccessibilityApi to CobaltExtensionAccessibilityApi but we steel have StarboardExtensionAccessibilityApi here and CobaltExtensionAccessibilityApi in the CL. I missed these changes I suppose when I was resolving conflicts. Which name should I used? I will update in the next commit.

iuriionishchenko avatar Jun 19 '24 16:06 iuriionishchenko

I am working on unit test and have some issue with debugging them. When I fix it I will update this PR to add the unit tests.

Lets add those in a separate PR as a follow-up please. We are getting a bit time critical to get the Starboard header changes in, so i'm okay to do those later.

Also I have a question. There is a comment, as I remember, in the CL to rename StarboardExtensionAccessibilityApi to CobaltExtensionAccessibilityApi but we steel have StarboardExtensionAccessibilityApi here and CobaltExtensionAccessibilityApi in the CL. I missed these changes I suppose when I was resolving conflicts. Which name should I used? I will update in the next commit.

The naming should generally be "Starboard Extension" everywhere. Again this is okay to follow-up with cleanup CLs.

Most important here is that i can merge this with green builds together with internal CLs 285260 and 285223

kaidokert avatar Jun 19 '24 19:06 kaidokert