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
