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

Philip Race philip.race at oracle.com
Wed Apr 13 19:15:50 UTC 2016


+1

-phil.

On 4/13/16, 11:32 AM, Semyon Sadetsky wrote:
> The fix was updated with the specification for the class serialization 
> protocol changes which I forget to add:
> - the new cause field
> - readResolve() method
>
> The updated webrev: 
> http://cr.openjdk.java.net/~ssadetsky/8080395/webrev.06/
>
> --Semyon
>
>
> On 4/5/2016 9:48 AM, Semyon Sadetsky wrote:
>> On 4/4/2016 9:47 PM, Philip Race wrote:
>>> +1 (assuming only the one change, since I didn't re-review in 
>>> entirety.).
>>>
>>> Also assuming the answer to this :
>>>
>>> >  39     private static final long serialVersionUID = 
>>> -3647309088427840738L;
>>>
>>> > Is this the value generated by JDK 8 ? It should be ..
>>>
>>> was "yes".
>> Sorry I didn't get where is this number from. Now I see.
>> This UID was auto-generated by the java default serialization 
>> procedure for CausedFocusEvent.class since serialVersionUID was not 
>> specified for the class explicitly. It was generated with the 
>> CausedFocusEvent class appearance which happened earlier than JDK 8.
>> The reg-test has a special case to check the backward compatibility 
>> and it would fail if this number were incorrect.
>>
>> --Semyon
>>>
>>> -phil.
>>>
>>> On 4/4/16, 1:21 AM, Alexander Scherbatiy wrote:
>>>>
>>>>   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
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>


More information about the awt-dev mailing list