[OpenJDK 2D-Dev] [PATCH] FontManager refactoring
Phil Race
Phil.Race at Sun.COM
Thu Jan 15 01:15:30 UTC 2009
Roman,
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.
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.
Frankly I'd like to delete the whole thing but as per the previous webrev comments
it was retained there for a licensee customer.
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 ?
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'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().
>> 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.
I'm supposing that instead of your interface etc you have this
or something similar :
package sun.font;
import java.awt.Font;
public abstract class FontAccessor {
static {
StrikeCache.unsafe.ensureClassInitialized(Font.class);
}
static FontAccessor fontAccessor;
public static FontAccessor getFontAccessor() {
return fontAccessor;
}
public static void setFontAccessor(FontAccessor fa) {
if (fontAccessor != null) {
throw new InternalError("Attempt to set FontAccessor twice");
}
fontAccessor = fa;
}
public abstract Font2D getFont2D(Font font);
public abstract void setFont2D(Font font, Font2DHandle handle);
public abstract void setCreatedFont(Font font);
public abstract boolean isCreatedFont(Font font);
}
-phil.
Roman Kennke wrote:
> Hi Phil,
>
> I finally got around to fix up the font manager stuff. Some comments
> from me inline.
>
>> > this is the big FontManager refactoring patch I already mentioned a
>> > couple of times. It's primary purpose is to make the font implementation
>> > more portable (or: portable at all), allowing alternative/derived
>> > implementations to be plugged in. The general architecture breaks down
>> > as follows:
>>
>> Plugging in alternate architectures is somewhat interesting but the refactoring
>> priority I see is that the FontManager class which has grown too large and
>> needs to be "downsized" This is done, which is OK,
>
> Well, for me it's equally important to plug in something else. On the
> systems I work on I have no system fonts available, so I'd like them to
> be built in and loaded from the application JAR instead. Works quite
> nice until now, although it needs some more work in the fontmanager
> code, but this should come after this patch here is through.
>
>> and the most important
>> part of how this is done is to separate out the platform, and that's partly
>> handled but, not as much as I'd like. For example : SunFontManager.java :
>> getDefaultPlatformFont() seems like a perfect candidate for pushing
>> down into the platform code.
>> The FontConfigManager class should go with it into the X11 side of the code.
>
> I did this now. The fontmanager stuff is now in the solaris tree, and
> the getDefaultPlatformFont() abstract in SFM and implemented in both
> platform subclasses.
>
>
>> It would be nice if each of these new classes had its charter documented
>> at the top, in a class comment.
>
> Fixed. I only added some rather small comments, probably needs
> extension. The thing is that to me it's pretty clear what each of these
> classes should do, would be nice to have somebody else look at it and
> see what is missing.
>
>> I guess previously we had an X11/Win32GE instance for API reasons, but when the
>> font code was moved out to the FontManager subclasses it maybe wasn't
>> needed to make that also have an instance?
>
> TBH, I don't understand this question. Have instance of what?
>
>> Is DefaultFontManager really necessary?? Seems not to me.
>
> Removed. This was an artifact from me not wanting to have native stuff
> in SFM. Now the native stuff is simply moved to the platform
> implementations.
>
>> Also a number of changes appear unrelated, eg :
>> make/java/nio/Makefile
>> make/java/nio/spp.sh
>> src/share/classes/com/sun/jmx/mbeanserver/DefaultMXBeanMappingFactory.java
>> src/share/classes/com/sun/jmx/mbeanserver/MBeanIntrospector.java
>
> Also an artifact of me fixing broken stuff in other places. I removed
> this.
>
>> 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
>
>> src/share/classes/java/awt/Component.java, and
>> src/windows/classes/sun/awt/windows/WToolkit.java
>> This was left in for a licensee, I'd be inclined to leave it alone
>> unless removing it has definite benefits.
>> 2770 // REMIND: PlatformFont flag should be obsolete soon...
>> 2771 if (sun.font.FontManager.usePlatformFontMetrics()) {
>> 2772 if (peer != null &&
>> 2773 !(peer instanceof LightweightPeer)) {
>> 2774 return peer.getFontMetrics(font);
>> 2775 }
>> 2776 }
>> 2777 return sun
>
> I put it back in, including a comment to not remove it.
>
>> We don't need these and similar changes, since they aren't pre-defined constants
>> :
>> isSolaris->IS_SOLARIS, isLinux->IS_LINUX
>>
>> ie all caps is just for pre-defined constants, not ones determined at run time.
>
> Fixed.
>
>> Cmap.java
>> Please try to keep lines <=80 chars, eg line 411 here :
>> 410 if (FontUtilities.isLogging()) {
>> 411 FontUtilities.getLogger().warning("Cmap subtable overflows
>> buffer.");
>> 412 }
>> 413 }
>
> Hmm, How can I check all my code for this?
>
>> CompositeFont.java
>> 57 private SunFontManager fm = null;
>>
>> I don't see why every comp-font needs a ptr to this singleton.
>
> Removed.
>
>> PhysicalStrike.java
>> - 29 import java.awt.GraphicsEnvironment;
>> this isn't used
>
> Removed.
>
>> 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.
>
>> FontManager.java has a lot of "TODO" comments
>
> I added the missing comments.
>
>> Why is FontManagerForSGE needed?
>> ie why cannot SunFontManager be the class in which these are defined.
>
> I think I already replied to this. This is needed when an implementation
> wants to subclass SunGraphicsEnvironment but not reuse SunFontManager,
> just like I do. I have a fairly close GraphicsEnvironment, but a
> completely different FontManager here.
>
>> Win32FontManager :
>> 22 // FIXME: Windows build still needs to be abstracted from
>> 23 // SunGraphicEnvironment
>> 24
>> What does that mean?
>
> Dunno. :-) I removed it.
>
>> 25 // please, don't reference sgEnv in any other code in this class
>> 26 // use getGraphicsEnvironment.
>> 27 @Deprecated
>> This doesn't seem an appropriate use of deprecated.
>
>> 28 private static SunGraphicsEnvironment sgEnv = null;
>
> Infact, this fields wasn't even used anymore, so I removed it.
>
> The updated webrev can be found here:
>
> http://kennke.org/~roman/fontmanager/webrev/
>
> Have fun, Roman
>
More information about the 2d-dev
mailing list