Looks fine, thanks.
On 05.04.16 18:02, Jayathirth D V wrote:
Hi Sergey,
It will be pushed after removing the redundant code.
Please find the webrev:
http://cr.openjdk.java.net/~jdv/8044289/webrev.07/
Thanks,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Tuesday, April 05, 2016
Hi Sergey,
It will be pushed after removing the redundant code.
Please find the webrev:
http://cr.openjdk.java.net/~jdv/8044289/webrev.07/
Thanks,
Jay
-Original Message-
From: Sergey Bylokhov
Sent: Tuesday, April 05, 2016 8:05 PM
To: Jayathirth D V
Cc: 2d-dev@openjdk.java.net
Subject:
On 30.03.16 9:48, Jayathirth D V wrote:
Thanks for pointing it out. It looks like redundant try-catch block.
There is small code clean up is possible after your latest change:
1587 ImageOutputStream stream = null;
1588 stream = createImageOutputStream(output);
I guess such code is redundant
Hi Sergey,
Thanks for pointing it out. It looks like redundant try-catch block.
I have removed these blocks in write() methods so that we are handling "cant
create cache file" exception in common way in all read() and write() methods.
Also I have updated test case description to hold true for
Hi Phil,
Yes I agree with that. This test case is only for those read() and write()
functions which tend to use createImageXXXStream() and not for those which get
stream directly as part of argument and follow the spec. Thanks for your inputs.
-Jay
From: Phil Race
Sent: Tuesday,
This is OK but you ought to remember that read() and write() only HAPPEN
to use createImageXXXStream(). They could get the stream in some other way
and still be a spec. compliant implementation.
-phil.
On 03/29/2016 02:27 AM, Jayathirth D V wrote:
Hi Prasanta,
Actually returns null in test
I am not sure what is the reason of this code?
1546 try {
1547 output.delete();
1548 stream = createImageOutputStream(output);
1549 } catch (IOException e) {
1550 throw e;
1551 }
On 29.03.16 1:12, Philip Race wrote:
This all looks fine
Looks ok.
Regards
Prasanta
On 3/29/2016 2:57 PM, Jayathirth D V wrote:
Hi Prasanta,
Actually returns null in test description is for
createImageXXXStream() and not for read() and write() functions. But
for more clarity I have added why we catch IOException also in test
description.
Hi Prasanta,
Actually returns null in test description is for createImageXXXStream() and not
for read() and write() functions. But for more clarity I have added why we
catch IOException also in test description.
Please review the updated webrev:
+1.
Looks ok to me. Only thing is that whether the test summary is appropriate "
Test verifies that when createImageInputStream() or
28 * createImageOutputStream() returns null while read or write,"
Should it not be "verifies whether ...throws appropriate exception"?
Regards
This all looks fine to me. Although no new Exception types are thrown
I think we need a CCC anyway, so please prepare and get that approved
before pushing.
-phil.
On 3/28/16, 3:30 AM, Jayathirth D V wrote:
Hi Phil,
After discussion with Sergey we have come to conclusion that Sergey is
okay
Hi Phil,
We can change the code to something like :
1522 * @return false if no appropriate writer is found or
1523 * when needed stream is null.
1545 try {
1546 output.delete();
1547 stream = createImageOutputStream(output);
1548 }
I don't think this is ready and we need to discuss whether to rework it.
In general I think I prefer IIO rather than null when we have a stream problem.
For one thing you have this change :-
1522 * @returnfalse if no appropriate writer is found or
1523 * not able to create required
Thanks for the review Sergey.
Can I get +1 for this please?
-Jay
-Original Message-
From: Sergey Bylokhov
Sent: Tuesday, March 22, 2016 9:52 PM
To: Jayathirth D V; Philip Race
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review Request for JDK-8044289 : In
ImageIO.read()
This fix looks fine to me.
At least it made all code work in a similar way. But probably someone
will have other opinion?
On 22.03.16 12:34, Jayathirth D V wrote:
Hi Sergey,
I have unified changes related to ImageIO.read() and ImageIO.write().
In case of read() we will be returning null
Hi Sergey,
I have unified changes related to ImageIO.read() and ImageIO.write().
In case of read() we will be returning null when createImageInputStream()
returns null.
In case of write() we will be returning false when createImageOutputStream()
returns null.
Please find updated webrev for
Hi, Jay.
Since we decided to update the specification we get an additional
flexibility in the fix, because we can update the "@exception
IOException" part of javadoc and describe that it can be thrown if
ImageXXXStream cannot be created.
I don't see a big differences of both approaches in
Hi Sergey,
For the second approach I have created webrev for review.
Please review updated webrev :
http://cr.openjdk.java.net/~jdv/8044289/webrev.02/
Thanks,
Jay
-Original Message-
From: Jayathirth D V
Sent: Friday, March 18, 2016 2:23 PM
To: Sergey Bylokhov
Cc:
I don't think that is the actual reason here - it seems likely that there
was some kind of internal error or bad SPI causing this.
So throwing an exception seems more appropriate.
And also the bug was originally solely about write(..).
-phil.
On 03/16/2016 01:35 PM, Sergey Bylokhov wrote:
As
Hi Sergey,
My Observations:
There are many places in createImageInputStream() and createImageOutputStream()
where it will return null for stream.
So for NPE described in bug to occur there can be multiple paths:
1) One of the path described in bug is when theRegistry.getServiceProviders()
The test writes out into "test.src".
I believe that you should treat that as a "read-only" location.
Write to a tempfile (File.createTempFile()) or TESTCLASSES.
-phil.
On 3/15/16, 10:50 PM, Jayathirth D V wrote:
Hi Phil,All
_Please review the following fix in JDK9:_
__
Bug :
Hi Phil,
Thanks for the review.
I have made following changes :
1) I am using File.createTempFile() both in case of read and write as a result
of which we don't need external .png file which was there in previous
webrev.
2) Also since I am using temporary file
As far as I understand the createImageInputStream() returns null is this
stream is unsupported by current plugins via spi. Probably the read()
method should return null too?
/**
* Returns a {@code BufferedImage} as the result of decoding
* a supplied {@code InputStream} with an
On 17.03.16 0:36, Phil Race wrote:
I don't think that is the actual reason here
But the test doing exactly this thing -> deregister all spi => in this
case according to specification all related
methods(read/write/creatImageInputStream/creatImageOutputStream) should
return null or false.
Hi Sergey,
Thanks for your inputs.
Indeed all the null values returned by createImageInputStream() and
createImageInputStream() are valid only.
I went through the code more and found that there is no need for fix in
ImageIO.read() methods as null stream is handled in read(ImageInputStream
25 matches
Mail list logo
