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
