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

[Pavel Porvatov]

Hi Frank,
> Hi Pavel,
>   Thanks for your review again.  Another review package incorporating 
> your comments is @ http://cr.openjdk.java.net/~dingxmin/7188612/webrev.01
>   Could you please take a look at it ?
The fix looks good for me.

Regards, Pavel
>
> Thanks,
> Frank
>
> On 8/16/2012 11:02 PM, Pavel Porvatov wrote:
>> 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
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>