looks good to me.
Regards
Prasanta
On 12-Sep-18 7:57 PM, Dmitry Markov wrote:
Hi Prasanta,
I have updated the fix based on your recommendations. The new webrev
is located at
http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.03/
<http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.03/>
Thanks,
Dmitry
On 12 Sep 2018, at 07:09, Prasanta Sadhukhan
<[email protected]
<mailto:[email protected]>> wrote:
I guess we could check for
if (((typo_flags & 0x80000000) != 0) && isAAT(font))
before and could save on calling
font.getLayoutTableCache()
In present condition, it overwrites layoutTables to 0 making calling
getLayoutTableCache meaningless. Maybe , do something like
long layoutTables = 0;
if !(((typo_flags & 0x80000000) != 0) && isAAT(font)) { layoutTables =
font.getLayoutTableCache();
}
BTW, please correct the indentation of l156 and l160.
Regards
Prasanta
On 11-Sep-18 10:27 PM, Phil Race wrote:
fine by me.
-phil.
On 9/10/2018 1:18 AM, Dmitry Markov wrote:
Phil, Prasanta,
Any thoughts/concerns regarding the latest webrev:
http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.02/
<http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.02/>
Thanks,
Dmitry
On 5 Sep 2018, at 12:27, Dmitry Markov <[email protected]
<mailto:[email protected]>> wrote:
Hi Phil,
Thank you for the comments. You are right the invocation of
isAAT() method might be too expensive especially on OSX. I have
changed the if-statement as you suggested. Please find the new
webrev here:
http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.02/
<http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.02/>
Also I will be happy to port the fix for JDK-8210384 to 8u once it
is ready.
Thanks,
Dmitry
On 4 Sep 2018, at 20:34, Phil Race <[email protected]
<mailto:[email protected]>> wrote:
I see you noticed the problem with it not necessarily being an
AAT font.
It seems this method is a copy/paste of the method added in JDK 9.
So this will work but looking at it, I think in JDK 9 I should
have made this more efficient.
Probably it was a detail in the larger work, that I never came
back to.
On MacOS where it calls through the PhysicalFont interface, since
CFont is not a TrueTypeFont,
it will be retrieving all the data in the table from native to
Java on every call to layout. That will be expensive.
You can mitigate this in your case by reversing the order here
+ if (isAAT(font) && (typo_flags & 0x80000000) != 0) {
to
+ if (((typo_flags & 0x80000000) != 0) && isAAT(font)) {
So it will only take the hit for RTL text which will save a lot
of cases ...
I have filed https://bugs.openjdk.java.net/browse/JDK-8210384
which I will fix for 12.
Up to you whether you also want to wait for that fix, or just
update as I have suggested
and maybe take a follow-on fix later.
Note that whilst your fix will at least make sure text reads in
the correct direction, it
means it will be using default built-in shaping rules of ICU and
so may miss at least
some features provided by the font.
-phil.
On 09/03/2018 05:54 AM, Dmitry Markov wrote:
Hi Prasanta,
Thank you for the feedback. I missed the usage of OSX specific
class in shared code for some reasons. Please find the updated
webrev here:
http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.01/
<http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.01/>
I replaced the usage of CFont with a new private method which
tests either a font is AAT or not.
The issue is jdk8u (ICU) specific. It does not take place on
jdk12 because starting from jdk9 we use Harfbuzz as default
layout engine.
Thanks,
Dmitry
On 3 Sep 2018, at 11:33, Prasanta Sadhukhan
<[email protected]
<mailto:[email protected]>> wrote:
Hi Dmitry,
Not going into technicalities of the fix, but it seems it will
break non-macos build as you are checking for CFont in a shared
class?
Also, if it's the issue still exists, then why you are fixing
only in 8u and not in jdk12?
Regards
Prasanta
On 9/3/2018 3:20 PM, Dmitry Markov wrote:
Hello,
Could you review a fix for jdk8u, please?
bug: https://bugs.openjdk.java.net/browse/JDK-8201801
webrev:
http://cr.openjdk.java.net/~dmarkov/8201801/jdk8u/webrev.00/
<http://cr.openjdk.java.net/%7Edmarkov/8201801/jdk8u/webrev.00/>
Problem description:
The fix for 7162125 [1] enabled font tables processing on OSX.
However there is a lack of support for RTL languages in ICU
layout engine. Quote from ICU guide: “…The AAT processing in
the LayoutEngine is relatively basic as it only applies the
default features in left-to-right text. This processing has
been tested for Devanagari text. Since AAT processing is not
script-specific, it might not work for other scripts…”, more
details at http://userguide.icu-project.org/layoutengine
As a result all RTL languages on OSX are incorrectly laid out,
(i.e. the letters are reversely presented).
Fix:
Skip font tables for RTL languages on OSX platform.
Thanks,
Dmitry
[1] - https://bugs.openjdk.java.net/browse/JDK-7162125
