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

[Sergey Bylokhov]

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.

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


-- 
Best regards, Sergey.