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

[Kevin Rushforth]

mandy chung wrote:
>
>
> 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)

If not too hard, that might be a good way to go.

-- Kevin

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