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

Sat Jan 17 19:48:29 UTC 2009

Hi Phil,

Here we go with another round:

> PhysicalFont.java : US_LCID better belongs in TrueTypeFont.java
> since you moved the only uses of it to that file.

Done.

> 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

Done. I've removed the getPlatformFontVar() method altogether and pulled
the System.getenv() call in the same privileged block that checks the
property.

> 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 ?

Fixed.

> 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?

I've fixed this too. jreLibDirName and jreFontDirName have been moved to
SunFontManager before, so I changed the remaining references to these
fields to the SunFontManager fields. I moved eudcFontFileName and all
related methods completely to the Win32FontManager code. HOWEVER, since
I have no Windows machine at home, and at work is very difficult to do
anything useful on the few Win32 machines we have there, I've NOT tested
this. I tried to be as careful as possible, but it's quite likely that
I've forgot an import or similar. Would be very nice if somebody with a
working Windows setup could test this stuff.

>  >> 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().

I removed these isCopy things. Somehow, I tried to be close in behaviour
to the original behaviour, but it actually doesn't make any sense (only
closing channels if somebody called createFont(InputStream) ? Must be a
mistake). I hope that this is what you were critizing, no?

>  >> 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 did just what you proposed, with one little omission:

>      static {
>          StrikeCache.unsafe.ensureClassInitialized(Font.class);
>      }

I don't think that is needed. All the methods take a Font argument, so
any caller must:
- either already have a Font object, in which case the Font class is
already loaded - no problem here.
- pass null, in which case we end up with an NPE (because the accessor
field is still null). But calling this stuff with null would result in
an NPE anyway, so also no problem here.

Correct me if I'm wrong.

The new webrev is here:

http://kennke.org/~roman/fontmanager3/webrev/

Thanks for taking your time.

/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