Hi Phil
Thanks for the pointers. Since you mentioned that the bug pattern was observed before, I just did a quick search in accessibility module to see if any such cases are there. I could only find that for a Vector, which as you said is fine. Second, I was also trying to understand if the "contains" function was really intended in the bug, but looking at the code, I did not find any reason that they should have used contains rather than "containsKey". Sure will fix the space issue before pushing. Thanks, Krishna From: Philip Race Sent: Tuesday, March 6, 2018 12:36 AM To: Krishna Addepalli <[email protected]> Cc: [email protected] Subject: Re: <Swing Dev> [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString On 03/05/2018 05:39 AM, Krishna Addepalli wrote: Hi Phil Thank you for the review. I have checked the accessibility package, and found that contains is used with a Vector of AccessibleState objects in the file AccessibleStateset. contains is fine for a Vector .. it is the map case that folks can get wrong. However in AccessibilityBundle, the table is used to store a set of values associated with a particular locale, and going by the context, I find little reason that using contains is intentional here. I did not suggest it was. I was observing that using contains instead of containsKey is a mistake we've seen before. And regarding the testcase, the thought of making it headless crossed my mind while writing it, but I was not sure if just creating a Button is allowed in headless mode. Fortunately, I modified the testcase enough that, no swing widgets need to be created, and we can safely run the test in headless mode. java.awt.Button is heavyweight and can't even be constructed in headless. JButton is lightweight and can be constructed in headless mode. Here is the new webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8197785/webrev01/"http://cr.openjdk.java.net/~kaddepalli/8197785/webrev01/ 42 public static void main(String... args) throws Exception{ "Exception{" -> "Exception {" Please fix before pushing. Looks good other than that. -phil. Thanks, Krishna From: Philip Race Sent: Saturday, March 3, 2018 9:44 PM To: Krishna Addepalli HYPERLINK "mailto:[email protected]";<[email protected]> Cc: HYPERLINK "mailto:[email protected]"[email protected] Subject: Re: <Swing Dev> [11][JDK-8197785]javax.accessibility.AccessibilityBundle will reload the ResourceBundle for every call to toDisplayString The fix is straightforward and I've seen this bug pattern before. In fact there may even have been a sweep for uses of contains() to make sure it was as intended, but if so it wasn't thorough enough. But I'm wondering why the test extends Button and not JButton ? I'm then further wondering if it could then be made headless .. ie no need to create or display a Frame ? -phil. On 3/3/18, 6:30 AM, Krishna Addepalli wrote: Hi All, Please review a simple fix for JDK-8197785: https://bugs.openjdk.java.net/browse/JDK-8197785 Webrev: HYPERLINK "http://cr.openjdk.java.net/%7Ekaddepalli/8197785/webrev00/"http://cr.openjdk.java.net/~kaddepalli/8197785/webrev00/ As the bug description suggests, AccessibleBundle.loadResourceBundle has the line "table.contains" which causes it to reload the resource bundle for each call. Changing it to "table.containsKey" fixes the problem. Thanks, Krishna
