[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-11-14 10:34:37
Message-ID: 1226658877.8026.18.camel () moonlight
[Download RAW message or body]

Hi Phil,

> Some comments (at last) :

Thanks!

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

Yeah, the refactoring isn't complete at all, but I wanted to push what
we have now, so that future (parallel) development in the font code,
like what you did back then, doesn't get in the way so much. I think I
spent one or two whole days merging your changes into this patch.

> The FontConfigManager class should go with it into the X11 side of the code.

I agree.

> 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'll do that.

> 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?

I think I don't understand this question.

> Is DefaultFontManager really necessary?? Seems not to me.

Not really. I think I introduced it because I didn't like to have native
methods in SunFontManager, but those native methods can be moved down
to the platform classes just as well. I'll fix this.

> 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

Oh yeah, those probably slipped in accidentally. I'll remove them.

> 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

Right.

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

Actually, what I really would like to have here is to use the
SharedSecrets mechanism to be used here. This has a number of
advantages: no reflection/JNI is used at all, we have compile time
checking, and performance is most likely optimal (no reflection and no
java<->jni transitions). If you agree with that, I'll implement this
instead. SharedSecrets can also be used in many other places where
reflection or JNI is used at the moment.


> 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

Ok, then we'll leave them in. Should we change the comment then to sth
like 'we leave this in forever for a licensee'?

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

Ok, no problem.

> 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

Ok, no problem.

> CompositeFont.java
>   57     private SunFontManager fm = null;
> 
> I don't see why every comp-font needs a ptr to this singleton.

I think this slipped in during refactoring because we'd get an
initialization loop otherwise. I think we can remove this now, but I
need to check this.

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

Yeah right.

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

I was thinking about where to move this code and since it was only used
in TrueTypeFont, I moved it there. What do you think would be a good
place? The FontManager interface? Or FontUtilities? Or maybe a new think
(FontPool??)?

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

You sure? We add fonts to the pool also in other cases, for each font we
open. We don't want to cleanup this pool?

> FontManager.java has a lot of "TODO" comments

Ooops. I will fill them in.

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

Then a SunGraphicsEnvironment based graphics impl would require a
SunFontManager based font impl. In my own graphics implementation that
I'm doing, I want to use SunGraphicsEnvironment, but not SunFontManager,
because all the font lookup and handling is completely different there
(I need to load fonts from system resources - JAR, Classpath, etc - and
not from filesystem, because there is no filesystem on some of my target
systems). If you don't mind, I'd like to keep it that way.

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

Good question :-) That's probably already solved and should be removed.

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

I agree. I'll see if this can be removed completely.


Thanks alot Phil, I'll fix the suggested stuff and post another webrev.

Cheers, Roman

>    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
-- 
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]