-
Notifications
You must be signed in to change notification settings - Fork 118
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.
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
Move Accessibility API from Starboard to Extension #2426
Move Accessibility API from Starboard to Extension #2426
Conversation
eeb0d20
to
11ac418
Compare
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
@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 |
11ac418
to
c5733db
Compare
I have added SB_API_VERSION guards. |
607cf96
to
ea11401
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes, @iuriionishchenko.
Just one more thing - can you please update the commit message to follow https://cbea.ms/git-commit/#imperative? Maybe just "Move Accessibility API from Starboard to Extension"?
I left a few other small comments but other than that, the changes look good.
@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.
e610404
to
584dee3
Compare
584dee3
to
2295027
Compare
2295027
to
fae7e35
Compare
e964231
to
ad75766
Compare
ad75766
to
14d72e2
Compare
@johnxwork, can you have a quick look ? |
14d72e2
to
a15cba7
Compare
a15cba7
to
9671a68
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is failing tests because starboard/accessibility.h
is not updated.
starboard/nplb/accessibility_test.cc
also needs an update that disables it for Sb16
Further - Android code still references |
b/299639708 Change-Id: Ida75ca986b88ae444684321ae609e1aae5588bf5
e57df8d
to
1e1acbd
Compare
Internal CL 285260 at go/cobalt-cl/285260 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
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 |
b/299639708 Change-Id: Ida75ca986b88ae444684321ae609e1aae5588bf5 --------- Co-authored-by: Kaido Kert <[email protected]> (cherry picked from commit 30b5734)
…ion (#3608) Refer to the original PR: #2426 b/299639708 Change-Id: Ida75ca986b88ae444684321ae609e1aae5588bf5 Co-authored-by: iuriionishchenko <[email protected]>
This reverts commit 30b5734.
b/299639708
Change-Id: Ida75ca986b88ae444684321ae609e1aae5588bf5