<AWT Dev> <Awt Dev> [9] Review Request for 8080395: consider making sun.awt.CausedFocusEvent functionality public
Semyon Sadetsky
semyon.sadetsky at oracle.com
Tue Apr 5 06:48:23 UTC 2016
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