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

Reply via email to