<AWT Dev> <Awt Dev> [9] Review Request for 8080395: consider making sun.awt.CausedFocusEvent functionality public
Semyon Sadetsky
semyon.sadetsky at oracle.com
Wed Apr 13 18:32:00 UTC 2016
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