[11] Review request : JDK-8195799 : Use System logger instead of platform logger in javafx modules

[mandy chung]

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