[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: [11] Review request : JDK-8195806 : Eliminate dependency on sun.font.lookup in javafx.graphics
From:       Kevin Rushforth <kevin.rushforth () oracle ! com>
Date:       2018-02-28 0:00:49
Message-ID: 5A95F131.70502 () oracle ! com
[Download RAW message or body]

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@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@openjdk.java.net \
> <mailto:openjfx-dev@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
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]