[11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics

[Kevin Rushforth]

This looks fine to me. Please add this info to JBS when you get a 
chance, and also link the new bug to this bug.

The fix looks fine to me, although I was puzzled by the following:

-        return
-            System.getProperty("java.home","") + File.separator +
-                "lib" + File.separator + "fonts" + File.separator;
+        return System.getProperty("java.home","") + File.separator +
+                "lib" + File.separator + "fonts";


Your fix preserves existing behavior, given that the return value from 
the now-eliminated call to the JDKFontLookup class did not have the 
trailing File.separator, but I think that might be a bug given how the 
returned string from this method is used elsewhere in this file. Since 
this is preexisting, you might choose to address that as well in the 
follow-up bug (JDK-8198752). If so, please add a comment to that bug.

+1 from me (you need a +1 from Phil as well)

-- Kevin


Ajit Ghaisas wrote:
>
> Hi Phil,
>
>  
>
>    All my webrev does is to replace the way of obtaining name of JDK 
> font directory – and as rightly pointed by you below (first two lines 
> of your last reply) – it is what is needed to get rid of dependency on 
> sun.font.lookup.
>
>    I have tested it by running an OpenJDK build that doesn't contain a 
> lib/fonts directory using Ensemble8 sample.
>
>    
>
>     I got your point about Lucida Sans fonts being pointless with 
> openJDK & the font finding fallback code is being little dubious.
>
>     I would like to address that separately. I have filed bug 
> JDK-8198752 <https://bugs.openjdk.java.net/browse/JDK-8198752> to 
> handle that.
>
>  
>
> Regards,
>
> Ajit
>
>  
>
> *From:* Phil Race
> *Sent:* Thursday, February 15, 2018 10:44 PM
> *To:* Ajit Ghaisas
> *Cc:* Kevin Rushforth; openjfx-dev at openjdk.java.net
> *Subject:* Re: [11] Review request : JDK-8195806 : Eliminate 
> dependency on sun.font.lookup in javafx.graphics
>
>  
>
> This part is probably as good as you can do
>
> +        return System.getProperty("java.home","") + File.separator +
> +                "lib" + File.separator + "fonts";
>
>
> but if we want to support running against OpenJDK which does not have 
> Lucida Sans
> then we need to look at the caller of getJDKFontDir()
>
> So going forward all of this ..
>
>     private static final String jreDefaultFont   = "Lucida Sans Regular";
>     private static final String jreDefaultFontLC = "lucida sans regular";
>     private static final String jreDefaultFontFile = "LucidaSansRegular.ttf";
>
> .. is probably pointless.
>
> And if we can't find it then the first layer of fall back code is a 
> bit dubious
>
>     // Normally use the JRE default font as the last fallback.
>                 // If we can't find even that, use any platform font;
>                 for (String font : fontToFileMap.keySet()) {
>                     String file = findFile(font); // gets full path
>                     fontResource = createFontResource(jreDefaultFontLC, file);
>                     if (fontResource != null) {
>                         break;
>                     }
>  
> It did not really matter in the past .. it was just to prevent NPE in an unlikely scenario ..
> But if it is to be the first class way of finding this font it probably should be using
> "System" instead ? But then you need to make sure we don't have a circularity in the case
> that we are using this in initialiasing System.
>  
> -phil.
>
>  
>
> On 02/14/2018 09:39 PM, Ajit Ghaisas wrote:
>
>     Hi Phil,
>
>      
>
>        Thanks for having a look at the changes.
>
>        
>
>        I have little difficulty in understanding your question.
>
>        Are you referring to the sun.font.SunFontManager initialization?
>
>        I am asking as it is not evident from the code changes that I have done as part of this webrev.
>
>      
>
>        Request your guidance in this regard.
>
>           
>
>     Regards,
>
>     Ajit   
>
>      
>
>      
>
>     -----Original Message-----
>
>     From: Philip Race 
>
>     Sent: Wednesday, February 14, 2018 12:52 PM
>
>     To: Ajit Ghaisas
>
>     Cc: Kevin Rushforth; openjfx-dev at openjdk.java.net <mailto:openjfx-dev at openjdk.java.net>
>
>     Subject: Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
>
>      
>
>     Well that removes the dependency but how are you fixing the problem of how else to find the font ?
>
>     There needs to be alternate code or you need to go back to see what would happen if some code tried to locate that font and how it would fail.
>
>      
>
>     -phil.
>
>      
>
>     On 2/13/18, 11:11 PM, Ajit Ghaisas wrote:
>
>         Hi Kevin, Phil,
>
>          
>
>                Request you to review following fix :
>
>          
>
>                Issue :  https://bugs.openjdk.java.net/browse/JDK-8195806
>
>          
>
>                Fix :  
>
>         http://cr.openjdk.java.net/~aghaisas/fx/8195806/webrev.0/ <http://cr.openjdk.java.net/%7Eaghaisas/fx/8195806/webrev.0/>
>
>          
>
>         Regards,
>
>         Ajit
>
>  
>