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
