[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] [PATCH] FontManager refactoring
From:       roman.kennke () aicas ! com (Roman Kennke)
Date:       2008-12-10 14:13:37
Message-ID: 1228918417.7001.73.camel () moonlight
[Download RAW message or body]

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



[prev in list] [next in list] [prev in thread] [next in thread]