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
