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

[Semyon Sadetsky]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/awt-dev/attachments/20160323/822e34fa/attachment.html>