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

[Frank Ding]

Hi Pavel
   Thanks for review.
To Jonathan,
   Thanks for committing.  It looks good.

Best regards,
Frank

On 8/20/2012 1:20 PM, Jonathan Lu wrote:
> Hi Pavel,
>
> Thanks for review, the change has been pushed to
> http://hg.openjdk.java.net/jdk8/awt/jdk/rev/2fe9c1f0b16b
>
> changeset:   5692:2fe9c1f0b16b
> tag:         tip
> user:        dingxmin
> date:        Mon Aug 20 13:16:18 2012 +0800
> files:       src/share/classes/javax/swing/JTable.java 
> test/javax/swing/JTable/7188612/JTableAccessibleGetLocationOnScreen.java
> description:
> 7188612: JTable's AccessibleJTable throws 
> IllegalComponentStateException instead of null
> Reviewed-by: rupashka
>
> And to Frank, could you pls verify it?
>
> Thanks
> Jonathan
>
> On 08/17/2012 07:13 PM, Pavel Porvatov wrote:
>> 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>