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

[Frank Ding]

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 ?

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