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

List:       openjdk-2d-dev
Subject:    [OpenJDK 2D-Dev] [PATCH] FontManager refactoring, rev5
From:       Phil.Race () Sun ! COM (Phil Race)
Date:       2009-06-18 21:55:13
Message-ID: 4A3AB7C1.2060008 () sun ! com
[Download RAW message or body]

Roman,

Looks close. Just a few comments.

you aren't using these import in sun.font.FontManager :
   30 import java.util.Locale;
   31 import java.util.TreeMap;
   32
   33 import javax.swing.plaf.FontUIResource;

There may be other such cases but its easy to see in this much reduced file.


SunGraphicsEnvironment.java
173         assert fm instanceof FontManagerForSGE;
  174         return (FontManagerForSGE) fm;

I don't see the point of the assert here because the cast is
going to tell you if there's a problem whether asserts are on or not.

SharedSecrets.java

I think you can restore the empty line (115) you deleted and remove this
file from the webrev.

FcFontConfiguration.java

you aren't using the "fm" instance so can remove that line :
  331             SunFontManager fm = SunFontManager.getInstance();
  332             if (FontUtilities.debugFonts()) {
  333                 Logger logger = Logger.getLogger("sun.awt.FontConfiguration");
  334                 logger.warning("Exception identifying Linux distro.");
  335             }
  336         }
  337     }


WPathGraphics.java

I see you moved the definition of textLayoutIsCompatible in here.
Whilst its used only by this code, its also meaning that updating
that font internal code in the future will affect the printing code.
And the getDirectoryEntry() method isn't public so I'm not sure
how this will work! Ah, I just remembered you said you didn't test
on windows yet. Puzzlement over, this won't build. I'd prefer this
to go back to TrueTypeFont.
Clearly you need to get that windows build tested before you push.
You don't need to set up a windows machine, you can submit a jprt job
now that you have SWAN access.

-Phil.


Roman Kennke wrote:
> Finally I got back (with some post JavaOne pushing from Phil) to merging
> the newest stuff into my FontManager tree and here is the 5th revision
> of the FontManager refactoring patch:
> 
> http://cr.openjdk.java.net/~rkennke/fontmanager/webrev.05/webrev/
> 
> I think I implemented all suggestions from previous cycles and tried to
> be careful to also merge in all the latest bugfixes and improvements in
> Font related code from JDK7. I have smoke tested it on Linux. To be
> honest, I am not sure if it works well on Windows, but now that I have a
> Windows machine I will try this as soon as possible, but I was thinking
> that since the Windows specific part is rather small, that we can get
> the review rolling in parallel. So here we go.
> 
> Hey, do we actually already have a bug for this guy?
> 
> /Roman
> 
> 


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