[OpenJDK 2D-Dev] [PATCH] FontManager refactoring

[Roman Kennke]

Hi Phil,

> I've only gone through maybe 35-40% of the changes but here's my comments
> so far :
> 
> PhysicalFont.java : US_LCID better belongs in TrueTypeFont.java
> since you moved the only uses of it to that file.

Yeah right. I'll fix.

> Since you are touching this, and so much else, maybe the following clean up too :
> 
> sunFont.c :
> 
>   74 /*
>    75  * Class:     sun_font_FontManager
>    76  * Method:    getPlatformFontVar
>    77  * Signature: ()Z
>    78  */
>    79 JNIEXPORT jboolean JNICALL
>    80 Java_sun_font_SunFontManager_getPlatformFontVar(JNIEnv *env, jclass cl) {
>    81     char *c = getenv("JAVA2D_USEPLATFORMFONT");
>    82     if (c) {
>    83         return JNI_TRUE;
>    84     } else {
>    85         return JNI_FALSE;
>    86     }
>    87 }
>    88
> 
> 
> 
> SunFontManager.java
>   560         String prop = AccessController.doPrivileged(
>   561                          new GetPropertyAction("java2d.font.usePlatformFont"));
>   562         if (("true".equals(prop) || getPlatformFontVar())) {
>   563             usePlatformFontMetrics = true;
>   564             System.out.println("Enabling platform font metrics for win32. This is an unsupported option.");
>   565             System.out.println("This yields incorrect composite font metrics as reported by 1.1.x releases.");
>   566             System.out.println("It is appropriate only for use by applications which do not use any Java 2");
>   567             System.out.println("functionality. This property will be removed in a later release.");
>   568         }
>   569     }
> 
> 
> Once upon a time System.getEnv() was considered impure and didn't do anything.
> Now may be it would be better to scrap the native method and just call it from the Java code
> above that presently calls the native method which can then be removed.
> There's already a wealth of privileged blocks in the area that its used so perhaps you
> could just move this all inside one of them, as that is necessary.

I will do.


> SunFontManager.java
> 
>   238     /**
>   239      * Returns the global FontManagerBase instance. This is similar to
>   240      * {@link FontManagerFactory#getInstance()} but it returns a
>   241      * FontManagerBase instance instead. This is only used in internal classes
>   242      * where we can safely assume that a FontManagerBase is to be used.
>   243      *
>   244      * @return the global FontManagerBase instance
> 
> I suppose you changed the name of the class, so the comment should
> change too ?

Oh yeah. Will fix. SunFontManager used to be called FontManagerBase in
its first incarnation.

> SunGraphicsEnvironment :
>    84
>    85     // TODO: Refactor the follow 3 fields into FontManager API.
>    86     public static String jreLibDirName;
>    87     public static String jreFontDirName;
>    88
>    89     public static String eudcFontFileName; /* Initialised only on windows */
>    90
> 
> Are you planning on doing these?

Yeah sure. Last time I touched these it caused a dreaded initialization
loop, so I left them alone. I will try again.

>  >> I'm not sure about the movement of addToPool() to TrueTypeFont.java
>  >> Theoretically its independent of the font format, and could apply
>  >> to any FileFont subclass.
>  >> also the file closer shut down hook isn't needed except for created
>  >> fonts from tmp files, so that shouldn't be started unless needed.
>  >
>  > I moved this code to a new class called FontChannelPool, and only start
>  > the hook when needed.
> 
> I see that in FontChannelPool.java you start the closer when the TT font is a copy.
> I don't have your previous webrev any more to compare but isn't
> quite what I expected. The "closer" hook is so we can delete
> files, and its started from the createFont2D() code, which is
> the only case when we'd want to delete the files. You even
> left it in there, called fileCloser, but it no longer closes
> the files, just deletes them.
> 
> So: there should be no need to add the "isCopy" to TrueTypeFont,
> nor to pass a variable to addToPool().

Erm. I think I just became a little confused about this. I will look at
it again and fix it.

>  >> make/sun/font/mapfile-vers
>  >> The disappearance of these seems strange  ..
>  >>    40                 Java_sun_font_FontManager_getFont2D;
>  >>    41                 Java_sun_font_FontManager_setFont2D;
>  >>    42                 Java_sun_font_FontManager_isCreatedFont;
>  >>    43                 Java_sun_font_FontManager_setCreatedFont;
>  >>   .. ah, I see these are now using reflection In FontUtilities
>  >> Hmm. I don't think this change should be made
>  >> 1) As it stands it'll get a SecurityException
>  >> 2) I'm very dubious about changing the accessibility of these internal methods
>  >> Looks like a potential security hole.
>  >> 3) I'd want to such a change separately anyway to test its relative performance.
>  >
>  > As mentioned earlier, I think the best solution to this code is the
>  > SharedSecrets mechanism, which has the advantage to be
>  > compile-time-safe, doesn't need access checks, should be secure and
>  > should have best performance (no JNI call, no reflection call).
> 
> Jim Graham pointed out this out a few years ago when he implemented a
> similar change in the Image class (as Dmitri says he noted on your blog)
> but I never did get around to making the change.
> 
> So its fine in principle but I don't see any advantage to putting
> it into sun.misc.SharedSecrets and having the font implementation
> aware JavaFontAccess interface in sun.misc. Instead I think its better
> to do it directly as Jim did. Create an abstract class directly in
> the sun.font package like this although you can also have FontUtilities
> reference the package access "fontAccessor" variable directly
> knowing that the class initialisation should take care of setting it.

Yeah right, I also came to this conclusion lately. SharedSecrets is
nice, but it's better to implement something in sun.font so that the
code does not become accessible.

I will incorporate the suggested fixes and send another webrev (this
time with version number so you can compare ;-) ).

Cheers, Roman

-- 
Dipl.-Inform. (FH) Roman Kennke, Software Engineer, http://kennke.org
aicas Allerton Interworks Computer Automated Systems GmbH
Haid-und-Neu-Straße 18 * D-76131 Karlsruhe * Germany
http://www.aicas.com   * Tel: +49-721-663 968-48
USt-Id: DE216375633, Handelsregister HRB 109481, AG Karlsruhe
Geschäftsführer: Dr. James J. Hunt