[OpenJDK 2D-Dev] Review Request for 6879044

[Mandy Chung]

Alan, Thanks for the review.  I'll send out a new webrev to incorporate 
your feedback. Yes, I file a CR for the http protocol handler:
    6882384: Update http protocol handler to use PlatformLogger

Several 2D and sun.font classes are refactored in the 2D repository that 
will be integrated for b74.  Some classes I modified in the 2D repo are 
not in TL repo.  To simplify the integration, I file a new CR 6882376 
for the core-libs changes including changes in java.util classes that 
will go in TL repo that I think I can make b73.  The AWT/2D/Swing 
changes will go in 2D repo as a fix for 6879044.

I'll generate two new webrevs to separate the core-libs and awt/2d/swing 
stuff for the review.

Thanks
Mandy

Alan Bateman wrote:
> Mandy Chung wrote:
>> 6879044: Eliminate the dependency of logging from the JRE 
>> core/awt/swing classes
>>
>> Webrev:
>>   http://cr.openjdk.java.net/~mchung/6879044/webrev.00/
> The approach seems reasonable to me. It is a bit strange to redirect the 
> platform logging to j.u.logging if something up the stack starts logging 
> but I think we can live with this because these loggers are for 
> debugging purposes.
> 
> The changes to the AWT code are mostly mechanical, so I've only skimmed 
> these (assuming that someone from the AWT team with do a detailed check).
> 
> Have you tested this with a security manager set? I'm just wondering if 
> PlatformLogger's static initializer should do the property lookup in a 
> doPrivileged block. Also in the LoggerProxy for the line separator (BTW: 
> lineSeparator can be final).
> 
> Is isLoggingEnabled() used anywhere? I can't see it and wonder if it 
> should be removed. If it is used, then I assume it needs to be 
> synchronized with redirectPlatformLoggers.
> 
> You allow the PlatformLoggers to be GC'ed but the entries will remain in 
> the loggers map. I don't think this is a big deal because the AWT 
> classes will not be unloaded but might be worth putting a note in a 
> comment to re-visit this some time.
> 
> The static initializer in the forwarding proxy seems a bit messy. It 
> might look neater if changed to something like:
>    private static final Class<?> loggerClass = 
> getClass("java.lang.Logger");                          private static 
> final Method getLoggerMethod = getMethod(loggerClass, "getLogger", 
> String.class);
> where getClass does the Class.forName, returning null if CNF, getMethod 
> returns null if passed a null class, etc.
> 
> Minor nit but there are a few style/formatting issues in PlatformLogger. 
> For example, in JavaLogger there isn't a blank line between methods. It 
> might be worthing taking a pass over this to have it as neat as possible.
> 
> Do you have a bugID to track updating the http protocol handler? Jessie 
> did push some changes to eliminate the static dependency on logging and 
> it would be good if that code used PlatformLogger.
> 
> -Alan.