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
