Hi Frank,
Hi Pavel,
Thanks for your comments from which I learned a lot. A new review
patch is created by incorporating your comments @
http://cr.openjdk.java.net/~dingxmin/7188612/webrev.00/
Could you please review it again?
I have few comments about the last webrev:
1. There is no need to re-throw exceptions via exThrownInEDT. You can
just throw RuntimeException if the test fails. It's ok to throw
RuntimeException on the EDT, jtreg will fail in that case
2. Can we join two SwingUtilities.invokeAndWait into one?
3. Comments
96 // cast to interface AccessibleComponent to call
getLocationOnScreen()
and
109 // cast to interface AccessibleComponent to call
getLocationOnScreen()
look unnecessary
Other code looks good for me
Regards, Pavel
Best regards,
Frank
On 8/6/2012 9:54 PM, Pavel Porvatov wrote:
Hi Frank,
The fix looks good but I have several comments for the test:
1. The Swing components should be created and accessed only from EDT
thread. Therefore the assertGetLocation method should be on EDT as well
2. I'd remove lines 59-61
3. The lines
79 // Display the window.
80 frame.setVisible(true);
81 // make the frame invisible to test getLocationOnScreen() of
82 // JTable$AccessibleJTable$AccessibleJTableHeaderCell
83 // and JTable$AccessibleJTable$AccessibleJTableCell
84 frame.setVisible(false);
don't guaranty that the frame becomes visible for awhile. You should
use SunToolkit.realsync for that (take a look at other tests). Is it
really necessary for the test to show and hide the frame?
4. Why don't you cast getAccessibleContext into AccessibleTable in
one line, but wrote six lines for that:
88 AccessibleContext context = (AccessibleContext) table
89 .getAccessibleContext();
90 // To get
JTable$AccessibleJTable$AccessibleJTableHeaderCell instance,
91 // we need to first
92 // convert the context var to type AccessibleTable
93 AccessibleTable accessibleTable = (AccessibleTable) context;
5. Could you please remove useless comments like:
79 // Display the window.
80 frame.setVisible(true);
90 // To get
JTable$AccessibleJTable$AccessibleJTableHeaderCell instance,
91 // we need to first
92 // convert the context var to type AccessibleTable
94 // and then get
JTable$AccessibleJTable$AccessibleTableHeader
etc.
Only comments like
81 // make the frame invisible to test getLocationOnScreen() of
82 // JTable$AccessibleJTable$AccessibleJTableHeaderCell
83 // and JTable$AccessibleJTable$AccessibleJTableCell
100 // getLocation() must be null according to its javadoc
and no exception
101 // is thrown
looks reasonable for me in the provided test
Regards, Pavel
Hi guys
According to javadoc of AccessibleComponent.getLocationOnScreen,
it returns null instead of throwing IllegalComponentStateException
when the corresponding GUI object is not visible.
However, two accessibility inner classes of JTable,
JTable$AccessibleJTable$AccessibleJTableCell and
Table$AccessibleJTable$AccessibleJTableHeaderCell implementing
AccessibleComponent throw IllegalComponentStateException when the
corresponding JTable is not visible. This behavior conflicts with
the spec.
I have submitted a bug in sunbug system reporting the issue, and
received two acknowledgement emails (bug id 7188613 and 7188612,
don't know why there are two?). I created a patch to fix the issue
with a jtreg test case. Both the patch and the test are uploaded
into the following space.
http://cr.openjdk.java.net/~youdwei/ojdk-491/webrev.02/
Would anyone help to review the patch?
Best regards,
Frank
