<Swing Dev> Accessibility inner classes in AccessibleJTable do not stick to AccessibleComponent.getLocationOnScreen api

[Pavel Porvatov]

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
>>>
>>>
>>>
>>
>