On 3/23/18 10:51 AM, Kevin Rushforth wrote:
Hi Daniel,
Thanks for the review.
I like the idea of removing the unused levels and methods.
As for directly using System.Logger.Level, we have enough usages of
the Level and convenience logging methods (e.g., "warn", "fine",
etc.), that I think it's better to file a follow-up issue (to minimize
the changes and so it would be more feasible to review the existing
changes). The idea of doing a refactor / rename of the convenience
methods and level names to match seems like a good one for that
follow-up JBS issue.
When you do the clean up, I suggest to drop PlatformLogger completely
and directly use System.Logger (in addition to System.Logger.Level)
Mandy
-- Kevin
Daniel Fuchs wrote:
Hi Ajit,
I have two remarks,
1. I wonder if it's wise to keep the old unused levels like e.g.
Level.CONFIG in your new PlatformLogger.
The sun.util.logging.PlatformLogger had a bridge that allowed
it to transfer these levels unchanged to java.logging when
java.logging was the backend.
Your new PlatformLogger does not have these bridges.
Which means that code that might log with Level.CONFIG
will in reality log with Level.DEBUG, which will be translated
to Level.FINE when java.logging is the backend.
So with the old sun.u.l.PlatformLogger, logging with Level.CONFIG
would have ended up logging with Level.CONFIG in java.logging,
but with your new implementation, this will end up as Level.FINE
(as DEBUG maps to FINE when java.logging is the backend).
AFAICS - there's no code in JavaFX (at least in the files present
in your webrev) that logs with Level.CONFIG.
So why not just remove these unused levels from your implementation
of PlatformLogger? You certainly don't want new code
to start using PlatformLogger::config (as that's actually
an alias to DEBUG).
The best way of avoiding future usage when there's none
today is just to remove these problematic API point.
2. I understand the desire of minimizing the code changes, and
introducing a PlatformLogger class that mimics the old
sun.util.logging.PlatformLogger certainly achieve this goal.
Now the interesting thing is: how many log statements are there
throughout the JavaFX code base?
If there are not too many - then you might want consider
changing them to use directly the levels provided by
System.Logger - and then you might want to use the
"Refactor -> Rename" option of your IDE to simply
rename the methods
PlatformLogger::severe to PlatformLogger::error
PlatformLogger::fine to PlatformLogger::debug
PlatformLogger::finer to PlatformLogger::trace
PlatformLogger::finest to PlatformLogger::trace
Otherwise looks mostly good - I guess.
best regards,
-- daniel
On 23/03/2018 16:34, Ajit Ghaisas wrote:
Hi Kevin, Mandy and Daniel,
Please review the changeset that removes dependency on
sun.util.logging package from JavaFX code.
Bug : https://bugs.openjdk.java.net/browse/JDK-8195799
Fix : http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/
Request you to review.
Regards,
Ajit
