<AWT Dev> <Awt Dev> [9] Review Request for 8080395: consider making sun.awt.CausedFocusEvent functionality public
Philip Race
philip.race at oracle.com
Mon Apr 4 18:47:39 UTC 2016
+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".
-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