Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation Selectors(Resend)

2018-06-19 Thread Nakajima Akira 

Hi Phil.

>> > Updated webrev
>> 02 _http://cr.openjdk.java.net/~srl/8187100/webrev.02/_
>> 

I think that this patch is better than my patch.
So I drop my patch.


Thanks.
Akira Nakajima


On 2018/06/20 5:38, Phil Race wrote:



On 06/18/2018 10:28 AM, Toshio 5 Nakamura wrote:


Hi Phil,

Thank you very much for your great comments.
I tried to handle your comment as much as I could. Could you kindly
rereview it?
(So far, it contains only our contribution.)

> Updated webrev
02 _http://cr.openjdk.java.net/~srl/8187100/webrev.02/_


The following points were not directly applicable. I'd like to explain
them.

- Performance concern of fast path
We'd like to handle direct drawing such as Graphics2D.drawString, and
layout is not used in this case.
I tired to separate the original methods and VS capable ones to
minimize a impact.
Only if VS character appears, VS capable method will be called.



right and I was saying in my email of 11th June, that your way of
detecting if a VS char is there could be more efficient
It is somewhat improved in this update ..


(except FontRunIterator.java:next, which uses a member variable and
couldn't use that way.)

- Complex code of CMap
Composite fonts need to search a glyph by two steps.
1 - Search VS specific glyph in each composed font.
2 - If it's not available in all fonts, search a glyph without VS.
I changed getGlyph method with allowFallback parameter,
hope it clears our purpose.



I believe that if you are looking for a glyph for (say) U+ then it
should always come
from the same slot of a composite font whether a variation selector is
specified or not.
Otherwise you may get glyphs with variations from a different font than
the glyphs from
the same script without variations and that may look very wrong.
So I am not sure about that and maybe we'll change it in the future.

I am running some general font tests to make sure this does not break
anything,

If that passes OK then we should be OK.

-phil.



- isVariationSelector for two blocks
VS characters are U+FE00-U+FE0F and U+E0100-U+E01FF.
The latter is represented by Surrogate pair in a char array.
My previous code named the same for them, and it may cause a confusion.
I updated the names to isVariationSelectorBMP and isVariationSelectorExt.


Steven,
Thank you for your kind support.

Best regards,
Toshio Nakamura, IBM Japan

Inactive hide details for "Steven R. Loomis" ---2018/06/19
02:02:35---Updated webrev 02
https://urldefense.proofpoint.com/v2/ur"Steven R. Loomis"
---2018/06/19 02:02:35---Updated webrev 02 INVALID URI REMOVED


From: "Steven R. Loomis" 
To: Toshio 5 Nakamura 
Cc: Philip Race , 2d-dev <2d-dev@openjdk.java.net>
Date: 2018/06/19 02:02
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation
Selectors(Resend)
Sent by: srl...@gmail.com





Updated webrev 02 _http://cr.openjdk.java.net/~srl/8187100/webrev.02/_


On Thu, May 31, 2018 at 3:19 PM, Steven R. Loomis
<_srl@icu-project.org_ > wrote:

Updated webrev:

_http://cr.openjdk.java.net/~srl/8187100/webrev.01/_


On Fri, May 18, 2018 at 9:16 AM, Toshio 5 Nakamura
<_toshi...@jp.ibm.com_ > wrote:
Thank you for your review, Phil.
I'm working to handle your points.

Toshio Nakamura

Philip Race <_philip.race@oracle.com_
> wrote on 2018/05/18 13:39:35:

> From: Philip Race <_philip.race@oracle.com_
>
> To: Toshio 5 Nakamura <_toshi...@jp.ibm.com_
>
> Cc: 2d-dev <_2d-...@openjdk.java.net_
>
> Date: 2018/05/18 13:39


> Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation
> Selectors(Resend)
>
> There's a lot to think about here but I have some requests to first
> clean up the proposed code to conform to coding standards.
>
> I see lots of lines > 80 chars. Please fix
>
> I see if(foo){ and else{ which should be "if (foo) {" and "else {"
>
> Basically always have a space before { and after ) and around
"=" and "=="
>
> One line that WAS
>   51 hb_codepoint_t u = (variation_selector==0) ? unicode :
> variation_selector;
>
> has no spaces around "==" almost certainly to avoid going over
80 chars.
> Now you've broken the line you can fix that.

>
> Eliminate all wild card imports and import exactly what is needed.
> Maybe this is only about the test.
>
> remove what looks like SCCS style comments on the @test line.
> Make the test simply print a warning if the person didn't install
> fonts where

Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation Selectors(Resend)

2018-06-19 Thread Phil Race 



On 06/18/2018 10:28 AM, Toshio 5 Nakamura wrote:


Hi Phil,

Thank you very much for your great comments.
I tried to handle your comment as much as I could. Could you kindly 
rereview it?

(So far, it contains only our contribution.)

> Updated webrev 02 
_http://cr.openjdk.java.net/~srl/8187100/webrev.02/_ 



The following points were not directly applicable. I'd like to explain 
them.


- Performance concern of fast path
We'd like to handle direct drawing such as Graphics2D.drawString, and 
layout is not used in this case.
I tired to separate the original methods and VS capable ones to 
minimize a impact.

Only if VS character appears, VS capable method will be called.



right and I was saying in my email of 11th June, that your way of 
detecting if a VS char is there could be more efficient

It is somewhat improved in this update ..

(except FontRunIterator.java:next, which uses a member variable and 
couldn't use that way.)


- Complex code of CMap
Composite fonts need to search a glyph by two steps.
1 - Search VS specific glyph in each composed font.
2 - If it's not available in all fonts, search a glyph without VS.
I changed getGlyph method with allowFallback parameter,
hope it clears our purpose.



I believe that if you are looking for a glyph for (say) U+ then it 
should always come
from the same slot of a composite font whether a variation selector is 
specified or not.
Otherwise you may get glyphs with variations from a different font than 
the glyphs from

the same script without variations and that may look very wrong.
So I am not sure about that and maybe we'll change it in the future.

I am running some general font tests to make sure this does not break 
anything,


If that passes OK then we should be OK.

-phil.



- isVariationSelector for two blocks
VS characters are U+FE00-U+FE0F and U+E0100-U+E01FF.
The latter is represented by Surrogate pair in a char array.
My previous code named the same for them, and it may cause a confusion.
I updated the names to isVariationSelectorBMP and isVariationSelectorExt.


Steven,
Thank you for your kind support.

Best regards,
Toshio Nakamura, IBM Japan

Inactive hide details for "Steven R. Loomis" ---2018/06/19 
02:02:35---Updated webrev 02 
https://urldefense.proofpoint.com/v2/ur"Steven R. Loomis" 
---2018/06/19 02:02:35---Updated webrev 02 INVALID URI REMOVED 



From: "Steven R. Loomis" 
To: Toshio 5 Nakamura 
Cc: Philip Race , 2d-dev <2d-dev@openjdk.java.net>
Date: 2018/06/19 02:02
Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation 
Selectors(Resend)

Sent by: srl...@gmail.com





Updated webrev 02 _http://cr.openjdk.java.net/~srl/8187100/webrev.02/_ 



On Thu, May 31, 2018 at 3:19 PM, Steven R. Loomis 
<_srl@icu-project.org_ > wrote:


Updated webrev:

_http://cr.openjdk.java.net/~srl/8187100/webrev.01/_


On Fri, May 18, 2018 at 9:16 AM, Toshio 5 Nakamura
<_toshi...@jp.ibm.com_ > wrote:
Thank you for your review, Phil.
I'm working to handle your points.

Toshio Nakamura

Philip Race <_philip.race@oracle.com_
> wrote on 2018/05/18 13:39:35:

> From: Philip Race <_philip.race@oracle.com_
>
> To: Toshio 5 Nakamura <_toshi...@jp.ibm.com_
>
> Cc: 2d-dev <_2d-...@openjdk.java.net_
>
> Date: 2018/05/18 13:39


> Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation
> Selectors(Resend)
>
> There's a lot to think about here but I have some requests to first
> clean up the proposed code to conform to coding standards.
>
> I see lots of lines > 80 chars. Please fix
>
> I see if(foo){ and else{ which should be "if (foo) {" and "else {"
>
> Basically always have a space before { and after ) and around
"=" and "=="
>
> One line that WAS
>   51 hb_codepoint_t u = (variation_selector==0) ? unicode :
> variation_selector;
>
> has no spaces around "==" almost certainly to avoid going over
80 chars.
> Now you've broken the line you can fix that.

>
> Eliminate all wild card imports and import exactly what is needed.
> Maybe this is only about the test.
>
> remove what looks like SCCS style comments on the @test line.
> Make the test simply print a warning if the person didn't install
> fonts where you expected.
> Why? Because @ignore defaults to .. not being ignored.
>
> For the JNI methods calls read the spec and see if calling them may
> throw an exception
>
> I'm looking at sequences like
>