Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Semyon Sadetsky
On 4/15/2016 10:26 PM, Phil Race wrote: I touched on this several emails ago. An unresponsive URL may simply never produce an Image. If we wanted to handle it like that we could, but it would have to be explicit. And I don't know if you could justify the same for the method that takes a File.

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Sergey Bylokhov
On 15.04.16 21:49, Phil Race wrote: I think that even if no exception is thrown we ought to document what happens. Which of course means that we need to be sure that it does happen :-) And the difference depending on SM may be hard to defend. And I am referring to both methods, not just the URL

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Phil Race
I touched on this several emails ago. An unresponsive URL may simply never produce an Image. If we wanted to handle it like that we could, but it would have to be explicit. And I don't know if you could justify the same for the method that takes a File. -phil. On 04/15/2016 12:17 PM, Semyon

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Semyon Sadetsky
URL addresses are often taken from resources which may be unavailable for some reason. The question is what should we do with the app in this case: let it survive without showing the image (in the same way if the URL is unavailable) or let it die gracefully according to the specs. The second

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Alexander Scherbatiy
On 15/04/16 22:54, Sergey Bylokhov wrote: On 15.04.16 21:47, Alexander Scherbatiy wrote: If the second option is acceptable probably the Toolkit.getImage() javadoc can be updated. As far as I understand, we always throw NPE in case of SecManager, and throw NPE w/o secManager since

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Sergey Bylokhov
On 15.04.16 21:54, Sergey Bylokhov wrote: Also please take a look to the similar bug https://bugs.openjdk.java.net/browse/JDK-4358053 As usual I suggest to fail fast, and throw NPE. in Toolkit class and in ImageIcon, probably in some other places. The reason is that we already do that and I

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Sergey Bylokhov
On 15.04.16 21:47, Alexander Scherbatiy wrote: If the second option is acceptable probably the Toolkit.getImage() javadoc can be updated. As far as I understand, we always throw NPE in case of SecManager, and throw NPE w/o secManager since 7u40/8u25? Also please take a look to the similar

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Phil Race
I think that even if no exception is thrown we ought to document what happens. Which of course means that we need to be sure that it does happen :-) And the difference depending on SM may be hard to defend. And I am referring to both methods, not just the URL one. -phil. On 04/15/2016 11:47

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Alexander Scherbatiy
On 15/04/16 22:11, Phil Race wrote: And it prints the stack trace ?? This gets even messier. Is there anything we can do to have sensible specification of what happens with null that isn't likely to cause (much) harm ? I see only two options. Do not throw the NPE as it was before and throw

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Phil Race
And it prints the stack trace ?? This gets even messier. Is there anything we can do to have sensible specification of what happens with null that isn't likely to cause (much) harm ? -phil. On 04/15/2016 11:10 AM, Alexander Scherbatiy wrote: On 15/04/16 20:30, Sergey Bylokhov wrote: How the

Re: [9] Review request for JDK-8137137: [macosx] The native dialog doesn't have 'close'(X) button on Mac OS.

2016-04-15 Thread Phil Race
Hi, Good to get rid of one applet test. Only 1,499 to go :-) Even though you effectively re-wrote this you should set the (c) as a range "2007, 2016" Also I think we should try to use the natural size of the component rather than explicitly setting the size :- instructionFrame.setBounds(0,

Re: RFR: 8154269: Remove unused or unnecessary Xm/Xt files and header includes

2016-04-15 Thread Phil Race
On 04/15/2016 08:41 AM, Sergey Bylokhov wrote: Looks fine then, but mixing of int, jint and short in one method looks suspicious. A little, which is why I said "(as)" fine. I've double-checked and Intrinsic.h has : typedef short Position; /* Offset from 0 coordinate */

Re: [9] Review Request: 8154016 [macosx] Some HiDPI code can be removed

2016-04-15 Thread Phil Race
+1 -phil On 04/15/2016 09:22 AM, Sergey Bylokhov wrote: Hello, Please review the fix for jdk9. Noop code was removed. It was not removed before, because it was assumed that it can be reused for HiDPI support(but it was implemented differently) Bug:

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Sergey Bylokhov
How the object of URLImageSource(null) will be useful? I guess we will get some unspecified NPE exceptions if will try to call its methods? On 15.04.16 18:02, Alexander Scherbatiy wrote: Hello, Could you review the fix: bug: https://bugs.openjdk.java.net/browse/JDK-8132706 webrev:

Re: [9] Review Request: 8154016 [macosx] Some HiDPI code can be removed

2016-04-15 Thread Alexander Scherbatiy
The fix looks good to me. Thanks, Alexandr. On 15/04/16 20:22, Sergey Bylokhov wrote: Hello, Please review the fix for jdk9. Noop code was removed. It was not removed before, because it was assumed that it can be reused for HiDPI support(but it was implemented differently) Bug:

[9] Review Request: 8154016 [macosx] Some HiDPI code can be removed

2016-04-15 Thread Sergey Bylokhov
Hello, Please review the fix for jdk9. Noop code was removed. It was not removed before, because it was assumed that it can be reused for HiDPI support(but it was implemented differently) Bug: https://bugs.openjdk.java.net/browse/JDK-8154016 Webrev can be found at:

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Semyon Sadetsky
Looks good. --Semyon On 4/15/2016 6:02 PM, Alexander Scherbatiy wrote: Hello, Could you review the fix: bug: https://bugs.openjdk.java.net/browse/JDK-8132706 webrev: http://cr.openjdk.java.net/~alexsch/8132706/webrev.00 The fix makes the Toolkit.getDefaultToolkit().getImage(URL)

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Alexander Scherbatiy
On 15/04/16 19:26, prasanta sadhukhan wrote: Hi Alex, Just one observation. If url is null in getImageFromHash() then wouldn't we be getting NPE from checkPermissions(url) as it calls URLUtil.getConnectPermission(url); which calls url.toString().toLowerCase() so it will not come to your

Re: RFR: 8154269: Remove unused or unnecessary Xm/Xt files and header includes

2016-04-15 Thread Sergey Bylokhov
Looks fine then, but mixing of int, jint and short in one method looks suspicious. On 15.04.16 18:07, Philip Race wrote: I believe Position was a typedef of short so this should be (as) fine. -phil. On 4/15/16, 7:16 AM, Sergey Bylokhov wrote: Hi, Phil. Should the "Position" in

Re: [9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread prasanta sadhukhan
Hi Alex, Just one observation. If url is null in getImageFromHash() then wouldn't we be getting NPE from checkPermissions(url) as it calls URLUtil.getConnectPermission(url); which calls url.toString().toLowerCase() so it will not come to your check , right? String key = (url == null) ?

Re: RFR: 8154269: Remove unused or unnecessary Xm/Xt files and header includes

2016-04-15 Thread Philip Race
I believe Position was a typedef of short so this should be (as) fine. -phil. On 4/15/16, 7:16 AM, Sergey Bylokhov wrote: Hi, Phil. Should the "Position" in X11SurfaceData.c:X11SD_ClipToRoot() be replaced by int(or jint)? On 15.04.16 0:17, Phil Race wrote: I think it makes sense to remove

[9] Review request for 8132706 [macosx] Toolkit.getImage() throws NPE for null URL

2016-04-15 Thread Alexander Scherbatiy
Hello, Could you review the fix: bug: https://bugs.openjdk.java.net/browse/JDK-8132706 webrev: http://cr.openjdk.java.net/~alexsch/8132706/webrev.00 The fix makes the Toolkit.getDefaultToolkit().getImage(URL) return a ToolkitImage based on URLImageSource with null url as it was before

Re: RFR: 8154269: Remove unused or unnecessary Xm/Xt files and header includes

2016-04-15 Thread Sergey Bylokhov
Hi, Phil. Should the "Position" in X11SurfaceData.c:X11SD_ClipToRoot() be replaced by int(or jint)? On 15.04.16 0:17, Phil Race wrote: I think it makes sense to remove the typedef of Pixel and directly use unsigned long since the sole reference to Pixel is in order to prepare values to pass

Re: JDK-8031423 : Test java/awt/dnd/DisposeFrameOnDragCrash/DisposeFrameOnDragTest.java fails by Timeout on Windows

2016-04-15 Thread Yuri Nesterenko
+1 -yan On 04/15/2016 08:28 AM, Ajit Ghaisas wrote: Thanks Yuri. Here is the updated webrev with suggested change. http://cr.openjdk.java.net/~aghaisas/8031423/webrev.01/ Regards, Ajit -Original Message- From: Yuri Nesterenko Sent: Thursday, April 14, 2016 6:48 PM To: Ajit Ghaisas;

Re: RFR: 8154269: Remove unused or unnecessary Xm/Xt files and header includes

2016-04-15 Thread Semyon Sadetsky
Looks good. --Semyon On 4/15/2016 12:17 AM, Phil Race wrote: I think it makes sense to remove the typedef of Pixel and directly use unsigned long since the sole reference to Pixel is in order to prepare values to pass to XSetForeground which expects "unsigned long". Updated webrev :