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

Mon Apr 4 06:48:15 UTC 2016

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

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