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

[Roman Kennke]

What's the status of this?

/Roman

Am Mittwoch, den 10.12.2008, 15:13 +0100 schrieb Roman Kennke:
> 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).
> 
> > 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
> 
-- 
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