<AWT Dev> <Awt Dev> [9] Review Request for 8080395: consider making sun.awt.CausedFocusEvent functionality public

Mon Apr 4 08:21:59 UTC 2016

   The fix looks good to me.

   Thanks,
   Alexandr.

On 4/4/2016 9:48 AM, Semyon Sadetsky wrote:
> Thank you, Phil.
> The updates webrev: 
> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.05/
>
> --Semyon
>
> On 3/30/2016 7:22 PM, Phil Race wrote:
>> >  39     private static final long serialVersionUID = -3647309088427840738L;
>>
>> Is this the value generated by JDK 8 ? It should be ..
>>
>> I agree getCause() should be final.
>>
>>
>> Other than that this seems fine to me.
>>
>> BTW I would have preferred that the order of files  in the webrev 
>> place FocusEvent
>> as the first file. Almost all the changes flow from that so we should 
>> get to read it first.
>>
>> -phil.
>>
>> On 03/23/2016 12:33 AM, Semyon Sadetsky wrote:
>>> On 3/22/2016 7:54 PM, Sergey Bylokhov wrote:
>>>> I am not sure that the latest version became better than previous 
>>>> one. The code became much more complicated but do we really solved 
>>>> the problem related to serialization? Will we support both 
>>>> directions when the data serialized/deserialized in jkd9/jdk8/jdk7 
>>>> and so on. The readResolve() helped us to deserialize the data from 
>>>> the jdk8, but it does not help in case of jdk9 to jdk8 
>>>> serialisation, because our new FocusEvent from jdk9 should be 
>>>> converted to the old CausedFocusEvent but this will requires 
>>>> changes in jdk8/jdk7.
>>> Sergey, we must not support "forward compatibility" for AWT 
>>> serialization,  backward compatibility is enough.
>>>>
>>>> I understand the idea of "CausedFocusEvent exists for 
>>>> deserialization compatibility only." but not sure how it is good. 
>>>> Probably someone knows, do we have similar cases before? When we 
>>>> have some outdated internal class just for serialization 
>>>> compatibility?
>>>>
>>>> I still think that it will be better to update serial version UID, 
>>>> implement readObject() to throw an exception if "cause" is null, 
>>>> and possibly update the specification. In the same way as in most 
>>>> classes in the client.
>>>>
>>>>  - Why did your remove the "final" from the getCause() method? This 
>>>> will automatically blocks us from calling this method(or at least 
>>>> use it quite carefully) inside the jdk, because it can be 
>>>> overridden by the user. I suggest to restore it.
>>> It is not needed. There is no even one "final" method in the whole 
>>> "java.awt.event.*" package.
>>>
>>> --Semyon
>>>>  - Small comment: the "and" before "opposite" can be changed to ","?
>>>> 182      * specified temporary state and opposite {@code Component} 
>>>> and the
>>>> 183      * {@code Cause.UNKNOWN} cause.
>>>>
>>>> On 02.02.16 15:24, Semyon Sadetsky wrote:
>>>>> Please review the updated webrev:
>>>>> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.04/
>>>>>
>>>>> - CausedFocusEvent is restored to avoid CNFE during deserialization.
>>>>>
>>>>> - readResolve() is added to FocusEvent to handle the null cause test.
>>>>>
>>>>> - deserialization compatibility test scenarios added
>>>>>
>>>>> --Semyon
>>>>>
>>>>> On 11/6/2015 3:57 PM, Alexander Scherbatiy wrote:
>>>>>>
>>>>>> There is a problem with FocusEvent deserialization that the read 
>>>>>> cause
>>>>>> value is not checked to null.
>>>>>> You can fix it with the current fix or just create a separate 
>>>>>> issue on
>>>>>> it.
>>>>>>
>>>>>> Otherwise, the fix looks good to me.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>> On 10/29/2015 12:33 AM, Sergey Bylokhov wrote:
>>>>>>> On 27.10.15 13:01, Semyon Sadetsky wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 10/26/2015 5:31 PM, Sergey Bylokhov wrote:
>>>>>>>>>>> The test should verify this also.
>>>>>>>>>>> I assume it should be reverted and updated to:
>>>>>>>>>>> 252 if (cause == null)
>>>>>>>>>>> 253 throw new IllegalArgumentException("null cause");
>>>>>>>>>> Please review the updated webrev
>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.02/
>>>>>>>>>
>>>>>>>>> is possible to write a test fox this missed check?
>>>>>>>>> What about a sentence in:
>>>>>>>>> ********
>>>>>>>>> 228      * <p> This method throws an
>>>>>>>>> 229      * {@code IllegalArgumentException} if {@code source}
>>>>>>>>> 230      * is {@code null}.
>>>>>>>>> ********
>>>>>>>>> 250     public FocusEvent(Component source, int id, boolean 
>>>>>>>>> temporary,
>>>>>>>>> 251                       Component opposite, Cause cause) {
>>>>>>>>> ********
>>>>>>>>> Should we mention cause and IAE as well in the javadoc?
>>>>>>>> Test case is added. null case is mentioned in javadoc.
>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.03/
>>>>>>>
>>>>>>> The fix looks fine. I am not sure about this:
>>>>>>> * {@code IllegalArgumentException} if {@code source} or {@code 
>>>>>>> cause}
>>>>>>> * is {@code null}.
>>>>>>> I suppose it should be "are {@code null}?
>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Since you update the serialVersionUID then the comment is 
>>>>>>>>>>> not valid
>>>>>>>>>>> anymore: "JDK 1.1 serialVersionUID".
>>>>>>>>>>>
>>>>>>>>>>> Also I have requested an additional clarification from the 
>>>>>>>>>>> core libs
>>>>>>>>>>> team to confirm that we have an ability to change
>>>>>>>>>>> serialVersionUID in
>>>>>>>>>>> the major release.
>>>>>>>>>> Please decide finally you do need a new serialVersionUID or 
>>>>>>>>>> you don't
>>>>>>>>>> need it. Generally, adding a field is a compatible change 
>>>>>>>>>> FocusEvent
>>>>>>>>>> will be able to read the previous format. It is matter of
>>>>>>>>>> opinion/convention only.
>>>>>>>>>
>>>>>>>>> In this case the cause field will contain unspecified null value.
>>>>>>>>> I guess we will need to file a new ccc request.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>> On 11.09.15 0:15, Semyon Sadetsky wrote:
>>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Please review fix for JDK9:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8080395
>>>>>>>>>>>>>> webrev: 
>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.00/
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This fix moves the caused property to
>>>>>>>>>>>>>> java.awt.event.FocusEvent to
>>>>>>>>>>>>>> make
>>>>>>>>>>>>>> it public. The sun.awt.CausedFocusEvent class is removed as
>>>>>>>>>>>>>> unnecessary.
>>>>>>>>>>>>>> The API change was approved 
>>>>>>>>>>>>>> http://ccc.us.oracle.com/8080395.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>