[OpenJDK 2D-Dev] [8u] RFR 8201801: RTL language (Hebrew) is presented from left to right

[Phil Race]

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 <dmitry.markov at oracle.com 
>> <mailto:dmitry.markov at oracle.com>> 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 <philip.race at oracle.com 
>>> <mailto:philip.race at oracle.com>> 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 
>>>>> <prasanta.sadhukhan at oracle.com 
>>>>> <mailto:prasanta.sadhukhan at oracle.com>> 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
>>>>>
>>>>
>>>
>>
>