[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-2d-dev
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to FontUtilities.java
From: "Baesken, Matthias" <matthias.baesken () sap ! com>
Date: 2020-07-15 8:14:25
Message-ID: AM0PR02MB54125E2F1DF17C362C067CBF937E0 () AM0PR02MB5412 ! eurprd02 ! prod ! outlook ! com
[Download RAW message or body]
> I found a few lines where spaces around the plus signs are missing. Maybe you can \
> correct this formatting before pushing:
> CFontManager.java L99:, CMap.java L402, SunFontManager.java L565, L628
Here is the updated webrev , will pus it to the client repo .
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.2/
Best regards, Matthias
From: Langer, Christoph <christoph.langer@sap.com>
Sent: Dienstag, 14. Juli 2020 17:44
To: Baesken, Matthias <matthias.baesken@sap.com>; Philip Race \
<philip.race@oracle.com>
Cc: Peter Hull <peterhull90@gmail.com>; Jayathirth D v <JAYATHIRTH.D.V@oracle.com>; \
2d-dev@openjdk.java.net
Subject: RE: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
Hi Matthias,
this looks good to me. I agree that one could look at refactoring regarding Suppliers \
and maybe also improving the isLogging checks in a subsequent change.
I found a few lines where spaces around the plus signs are missing. Maybe you can \
correct this formatting before pushing:
CFontManager.java L99:, CMap.java L402, SunFontManager.java L565, L628
This should probably also be pushed to the client repo.
Thanks
Christoph
From: Baesken, Matthias <matthias.baesken@sap.com<mailto:matthias.baesken@sap.com>>
Sent: Dienstag, 14. Juli 2020 17:22
To: Philip Race <philip.race@oracle.com<mailto:philip.race@oracle.com>>
Cc: Peter Hull <peterhull90@gmail.com<mailto:peterhull90@gmail.com>>; Jayathirth D v \
<JAYATHIRTH.D.V@oracle.com<mailto:JAYATHIRTH.D.V@oracle.com>>; Langer, Christoph \
<christoph.langer@sap.com<mailto:christoph.langer@sap.com>>; \
2d-dev@openjdk.java.net<mailto:2d-dev@openjdk.java.net>
Subject: RE: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
> BTW I suppose you believe you have found all the cases in various packages
> and platform folders and converted them ?
> Or were there some cases you intentionally left alone ?
Hi Phil, I kept a few FontUtilities.getLogger - calls because they are a bit \
special or they use later when logging a second parameter :
grep -nH -r "FontUtilities.getLogger(" *
java.desktop/share/classes/sun/font/SunFontManager.java:1634: PlatformLogger \
logger = FontUtilities.getLogger(); \
java.desktop/share/classes/sun/font/SunFontManager.java:2898: && \
FontUtilities.getLogger().isLoggable(PlatformLogger.Level.INFO)) { \
java.desktop/share/classes/sun/font/TrueTypeFont.java:366: \
FontUtilities.getLogger().severe("While reading " + platName, e); \
java.desktop/share/classes/sun/font/TrueTypeFont.java:386: \
FontUtilities.getLogger().severe("While reading " + platName, e);
New webrev (that keeps the FontUtilities.isLogging() outside the new \
FontUtilities.log* functions ) :
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/
I also adjusted http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html
Following Christophs advice :
> Furthermore, initialization of logging in FontUtilities looks a bit awkward. I \
> think the if (debugFonts) in line 117 is unnecessary and the code of that block \
> could be added to the block before (of line 107: if (debugLevel != null && \
> >!debugLevel.equals("false"))). And you could also remove the following imports \
> > there (line 29ff):
> import java.io.BufferedReader;
> import java.io.File;
> import java.io.FileInputStream;
> import java.io.InputStreamReader;
However enhancing PlatformLogger with Supplier stuff is out of scope of this change \
(maybe another one) .
Best regards, Matthias
From: Philip Race <philip.race@oracle.com<mailto:philip.race@oracle.com>>
Sent: Freitag, 10. Juli 2020 16:45
To: Baesken, Matthias <matthias.baesken@sap.com<mailto:matthias.baesken@sap.com>>
Cc: Peter Hull <peterhull90@gmail.com<mailto:peterhull90@gmail.com>>; Jayathirth D v \
<JAYATHIRTH.D.V@oracle.com<mailto:JAYATHIRTH.D.V@oracle.com>>; Langer, Christoph \
<christoph.langer@sap.com<mailto:christoph.langer@sap.com>>; \
2d-dev@openjdk.java.net<mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
That is a good question. I am not sure any more how we ended up with
the mixed usage but isLogging() seems appropriate to guard the call to log.
So maybe make them consistent for all cases that make sense.
If the end result of that is there is no other usage that calls debugFonts()
that might be something else to take a look if it is basically synonomous.
BTW I suppose you believe you have found all the cases in various packages
and platform folders and converted them ?
Or were there some cases you intentionally left alone ?
Not saying you missed any - just want to know what to expect when I go checking.
-phil.
On 7/10/20, 12:34 AM, Baesken, Matthias wrote:
Hi Phil, okay get your point , thanks for clarification about avoiding the string \
concatenation if FontUtilities.isLogging() returns false . But I guess the coding \
guarded by FontUtilities.debugFonts() for example :
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- if \
(FontUtilities.debugFonts()) { \
src/java.desktop/share/classes/sun/awt/FontConfiguration.java:86: \
FontUtilities.getLogger() \
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-87- \
.info("Creating standard Font Configuration");
would continue using FontUtilities.debugFonts(), correct ?
So this would change to :
src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- if \
(FontUtilities.debugFonts()) { \
src/java.desktop/share/classes/sun/awt/FontConfiguration.java:86: \
FontUtilities.logInfo("Creating standard Font Configuration");
Best regards, Matthias
From: Philip Race <philip.race@oracle.com><mailto:philip.race@oracle.com>
Sent: Donnerstag, 9. Juli 2020 19:17
To: Baesken, Matthias <matthias.baesken@sap.com><mailto:matthias.baesken@sap.com>
Cc: Peter Hull <peterhull90@gmail.com><mailto:peterhull90@gmail.com>; Jayathirth D v \
<JAYATHIRTH.D.V@oracle.com><mailto:JAYATHIRTH.D.V@oracle.com>; Langer, Christoph \
<christoph.langer@sap.com><mailto:christoph.langer@sap.com>; \
2d-dev@openjdk.java.net<mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
There is no harm in repeating isLogging() inside logWarning() but
I don't think it is sufficient.
What we mean is that in some cases the code looks now like this :-
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/ \
sun/font/FileFontStrike.java.udiff.html<http://cr.openjdk.java.net/%7Embaesken/webrevs \
/8248802.0/src/java.desktop/share/classes/sun/font/FileFontStrike.java.udiff.html>
- if (FontUtilities.isLogging()) {
- FontUtilities.getLogger().warning(
- "Failed to render glyph using GDI: code=" + glyphCode
+ FontUtilities.logWarning("Failed to render glyph using GDI: code=" + \
glyphCode
+ ", fontFamily=" + family + ", style=" + style
+ ", size=" + size);
- }
So all that string concatenation always happens, even if we don't log.
The code before probably wasn't 100% consistent but if updating it
then I suggest to make it 100% consistent to always check isLogging()
before calling logWarning().
I suggest to do it even in cases like this
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/ \
sun/font/CMap.java.udiff.html<http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/CMap.java.udiff.html>
+ FontUtilities.logWarning("Cmap UVS subtable overflows buffer.");
where there is no concatenation work happening, just to establish a
consistent pattern.
-phil.
On 7/9/20, 8:07 AM, Baesken, Matthias wrote:
There always should be a call such as FontUtilities.isLogging() test
protecting doing unnecessary work.
Hi, in my patch I always call isLogging() in the new methods in
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/ \
sun/font/FontUtilities.java.frames.html<http://cr.openjdk.java.net/%7Embaesken/webrevs \
/8248802.0/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html>
So are you talking about other places in the coding ?
Best regards, Matthias
-----Original Message-----
From: Philip Race <philip.race@oracle.com><mailto:philip.race@oracle.com>
Sent: Donnerstag, 9. Juli 2020 17:04
To: Peter Hull <peterhull90@gmail.com><mailto:peterhull90@gmail.com>
Cc: Baesken, Matthias <matthias.baesken@sap.com><mailto:matthias.baesken@sap.com>; \
Jayathirth D v <JAYATHIRTH.D.V@oracle.com><mailto:JAYATHIRTH.D.V@oracle.com>; Langer, \
Christoph <christoph.langer@sap.com><mailto:christoph.langer@sap.com>; \
2d-dev@openjdk.java.net<mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
I agree.
There always should be a call such as FontUtilities.isLogging() test
protecting doing unnecessary work.
-phil.
On 7/9/20, 3:24 AM, Peter Hull wrote:
Probably not my place to comment, but, does it matter that it's doing
unnecessary work evaluating the argument to logWarning et al, in the
case where logging is not enabled? It only seems to be string
concatenation and maybe would be optimised out anyway, I don't know.
Peter
On Thu, 9 Jul 2020 at 08:32, Baesken, \
Matthias<matthias.baesken@sap.com><mailto:matthias.baesken@sap.com> wrote:
Thank's for the review !
May I get a second review ?
Best regards, Matthias
From: Jayathirth D v<JAYATHIRTH.D.V@ORACLE.COM><mailto:JAYATHIRTH.D.V@ORACLE.COM>
Sent: Donnerstag, 9. Juli 2020 07:21
To: Baesken, Matthias<matthias.baesken@sap.com><mailto:matthias.baesken@sap.com>
Cc: 2d-dev@openjdk.java.net<mailto:2d-dev@openjdk.java.net>
Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java
Looks good to me.
Thanks,
Jay
On 06-Jul-2020, at 12:43 PM, Baesken, \
Matthias<matthias.baesken@sap.com><mailto:matthias.baesken@sap.com> wrote:
Hello, please review this small change to font related logging .
We have a lot of font logging calls in java.desktop that look similar to this coding \
:
if (FontUtilities.isLogging()) {
FontUtilities.getLogger().info("Here comes my important info");
}
This coding could be simplified by adding static log methods to FontUtilities.java
public static void logWarning(String s);
public static void logInfo(String s);
public static void logSevere(String s);
doing the isLogging check + FontUtilities.getLogger(). ...
Bug/webrev :
https://bugs.openjdk.java.net/browse/JDK-8248802
http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/<http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/>
Thanks, Matthias
[Attachment #3 (text/html)]
<html xmlns:v="urn:schemas-microsoft-com:vml" \
xmlns:o="urn:schemas-microsoft-com:office:office" \
xmlns:w="urn:schemas-microsoft-com:office:word" \
xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" \
xmlns="http://www.w3.org/TR/REC-html40"> <head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Consolas;
panose-1:2 11 6 9 2 2 4 3 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0cm;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";
color:black;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:Consolas;
color:black;}
p.msonormal0, li.msonormal0, div.msonormal0
{mso-style-name:msonormal;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:11.0pt;
font-family:"Calibri",sans-serif;
color:black;}
span.removed
{mso-style-name:removed;}
span.new
{mso-style-name:new;}
span.EmailStyle22
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle23
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle24
{mso-style-type:personal;
font-family:"Calibri",sans-serif;
color:windowtext;}
span.EmailStyle26
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:windowtext;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:70.85pt 70.85pt 2.0cm 70.85pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body bgcolor="white" lang="DE" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" style="color:windowtext">>I found a few lines \
where spaces around the plus signs are missing. Maybe you can correct this formatting \
before pushing:<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">>CFontManager.java L99:, CMap.java L402, \
SunFontManager.java L565, L628<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext;mso-fareast-language:EN-US">Here is the updated webrev , will \
pus it to the client repo .<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><a \
href="http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.2/">http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.2/</a><o:p></o:p></p>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span style="color:windowtext;mso-fareast-language:EN-US">Best \
regards, Matthias<o:p></o:p></span></p> <p class="MsoNormal"><span \
style="color:windowtext;mso-fareast-language:EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span \
style="mso-fareast-language:EN-US"><o:p> </o:p></span></p> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="color:windowtext">From:</span></b><span lang="EN-US" style="color:windowtext"> \
Langer, Christoph <christoph.langer@sap.com> <br>
<b>Sent:</b> Dienstag, 14. Juli 2020 17:44<br>
<b>To:</b> Baesken, Matthias <matthias.baesken@sap.com>; Philip Race \
<philip.race@oracle.com><br> <b>Cc:</b> Peter Hull \
<peterhull90@gmail.com>; Jayathirth D v <JAYATHIRTH.D.V@oracle.com>; \
2d-dev@openjdk.java.net<br> <b>Subject:</b> RE: [OpenJDK 2D-Dev] RFR : 8248802: Add \
log helper methods to FontUtilities.java<o:p></o:p></span></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal"><span lang="EN-US" style="color:windowtext">Hi \
Matthias,<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">this looks good to me. I agree that one could \
look at refactoring regarding Suppliers and maybe also improving the isLogging checks \
in a subsequent change.<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">I found a few lines where spaces around the \
plus signs are missing. Maybe you can correct this formatting before \
pushing:<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">CFontManager.java L99:, CMap.java L402, \
SunFontManager.java L565, L628<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" style="color:windowtext">This should probably \
also be pushed to the client repo.<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">Thanks<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">Christoph<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <div style="border:none;border-left:solid \
blue 1.5pt;padding:0cm 0cm 0cm 4.0pt"> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="color:windowtext">From:</span></b><span lang="EN-US" style="color:windowtext"> \
Baesken, Matthias <<a \
href="mailto:matthias.baesken@sap.com">matthias.baesken@sap.com</a>> <br>
<b>Sent:</b> Dienstag, 14. Juli 2020 17:22<br>
<b>To:</b> Philip Race <<a \
href="mailto:philip.race@oracle.com">philip.race@oracle.com</a>><br> <b>Cc:</b> \
Peter Hull <<a href="mailto:peterhull90@gmail.com">peterhull90@gmail.com</a>>; \
Jayathirth D v <<a \
href="mailto:JAYATHIRTH.D.V@oracle.com">JAYATHIRTH.D.V@oracle.com</a>>; Langer, \
Christoph <<a href="mailto:christoph.langer@sap.com">christoph.langer@sap.com</a>>;
<a href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> RE: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java<o:p></o:p></span></p> </div>
</div>
<p class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p>
<p class="MsoNormal"><span lang="EN-US">>BTW I suppose you believe you have found \
all the cases in various packages<br> >and platform folders and converted them \
?<br> >Or were there some cases you intentionally left alone ?</span><span \
lang="EN-US" style="color:windowtext"><o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">Hi Phil, I kept a few \
FontUtilities.getLogger - calls because they are a bit special or \
they use later when logging a second parameter :<b><o:p></o:p></b></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">grep -nH -r \
"FontUtilities.getLogger(" *<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">java.desktop/share/classes/sun/font/SunFontManager.java:1634: \
PlatformLogger logger = FontUtilities.getLogger();<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">java.desktop/share/classes/sun/font/SunFontManager.java:2898: \
\
&& FontUtilities.getLogger().isLoggable(PlatformLogger.Level.INFO)) \
{<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">java.desktop/share/classes/sun/font/TrueTypeFont.java:366:&nb \
sp; \
FontUtilities.getLogger().severe("While reading " + platName, \
e);<o:p></o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">java.desktop/share/classes/sun/font/TrueTypeFont.java:386:&nb \
sp; \
FontUtilities.getLogger().severe("While reading " + platName, \
e);<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">New webrev (that keeps the FontUtilities.isLogging() \
outside the new FontUtilities.log* functions ) :<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"><o:p> </o:p></span></p> <pre><a \
href="http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/"><span \
lang="EN-US">http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/</span></a><o:p></o:p></pre>
<pre><o:p> </o:p></pre>
<pre><span lang="EN-US"><o:p> </o:p></span></pre>
<pre><span lang="EN-US">I also adjusted </span><a \
href="http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html"><span \
lang="EN-US">http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.1/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html</span></a><o:p></o:p></pre>
<pre><span lang="EN-US">Following Christophs advice :<o:p></o:p></span></pre>
<pre><span lang="EN-US"><o:p> </o:p></span></pre>
<p class="MsoNormal"><span lang="EN-US">>Furthermore, initialization of logging in \
FontUtilities looks a bit awkward. I think the if (debugFonts) in line 117 is \
unnecessary and the code of that block could be added to the block before (of line \
107: if (debugLevel
!= null && >!debugLevel.equals("false"))). And you could also \
remove the following imports there (line 29ff):<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US">>import \
java.io.BufferedReader;<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US">>import java.io.File;<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US">>import \
java.io.FileInputStream;<o:p></o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US">>import java.io.InputStreamReader;<o:p></o:p></span></p> <pre><span \
lang="EN-US"><o:p> </o:p></span></pre> <p class="MsoNormal"><span \
lang="EN-US"><o:p> </o:p></span></p> <p class="MsoNormal"><span \
lang="EN-US">However enhancing PlatformLogger with Supplier stuff is out of \
scope of this change (maybe another one) .<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US">Best regards, Matthias<o:p></o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p> <p \
class="MsoNormal"><span lang="EN-US"><o:p> </o:p></span></p> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="color:windowtext">From:</span></b><span lang="EN-US" style="color:windowtext"> \
Philip Race <<a href="mailto:philip.race@oracle.com">philip.race@oracle.com</a>>
<br>
<b>Sent:</b> Freitag, 10. Juli 2020 16:45<br>
<b>To:</b> Baesken, Matthias <<a \
href="mailto:matthias.baesken@sap.com">matthias.baesken@sap.com</a>><br> \
<b>Cc:</b> Peter Hull <<a \
href="mailto:peterhull90@gmail.com">peterhull90@gmail.com</a>>; Jayathirth D v \
<<a href="mailto:JAYATHIRTH.D.V@oracle.com">JAYATHIRTH.D.V@oracle.com</a>>; \
Langer, Christoph <<a \
href="mailto:christoph.langer@sap.com">christoph.langer@sap.com</a>>; <a \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><br> <b>Subject:</b> \
Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java<o:p></o:p></span></p> </div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">That is a good question. I am not sure any more how we ended up \
with<br> the mixed usage but isLogging() seems appropriate to guard the call to \
log.<br> <br>
So maybe make them consistent for all cases that make sense.<br>
<br>
If the end result of that is there is no other usage that calls debugFonts()<br>
that might be something else to take a look if it is basically synonomous.<br>
<br>
BTW I suppose you believe you have found all the cases in various packages<br>
and platform folders and converted them ?<br>
Or were there some cases you intentionally left alone ?<br>
<br>
Not saying you missed any - just want to know what to expect when I go checking.<br>
<br>
-phil.<br>
<br>
On 7/10/20, 12:34 AM, Baesken, Matthias wrote: <o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<p class="MsoNormal"><span lang="EN-US" style="color:windowtext">Hi Phil, okay \
get your point , thanks for clarification about avoiding the string \
concatenation if FontUtilities.isLogging() returns false \
.</span><o:p></o:p></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext">But I guess the coding guarded by \
FontUtilities.debugFonts() for example :</span><o:p></o:p></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"> </span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- \
if (FontUtilities.debugFonts()) {</span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">src/java.desktop/share/classes/sun/awt/FontConfi \
guration.java:86: \
FontUtilities.getLogger()</span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">src/java.desktop/share/classes/sun/awt/FontConfi \
guration.java-87- \
.info("Creating standard Font Configuration");</span><o:p></o:p></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"> </span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">would continue using \
FontUtilities.debugFonts(), correct ?</span><o:p></o:p></p> <p \
class="MsoNormal"><span lang="EN-US" style="color:windowtext">So this would change to \
:</span><o:p></o:p></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"> </span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">src/java.desktop/share/classes/sun/awt/FontConfiguration.java-85- \
if (FontUtilities.debugFonts()) {</span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">src/java.desktop/share/classes/sun/awt/FontConfi \
guration.java:86: \
FontUtilities.logInfo("Creating standard Font \
Configuration");</span><o:p></o:p></p> <p class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"> </span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US" style="color:windowtext">Best regards, Matthias</span><o:p></o:p></p> <p \
class="MsoNormal"><span lang="EN-US" \
style="color:windowtext"> </span><o:p></o:p></p> <p class="MsoNormal"><span \
lang="EN-US"> </span><o:p></o:p></p> <div>
<div style="border:none;border-top:solid #E1E1E1 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" \
style="color:windowtext">From:</span></b><span lang="EN-US" style="color:windowtext"> \
Philip Race <a href="mailto:philip.race@oracle.com"><philip.race@oracle.com></a> \
<br> <b>Sent:</b> Donnerstag, 9. Juli 2020 19:17<br>
<b>To:</b> Baesken, Matthias <a \
href="mailto:matthias.baesken@sap.com"><matthias.baesken@sap.com></a><br> \
<b>Cc:</b> Peter Hull <a \
href="mailto:peterhull90@gmail.com"><peterhull90@gmail.com></a>; Jayathirth D v \
<a href="mailto:JAYATHIRTH.D.V@oracle.com"><JAYATHIRTH.D.V@oracle.com></a>; \
Langer, Christoph <a \
href="mailto:christoph.langer@sap.com"><christoph.langer@sap.com></a>; <a \
href="mailto:2d-dev@openjdk.java.net"> 2d-dev@openjdk.java.net</a><br>
<b>Subject:</b> Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java</span><o:p></o:p></p> </div>
</div>
<p class="MsoNormal"> <o:p></o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt"><br>
There is no harm in repeating isLogging() inside logWarning() but<br>
I don't think it is sufficient.<br>
<br>
What we mean is that in some cases the code looks now like this :-<br>
<br>
<a href="http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/src/java.desktop/sha \
re/classes/sun/font/FileFontStrike.java.udiff.html">http://cr.openjdk.java.net/~mbaesk \
en/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/FileFontStrike.java.udiff.html</a><o:p></o:p></p>
<pre style="background:#EEEEEE"><span class="removed"><span \
style="color:brown">- \
if (FontUtilities.isLogging()) {</span></span><o:p></o:p></pre> <pre \
style="background:#EEEEEE"><span class="removed"><span \
style="color:brown">- \
FontUtilities.getLogger().warning(</span></span><o:p></o:p></pre> <pre \
style="background:#EEEEEE"><span class="removed"><span \
style="color:brown">- \
"Failed to render glyph using GDI: code=" + \
glyphCode</span></span><o:p></o:p></pre> <pre style="background:#EEEEEE"><span \
class="new"><span style="color:blue">+ \
FontUtilities.logWarning("Failed to render glyph using GDI: code=" + \
glyphCode</span></span><o:p></o:p></pre> <pre \
style="background:#EEEEEE">   \
; \
+ ", fontFamily=" + family + ", style=" + \
style<o:p></o:p></pre> <pre \
style="background:#EEEEEE">   \
; \
+ ", size=" + size);<o:p></o:p></pre> <pre \
style="background:#EEEEEE"><span class="removed"><span \
style="color:brown">- \
}</span></span><o:p></o:p></pre> <p class="MsoNormal" \
style="margin-bottom:12.0pt"><br> So all that string concatenation always happens, \
even if we don't log.<br> <br>
The code before probably wasn't 100% consistent but if updating it<br>
then I suggest to make it 100% consistent to always check isLogging()<br>
before calling logWarning().<br>
<br>
I suggest to do it even in cases like this<br>
<a href="http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/src/java.desktop/sha \
re/classes/sun/font/CMap.java.udiff.html">http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/CMap.java.udiff.html</a><br>
<br>
<o:p></o:p></p>
<pre style="background:#EEEEEE"><span class="new"><span \
style="color:blue">+ \
FontUtilities.logWarning("Cmap UVS subtable overflows \
buffer.");</span></span><o:p></o:p></pre> <p class="MsoNormal"><br>
where there is no concatenation work happening, just to establish a<br>
consistent pattern.<br>
<br>
<br>
-phil.<br>
<br>
On 7/9/20, 8:07 AM, Baesken, Matthias wrote: <o:p></o:p></p>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre> <o:p></o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>There always should be a call such as FontUtilities.isLogging() \
test<o:p></o:p></pre> <pre>protecting doing unnecessary work.<o:p></o:p></pre>
</blockquote>
<pre> <o:p></o:p></pre>
<pre>Hi, in my patch I always call isLogging() in the new \
methods in <o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre><a href="http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/src/java.deskto \
p/share/classes/sun/font/FontUtilities.java.frames.html">http://cr.openjdk.java.net/~m \
baesken/webrevs/8248802.0/src/java.desktop/share/classes/sun/font/FontUtilities.java.frames.html</a><o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>So are you talking about other places in the coding ?<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Best regards, Matthias<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>-----Original Message-----<o:p></o:p></pre>
<pre>From: Philip Race <a \
href="mailto:philip.race@oracle.com"><philip.race@oracle.com></a> \
<o:p></o:p></pre> <pre>Sent: Donnerstag, 9. Juli 2020 17:04<o:p></o:p></pre>
<pre>To: Peter Hull <a \
href="mailto:peterhull90@gmail.com"><peterhull90@gmail.com></a><o:p></o:p></pre>
<pre>Cc: Baesken, Matthias <a \
href="mailto:matthias.baesken@sap.com"><matthias.baesken@sap.com></a>; \
Jayathirth D v <a href="mailto:JAYATHIRTH.D.V@oracle.com"><JAYATHIRTH.D.V@oracle.com></a>; \
Langer, Christoph <a \
href="mailto:christoph.langer@sap.com"><christoph.langer@sap.com></a>; <a \
href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><o:p></o:p></pre> \
<pre>Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre>I agree.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>There always should be a call such as FontUtilities.isLogging() \
test<o:p></o:p></pre> <pre>protecting doing unnecessary work.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>-phil.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>On 7/9/20, 3:24 AM, Peter Hull wrote:<o:p></o:p></pre>
<blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Probably not my place to comment, but, does it matter that it's \
doing<o:p></o:p></pre> <pre>unnecessary work evaluating the argument to logWarning et \
al, in the<o:p></o:p></pre> <pre>case where logging is not enabled? It only seems to \
be string<o:p></o:p></pre> <pre>concatenation and maybe would be optimised out \
anyway, I don't know.<o:p></o:p></pre> <pre>Peter<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>On Thu, 9 Jul 2020 at 08:32, Baesken, Matthias<a \
href="mailto:matthias.baesken@sap.com"><matthias.baesken@sap.com></a> \
wrote:<o:p></o:p></pre> <blockquote style="margin-top:5.0pt;margin-bottom:5.0pt">
<pre>Thank's for the review !<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>May I get a second review ?<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Best regards, Matthias<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>From: Jayathirth D v<a \
href="mailto:JAYATHIRTH.D.V@ORACLE.COM"><JAYATHIRTH.D.V@ORACLE.COM></a><o:p></o:p></pre>
<pre>Sent: Donnerstag, 9. Juli 2020 07:21<o:p></o:p></pre>
<pre>To: Baesken, Matthias<a \
href="mailto:matthias.baesken@sap.com"><matthias.baesken@sap.com></a><o:p></o:p></pre>
<pre>Cc: <a href="mailto:2d-dev@openjdk.java.net">2d-dev@openjdk.java.net</a><o:p></o:p></pre>
<pre>Subject: Re: [OpenJDK 2D-Dev] RFR : 8248802: Add log helper methods to \
FontUtilities.java<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Looks good to me.<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Thanks,<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Jay<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>On 06-Jul-2020, at 12:43 PM, Baesken, Matthias<a \
href="mailto:matthias.baesken@sap.com"><matthias.baesken@sap.com></a> \
wrote:<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Hello, please review this small change to font related logging \
.<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>We have a lot of font logging calls in java.desktop that look similar to this \
coding :<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> if (FontUtilities.isLogging()) \
{<o:p></o:p></pre> <pre> \
FontUtilities.getLogger().info("Here comes my important \
info");<o:p></o:p></pre> <pre> \
}<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre>This coding could be simplified by adding static log methods to \
FontUtilities.java<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>public static void logWarning(String s);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>public static void logInfo(String s);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>public static void logSevere(String s);<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> doing the isLogging check + FontUtilities.getLogger(). \
...<o:p></o:p></pre> <pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Bug/webrev :<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre><a href="https://bugs.openjdk.java.net/browse/JDK-8248802">https://bugs.openjdk.java.net/browse/JDK-8248802</a><o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre><a href="http://cr.openjdk.java.net/%7Embaesken/webrevs/8248802.0/">http://cr.openjdk.java.net/~mbaesken/webrevs/8248802.0/</a><o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre>Thanks, Matthias<o:p></o:p></pre>
<pre> <o:p></o:p></pre>
<pre> <o:p></o:p></pre>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</div>
</div>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic