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

[Phil Race]

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

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