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

[Phil Race]

Some comments (at last) :

 > 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, 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.


It would be nice if each of these new classes had its charter documented
at the top, in a class comment.
SunFontManager
DefaultFontManager
X11FontManager
Win32FontManager
FontConfigManager
FontUtiltities
FontManagerFactory
FontScaler

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?

Is DefaultFontManager really necessary?? Seems not to me.

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

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.

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


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.

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         }

BTW code conventions are at
http://java.sun.com/docs/codeconv/html/CodeConvTOC.doc.html

CompositeFont.java
  57     private SunFontManager fm = null;

I don't see why every comp-font needs a ptr to this singleton.

PhysicalStrike.java
- 29 import java.awt.GraphicsEnvironment;
this isn't used

====

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.


FontManager.java has a lot of "TODO" comments

Why is FontManagerForSGE needed?
ie why cannot SunFontManager be the class in which these are defined.

Win32FontManager :
   22     // FIXME: Windows build still needs to be abstracted from
   23     // SunGraphicEnvironment
   24
  What does that mean?

   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;
   29

-phil.
Roman Kennke wrote:
> Hi,
> 
> 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:
> 
> - FontManager: is now a relatively small interface. This is the part
> that the AWT API classes (esp. java.awt.Font) talk to.
> - FontManagerForSGE: A subinterface of FontManager.
> SunGraphicsEnvironment uses this. If you implement a backend based on
> SGE, then you also need to implement this. Otherwise you can go with
> plain FontManager.
> - SunFontManager: A base implementation of FontManager(ForSGE). This has
> all the shared code, a lot of stuff that was previously in FontManager
> has been moved there.
> - X11FontManager, Win32FontManager: The platform specific stuff went
> there.
> - FontManagerFactory: Creates FontManager instance according to a
> property or default.
> - SunGraphicsEnvironment: Almost all font-related code has been moved
> out of this class.
> - FontUtilities: A new utility class. A couple of things from
> FontManager went there, i.e. logging, access tricks (get/setFont2D()),
> OS determination and general shared stuff.
> 
> For the most part, this was only moving around code, without changing
> functionality. However, in some places it was necessary or seemed useful
> to change some things:
> 
> - in sunFont.c we can now call the real isDisplayLocal() on the graphics
> environment.
> - in TrueTypeFont, the handling of the channel pool has been improved.
> Before, the cleanup-hook was only initialized when somebody called
> Font.createFont(), now it gets initialized whenever a channel is added
> to the pool. Is slightly cleaner than before (although I guess it
> doesn't matter much, since modern OSes cleanup resources quite well
> anyway).
> - the FontManager.usePlatformFontMetrics() for windows flag has been
> removed. I don't know if this is feasible, but the comments seemed to
> indicate that this was the plan anyway. Might break some obscure apps
> that rely on buggy code.
> 
> These are all functional changes I can think of from the top off my
> head.
> 
> The webrev for this is here:
> 
> http://kennke.org/~roman/fontmanager/webrev/
> 
> The raw patch can also be downloaded somewhere in the webrev.
> 
> I'd be happy to hear your comments soon. Note that I did only very basic
> testing on Windows. It is hellish to setup a build on windows,
> especially when you don't have the resources to buy the necessary
> licenses... Would be nice if somebody could test the stuff on Windows a
> bit more. I hope that this patch is feasible to be included in OpenJDK
> mainline, or that we can make it so...
> 
> Cheers, Roman