Re: [OpenJDK 2D-Dev] [8] Review request for 8011059 [macosx] Make JDK demos look perfect on retina displays

[Alexander Scherbatiy]

 Could you review the updated fix:
    http://cr.openjdk.java.net/~alexsch/8011059/webrev.06/
  Only internal API is exposed:
  - MultiResolutionImage interface with method 
"getResolutionVariant(int width, int height)" is added to the 
com.sun.awt package
  - Hints to switch on/off the resolution variant usage are added to 
SunHints class
  - Test is updated to use the  MultiResolutionImage interface

 Thanks,
 Alexandr.

On 11/5/2013 3:16 PM, Alexander Scherbatiy wrote:

   Thank you for the review.

  Could you look at the updated fix:
     http://cr.openjdk.java.net/~alexsch/8011059/webrev.05/

  - URL is parsed to protocol, host, port, and path parts in the 
LWCToolkit class.
     I checked that URL(protocol, host, port, file) constructor 
correctly handles -1 port value.
  - Only last file name after last '/' in the URL path  is converted 
to @2x name
  - Tests that check correct URL and path translation to @2x names are 
added to the ScalableImageTest

  Thanks,
  Alexandr.


On 11/1/2013 12:46 AM, Peter Levart wrote:

On 10/29/2013 05:45 PM, Alexander Scherbatiy wrote:
2. I'm not sure that the proposed getScaledImageName() 
implementation in ScalableToolkitImage works perfectly for URLs 
like this:

http://www.exampmle.com/dir/image

In this case it will try to find 2x image here:

http://www.exam...@2x.com/dir/image

which doesn't look correct.
       Fixed. Only path part of a URL is converted to path2x. 

Hi Alexander,

URLs like this:

http://www.example.com/dir.ext/image

will still be translated to:

http://www.example.com/d...@2x.ext/image


I think you need to search for last '.' after the last  '/' in the 
getScaledImageName();


Also the following code has some additional bugs:

  853         static Image toScalableImage(Image image, URL url) {
  854
  855             if (url != null && !url.toString().contains("@2x")
  856                     && !(image instanceof ScalableToolkitImage)) {
  857                 String protocol = url.getProtocol();
  858                 String host = url.getHost();
  859                 String file = url.getPath();
  860                 String file2x =*host +*getScaledImageName(file);
  861                 try {
  862                     URL url2x = new URL(protocol, host, file2x);
  863                     url2x.openStream();
  864                     return new ScalableToolkitImage(image, 
getDefaultToolkit().getImage(url2x));
  865                 } catch (Exception ignore) {
  866                 }
  867             }
  868             return image;
  869         }

Why are you prepending *host* to getScaledImageName(file) in line 
860? Let's take the following URL for example:

http://www.example.com/dir/image.jpg

protocol = "http"
host = "www.example.com"
file = "/dir/image.jpg"
file2x = "*www.example.com*/dir/im...@2x.jpg"
url2x = URL("http://www.example.com*www.example.com*/dir/im...@2x.jpg";)


You are missing a part in URL (de)construction - the optional port! 
For example in the following URL:

http://www.example.com:8080/dir/image.jpg

You should extract the port from original URL and use it in new URL 
construction if present (!= -1).


I would also close the stream explicitly after probing for existence 
of resource rather than delegating to GC which might not be promptly 
and can cause resource exhaustion (think of MAX. # of open file 
descriptors):

        try (InputStream probe = url.openStream()) {}



Regards, Peter