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

[Prasanta Sadhukhan]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20180913/45b3763a/attachment-0001.html>