JDK-8116056 : Upgrade to use PlatformLogger.Level instead of primitive int type
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2013-04-05
  • Updated: 2015-06-17
  • Resolved: 2013-06-13
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 8
8Fixed
Related Reports
Blocks :  
Blocks :  
Blocks :  
Description
sun.util.logging.PlatformLogger has been improved in JDK 8 for better performance and also PlatformLogger.Level enum class was defined for better type checking.

There are several places in JFX binding the level type to int that prohibits the API change that
is source compatibility with jdk once it's rebuilt.   JFX can migrate to use the new PlatformLogger.Level
enum class so that we can remove the deprecated methods:
    http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8011638

For example, in glass/glass/src/com/sun/glass/ui/accessible/AccessibleLogger.java:
        // simplified (original code wrapped this with doPrivileged)
        String levelString = System.getProperty("log.lens");  
        int level = PlatformLogger.SEVERE;
        if (levelString != null) {
            try {
                level = Integer.parseInt(levelString);
            } catch (NumberFormatException nfe) {
                try {
                    level = PlatformLogger.class
                            .getField(levelString.toUpperCase())
                            .getInt(null);
                } catch (Exception e) { }
            }
        }
        logger.setLevel(level);

can be simply changed to:

        String levelString = System.getProperty("log.lens", "SEVERE").toUpperCase();
        logger.setLevel(PlatformLogger.Level.valueOf(levelString); 

glass/src/com/sun/glass/ui/lens/LensLogger.java - has similar code as above and also has native code referencing these constants (glass/glass-lib-lens/src/LensLogger.c)

Due to the JFX and JDK are not built together, it require many more source files be changed.  Suggest to add:
    import static sun.util.logging.PlatformLogger.Level.*;

in the files referencing PlatformLogger.SEVERE and other constants; then change PlatformLogger.XXX to XXX.

PlatformLogger.getLevel() to PlatformLogger.level() that will return PlatformLogger.Level object.
Comments
Reviewed and tested. +1.
13-06-2013

Hi Daniel, thanks for the hint When I saw the m_ and c_ in LensLogger.c I thought you used Hungarian notation in your native code. That is why I thought glass_log_level, as global var, should be prefixed with g_ Now is clear to me m_ if for method identifier and c_ for class.
12-06-2013

You can run on the Linux desktop by building with -Dcross.platform=x86 and running on Ubuntu 12.04 that has these packages installed: libgl1-mesa-dev, libegl1-mesa-dev, libgles2-mesa-dev, libgles2-mesa ..but no need for that. I can test your changes.
12-06-2013

I didn't noticed glass_log_level was declared in LensCommon.h (maybe prefix with g_ ?). Anyway, I agree intValue() is the simplest change. If you like I can write code, but not sure I know how to test this. I don't have the hardware for it and I don't know if it is possible to run Lens on the desktop.
12-06-2013

It looks to me like the simplest change is to set glass_log_level in glass_logger_init() based on the result of intValue() on the Level object
12-06-2013

This change would cause many compilation errors. Did you have a corresponding change to LensCommon.h?
12-06-2013

Hi Mandy, if you would like to send my a rt.jar I can try building JFX with it.
10-06-2013

Danno, here is the patch.
10-06-2013

Where's the link for the webrev? i'm not seeing it on the atom feed,of course I may be looking for the wrong ID.
10-06-2013

The webrev looks fine. Would it be useful if I build you a JDK8 with the removal of the PlatformLogger.XXX static fields and the deprecated methods? So that you can run it with some FX tests to catch if this patch misses anything?
10-06-2013

Hi Mandy, See the implementation LensLogger. public boolean isLogging(level) { return level >= logLevel; } PlatformLogger public boolean isLoggable(int level) { if (level < levelValue || levelValue == OFF) { return false; } return true; } I say we should remove isLogging and use isLoggable instead. As for AccessibleLogger#isLogging(), it had no references, so I just delete it. The messy part is LensLogger.c
07-06-2013

Felipe - thanks for fixing this. The diff in com/sun/glass/ui/accessible/AccessibleLogger.java shows that the use of the PlatformLogger.Level enums is a good cleanup. I recommend to move away from int level and use PlatformLogger.Level instead. In other words, it makes sense to replace the isLogging(int level) method with isLogging(Level level). The question I would ask is what the LensLogger.isLogging method is for? LensLogger.isLogging(level) can be replaced by PlatformLogger.isLoggable(level). I suspect there is something in the native code that access directly the logger and level static fields in LensLogger.
06-06-2013

Alternatively we could remove isLogging() from AccessibleLogger and LensLogger and use logger.isLoggable(Level) as most places do.
06-06-2013

and about all references to, for example, LensLogger.isLogging(PlatformLogger.FINE) ? Do we need to change them all to: LensLogger.isLogging(PlatformLogger.FINE) ? I have fixed the current implementation of isLogging to: public static boolean isLogging(int level) { return level >= logLevel.intValue(); } Do I need to replace it with: public static boolean isLogging(Level level) { return level.compareTo(logLevel) >= 0; } (and fix all callers?)
06-06-2013

Note the code change is not quite the same thing. Try setting -Dlog.lens="BADNAME"; before running String levelString = System.getProperty("log.lens", "SEVERE").toUpperCase(); logger.setLevel(PlatformLogger.Level.valueOf(levelString); it will walkback: Caused by: java.lang.IllegalArgumentException: No enum constant sun.util.logging.PlatformLogger.Level. at java.lang.Enum.valueOf(Enum.java:238) at sun.util.logging.PlatformLogger$Level.valueOf(PlatformLogger.java:108) Note the old code would handle "BADNAME" nicely and default to SEVERE.
06-06-2013

The earliest would be b94, if the fix is pushed to graphics before Tuesday.
06-06-2013

Will this be landing in b93 or b94? I usually work off of the master pushes.
06-06-2013

Added Danno to the watch list, as this will impact the community JDK 7 backport effort.
05-06-2013

Note that we need to wait until we switch our Hudson builds to JDK 8-b86 or later.
26-04-2013

I had a discussion with Mandy about this last week, and yes, it does depend on FX switching to JDK 8 for building. Here is what needs to be done: 1) JDK has deprecated the existing logger methods and added new methods (see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8011380); these changes will be in b86. 2) FX will switch Hudson builds to JDK 8 -- initially b81 or b84, but eventually, upgrading to b86 or later. 3) Once FX is using JDK b86 or later to build, we need to implement the fixes described in this bug. 4) After the FX build with the fix for this bug is promoted, the old, deprecated methods can be removed from the JDK (see http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8011638).
08-04-2013

This issue cannot be fixed before we switch to JDK 8 builds. By searching through the code, it looks like only glass have issues with assigning PlatformLogger levels to int, all other code passes this to the PlatformLogger methods, which won't break the code after the switch. Outside of glass code, I found 2 tests that turn off / restrict logging and set it back after the test, saving the level to an int: javafx-ui-controls/test/javafx/scene/control/ControlTest.java javafx-beans/test/com/sun/javafx/binding/SelectBindingTest.java Removing or changing this shouldn't affect the test (just the output). Assigning to Kevin, as this spans multiple components.
08-04-2013