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

2018-03-27 Thread mandy chung 
It can be addressed later while I think this is a simple change to the 
getInstance method.  I guess you prefer to keep PlatformLogger as close 
to the original version and that's fine.


Mandy

On 3/27/18 4:46 AM, Kevin Rushforth wrote:
It doesn't seem harmful to keep the current implementation. This seems 
better to leave for the follow-on JBS issue (JDK-8200236) unless there 
something I am missing.


-- Kevin


mandy chung wrote:

You don't need the loggers map and getLogger method can simply return
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy


On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in 
-http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs;openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
  public static void setUpClass() {
  >>>System.err.println("SelectBindingTest : log messages are
expected from these tests.");


All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


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

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

2018-03-27 Thread Kevin Rushforth 
Yes it could be converted, but wasn't necessary to remove the depenency, 
so I filed a separate JBS issue to track this replacing the calls to 
j.u.l: JDK-8195974 <https://bugs.openjdk.java.net/browse/JDK-8195974>.


-- Kevin


Tom Schindl wrote:

Minor question on this: I see the test stuff is using java.util.Logging
could this not be ported to PlatformLogger?

Tom

On 26.03.18 22:46, Kevin Rushforth wrote:
  

This looks fine to me now.

-- Kevin


Ajit Ghaisas wrote:


Thanks all for the review.

I have addressed the review comments in -
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java

3. Removed unused methods from com.sun.javafx.logging.PlatformLogger
class.

Also, I have created a new bug JDK-8200236 to address some of the
valid suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger
instead of platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be
changed to spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java

 public static void setUpClass() {
 >>>System.err.println("SelectBindingTest : log messages are
expected from these tests.");


All my testing looks good. With this patch I am now able to run
applications with OpenJDK 10 + a standalone FX SDK with no qualified
exports on the command line (as long as it doesn't use Swing interop).

-- Kevin


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

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

2018-03-27 Thread Tom Schindl 
Minor question on this: I see the test stuff is using java.util.Logging
could this not be ported to PlatformLogger?

Tom

On 26.03.18 22:46, Kevin Rushforth wrote:
> This looks fine to me now.
> 
> -- Kevin
> 
> 
> Ajit Ghaisas wrote:
>> Thanks all for the review.
>>
>> I have addressed the review comments in -
>> http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/
>>
>> The changes are -
>> 1. Addressed the (c) year inaccuracies in files -
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
>>
>> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>>
>> 2. Removed tabs from
>> modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
>>
>> 3. Removed unused methods from com.sun.javafx.logging.PlatformLogger
>> class.
>>
>> Also, I have created a new bug JDK-8200236 to address some of the
>> valid suggestions from Mandy and Daniel.
>>
>> Request you to review the new webrev.
>>
>> Regards,
>> Ajit
>>
>> -Original Message-
>> From: Kevin Rushforth Sent: Saturday, March 24, 2018 3:27 AM
>> To: Ajit Ghaisas
>> Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
>> Subject: Re: [11] Review request : JDK-8195799 : Use System logger
>> instead of platform logger in javafx modules
>>
>> The only additional comments I have are couple typos and a white-space
>> issue:
>>
>> 1. There is a typo in the Copyright year (201 rather than 2018) in the
>> following two files:
>>
>> modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
>>
>> modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>>
>>
>> 2. There are tab characters in the following file that need to be
>> changed to spaces:
>>
>> modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
>>
>>  public static void setUpClass() {
>>  >>>    System.err.println("SelectBindingTest : log messages are
>> expected from these tests.");
>>
>>
>> All my testing looks good. With this patch I am now able to run
>> applications with OpenJDK 10 + a standalone FX SDK with no qualified
>> exports on the command line (as long as it doesn't use Swing interop).
>>
>> -- Kevin
>>
>>
>> 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
>>>      

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

2018-03-27 Thread Ajit Ghaisas 
Thanks for the review.

As suggested, I have added Mandy’s point about loggers map to be addressed as 
part of JDK-8200236.

 

Regards,

Ajit

 

From: Kevin Rushforth 
Sent: Tuesday, March 27, 2018 2:16 AM
To: mandy chung
Cc: Ajit Ghaisas; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

 

It doesn't seem harmful to keep the current implementation. This seems better 
to leave for the follow-on JBS issue (JDK-8200236) unless there something I am 
missing.

-- Kevin


mandy chung wrote: 

You don't need the loggers map and getLogger method can simply return 
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy



On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.
 
I have addressed the review comments in - HYPERLINK 
"http://cr.openjdk.java.net/%7Eaghaisas/fx/8195799/webrev.1/"http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/
 
The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
 
2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.
 
Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.
 
Request you to review the new webrev.
 
Regards,
Ajit
 
-Original Message-
From: Kevin Rushforth 
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; HYPERLINK 
"mailto:openjfx-dev@openjdk.java.net"openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules
 
The only additional comments I have are couple typos and a white-space
issue:
 
1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:
 
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
 
 
2. There are tab characters in the following file that need to be changed to 
spaces:
 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
 public static void setUpClass() {
 >>>    System.err.println("SelectBindingTest : log messages are 
expected from these tests.");
 
 
All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).
 
-- Kevin
 
 
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 :  HYPERLINK 
"http://cr.openjdk.java.net/%7Eaghaisas/fx/8195799/webrev.0/"http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.0/
 
    Request you to review.
 
Regards,
Ajit
  
  

 

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

2018-03-26 Thread Kevin Rushforth 

This looks fine to me now.

-- Kevin


Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in - 
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth 
Sent: Saturday, March 24, 2018 3:27 AM

To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
 public static void setUpClass() {
 >>>System.err.println("SelectBindingTest : log messages are 
expected from these tests.");



All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


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

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

2018-03-26 Thread Kevin Rushforth 
It doesn't seem harmful to keep the current implementation. This seems 
better to leave for the follow-on JBS issue (JDK-8200236) unless there 
something I am missing.


-- Kevin


mandy chung wrote:

You don't need the loggers map and getLogger method can simply return
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy


On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in - 
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth 
Sent: Saturday, March 24, 2018 3:27 AM

To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
 public static void setUpClass() {
 >>>System.err.println("SelectBindingTest : log messages are 
expected from these tests.");



All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


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

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

2018-03-26 Thread mandy chung 

You don't need the loggers map and getLogger method can simply return
 return new PlatformLogger(System.getLogger(name));

Other than this, looks fine.

Mandy


On 3/26/18 4:36 AM, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in - 
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
  public static void setUpClass() {
  >>>System.err.println("SelectBindingTest : log messages are
expected from these tests.");


All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


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

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

2018-03-26 Thread Daniel Fuchs 

Hi Ajit,

Looks good to me. I obviously didn't review the
build changes.

best regards,

-- daniel

On 26/03/2018 12:36, Ajit Ghaisas wrote:

Thanks all for the review.

I have addressed the review comments in 
-http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

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

2018-03-26 Thread Ajit Ghaisas 
Thanks all for the review.

I have addressed the review comments in - 
http://cr.openjdk.java.net/~aghaisas/fx/8195799/webrev.1/

The changes are -
1. Addressed the (c) year inaccuracies in files -
modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java

2. Removed tabs from 
modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
3. Removed unused methods from com.sun.javafx.logging.PlatformLogger class.

Also, I have created a new bug JDK-8200236 to address some of the valid 
suggestions from Mandy and Daniel.

Request you to review the new webrev.

Regards,
Ajit

-Original Message-
From: Kevin Rushforth 
Sent: Saturday, March 24, 2018 3:27 AM
To: Ajit Ghaisas
Cc: Mandy Chung; Daniel Fuchs; openjfx-dev@openjdk.java.net
Subject: Re: [11] Review request : JDK-8195799 : Use System logger instead of 
platform logger in javafx modules

The only additional comments I have are couple typos and a white-space
issue:

1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:

modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be changed to 
spaces:

modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
 public static void setUpClass() {
 >>>System.err.println("SelectBindingTest : log messages are 
expected from these tests.");


All my testing looks good. With this patch I am now able to run applications 
with OpenJDK 10 + a standalone FX SDK with no qualified exports on the command 
line (as long as it doesn't use Swing interop).

-- Kevin


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
>

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

2018-03-23 Thread Kevin Rushforth 
The only additional comments I have are couple typos and a white-space 
issue:


1. There is a typo in the Copyright year (201 rather than 2018) in the 
following two files:


modules/javafx.base/src/main/java/com/sun/javafx/collections/SetListenerHelper.java
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java


2. There are tab characters in the following file that need to be 
changed to spaces:


modules/javafx.base/src/test/java/test/com/sun/javafx/binding/SelectBindingTest.java
public static void setUpClass() {
>>>System.err.println("SelectBindingTest : log messages are 
expected from these tests.");



All my testing looks good. With this patch I am now able to run 
applications with OpenJDK 10 + a standalone FX SDK with no qualified 
exports on the command line (as long as it doesn't use Swing interop).


-- Kevin


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

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

2018-03-23 Thread Kevin Rushforth 

inline

mandy chung wrote:



On 3/23/18 9:34 AM, 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/




buildSrc/addExports
   FX modules are compiled together and I don't expect these 
--add-exports are needed.  I suspect it's because of the boot JDK and 
this is a temporary dance?


Exactly. It is needed for changes that have been added to 
module-info.java and are not in the minimum required boot JDK, so that 
we can build/run apps and tests.



PlatformLogger.java
 150 public static synchronized PlatformLogger getLogger(String 
name) {


This keeps the weak reference to all PlatformLogger created.  A 
simplification is to return

   new PlatformLogger(System.getLogger(name));

  
System::getLogger should return the same instance if it has been 
created.  I also suspect the caller of PlatformLogger::getLogger keeps 
a strong reference and calls it once.  

I recalled there were some native methods calling the Java API to set 
level.  It has been a while back since I looked at it and things 
miight have changed.  Is there such reference any more?


The ones that you were thinking of are in javafx.media are not calls to 
PlatformLogger at all, but calls to a similarly named convenience class 
in the javafx.media module.



Other than the above comments, this change looks good.


Thanks.

-- Kevin



Mandy

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

2018-03-23 Thread 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

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

2018-03-23 Thread 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

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

2018-03-23 Thread Daniel Fuchs 

Hi Kevin,

This sounds reasonable to me.

best regards,

-- daniel

On 23/03/2018 17:51, 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.


-- Kevin

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

2018-03-23 Thread Kevin Rushforth 

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.


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

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

2018-03-23 Thread Daniel Fuchs 

Hi Mandy,

On 23/03/2018 17:00, mandy chung wrote:
System::getLogger should return the same instance if it has been 
created.  


Not necessarily. j.u.l does that, but System::getLogger may return a
new cheap wrapper. As you noted, if JavaFX sources only creates a
handful of loggers, the cost of maintaining a Map and
dealing with the complexity of weak reference may not be justified.

Best regards,

-- daniel

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

2018-03-23 Thread Daniel Fuchs 

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

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

2018-03-23 Thread mandy chung 



On 3/23/18 9:34 AM, 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/



buildSrc/addExports
   FX modules are compiled together and I don't expect these 
--add-exports are needed.  I suspect it's because of the boot JDK and 
this is a temporary dance?



PlatformLogger.java
 150 public static synchronized PlatformLogger getLogger(String name) {

This keeps the weak reference to all PlatformLogger created.  A 
simplification is to return


   new PlatformLogger(System.getLogger(name));

System::getLogger should return the same instance if it has been 
created.  I also suspect the caller of PlatformLogger::getLogger keeps a 
strong reference and calls it once.


I recalled there were some native methods calling the Java API to set 
level.  It has been a while back since I looked at it and things miight 
have changed.  Is there such reference any more?


Other than the above comments, this change looks good.

Mandy

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

2018-03-23 Thread Ajit Ghaisas 
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