8378057: CAccessibility roleKey and AWTAccessor.AccessibleBundleAccessor are Redundant#29868
8378057: CAccessibility roleKey and AWTAccessor.AccessibleBundleAccessor are Redundant#29868mickleness wants to merge 3 commits intoopenjdk:masterfrom
Conversation
The `roleKey(x)` method does the same thing as `AWTAccessor.getAccessibleBundleAccessor().getKey(x)`. Neither supports a null argument. I don't have a strong preference which one is removed. I'm proposing removing `roleKey` first, simply because it appears similar to reflection (which we try to avoid). I ran jtreg against all the tests in the jdk/test/jdk/javax/accessibility/ directory, and this did not introduce any new regressions.
|
👋 Welcome back jwood! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
|
@mickleness The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| final AccessibleRole role = context.getAccessibleRole(); | ||
| String roleStr = role == null ? null : | ||
| AWTAccessor.getAccessibleBundleAccessor().getKey(role); | ||
| if (role != null && ignoredRoles != null && |
There was a problem hiding this comment.
this can be changed to roleStr != null?
This is in response to: openjdk#29868 (comment)
| if (!ignoredRoles.contains(roleKey(nvRole))) { | ||
| String roleStr = AWTAccessor.getAccessibleBundleAccessor(). | ||
| getKey(nvRole); | ||
| if (!ignoredRoles.contains(roleStr)) { |
There was a problem hiding this comment.
The change looks fine, but I’m wondering if we need the same null check here as well.
There was a problem hiding this comment.
I don't think we need it, but I added one just now since you asked about it.
Previously this invocation (at line 119) did not include a null check, and the other invocation (at line 1037) did.
This is in response to: https://github.com/openjdk/jdk/pull/29868/changes#r2866206079
2 participants
This PR proposes replacing the native
roleKeymethod with theAWTAccessor.AccessibleBundleAccessor. They appear to do the same thing.I ran all the existing jtreg tests in the javax/accessibility folder and observed no new regressions (tested on Mac).
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/29868/head:pull/29868$ git checkout pull/29868Update a local copy of the PR:
$ git checkout pull/29868$ git pull https://git.openjdk.org/jdk.git pull/29868/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 29868View PR using the GUI difftool:
$ git pr show -t 29868Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/29868.diff
Using Webrev
Link to Webrev Comment