<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8">
  </head>
  <body text="#000000" bgcolor="#FFFFFF">
    It seems that although there is code added at the harfbuzz level<br>
    to locate variations, this is being treated analagously to the
    surrogates<br>
    where the basic font code can handle it, inserting empty glyphs, and<br>
    not caching the glyphs for the base unicode code point.<br>
    This <br>
    - leads to the need to update the DefaultEditorKit code but I<br>
    am not sure if this is missing anything.<br>
    - adds complexity and overhead to that basic fast path, that seems
    to much exceed<br>
    what we have with surrogates.<br>
    <br>
    Did you look at just handling variations purely by layout ?<br>
    <br>
    For example we could explore having isComplexCharCode return true
    for these and<br>
    directing all uses down the layout path ? Then we can maybe remove
    all this logic from<br>
    case used by the fast path code.<br>
    <br>
    Parsing of the table .. <br>
    <br>
    Somewhere you should catch IOException or any other exception<br>
    that might occur for a corrupt or invalid table<br>
    Probably wrap the "new UVS(..)" call in a catch Throwable.<br>
    Then we can ignore an invalid table.<span class="new"></span><br>
    <span class="new"></span>
    <pre><span class="new">
The values read should all be positive but it seems to me we may
read some of them as negative.
</span>
<span class="new">+                int tableOffset = buffer.getInt(offset + 10 + i * 11 + 3);
</span>
<span class="new"><span class="new">+                tableOffset = buffer.getInt(offset + 10 + i * 11 + 7);

If these are negative we should bail !



</span></span>And this :
<span class="new"><span class="new">+                        additionalCount[i][j] =  buffer.get();</span> 


is storing a uint8 into a byte, so it can't even hold the valid range!

You will need to store into a short after masking with &0FF
'
</span>Is there some logic we can use to verify the size of arrays such as these is valid ?


<span class="new">+            selector = new int[numSelectors];

</span><span class="new">+                    startUnicodeValue[i] = new int[numUnicodeValueRanges[i]];

</span><span class="new">+                    unicodeValue[i] = new int[numUVSMapping[i]];

For example I think if iterating over the number would inevitably make us
read off the end of the table, we should bail there and then.

Also the spec says some things must be stored in increasing order.
As we parse the table, we should validate that and bail if the table
does not follow the rules.
</span></pre>
    And also either when parsing the table, for the non-default case,<br>
    where a glyphID is specified, or later, when a lookup happens,<br>
    there should be some validation of this against the actual number<br>
    of glyphs supported in the font so we don't hand off an invalid<br>
    glyph ID to calling code.<br>
    <br>
    <pre><span class="new">+                    if (index >=0 &&

add a space : " >= 0"


</span></pre>
    <br>
    <pre><span class="new">+</span>
<span class="new">+        /* getGlyph for Variation selector</span>
<span class="new">+           return value:</span>
<span class="new">+            0: A special glyph for the variation selector is Not found</span>
<span class="new">+           -1: Default glyph should be used</span>
<span class="new">+           0>: A special glyph is found</span>
<span class="new">+        */</span>
<span class="new">+        int getGlyph(int charCode, int variationSelector) {

I don't understand why we need or want 0 here.

Looking at 
</span>
<span class="new"><span class="new">+    public char getGlyph(int charCode, int variationSelector) {</span>
<span class="new">+        if (uvs == null) {</span>
<span class="new">+            return 0;</span>
<span class="new">+        }</span>
<span class="new">+        int result = uvs.getGlyph(charCode, variationSelector);</span>
<span class="new">+        if (result == -1) {</span>
<span class="new">+            result = this.getGlyph(charCode);</span>
<span class="new">+        }</span>
<span class="new">+        return (char)(result & 0xFFFF);</span>
<span class="new">+    }

It seems that if the UVS subtable has no entry that corresponds we'll
return the missing glyph. don't you just want to ignore the variation
selector and so behave as if -1 was returned ?

Put another way, supposing someone provides an invalid selector, or
that the unicode spec added a new standardised selector after this
font was created. It may have no mapping for it because it just
is unaware. You'll return a missing glyph instead of the far more
logical base glyph ..

It is not even clear to me that the spec says you must map every
single unicode point, so this seems more or less inevitable.
</span></span><span class="new"><span class="new"><span class="new">
</span></span></span>
I do see that in TrueTypeGlyphMapper.java 

<span class="new"><span class="new"><span class="new"><span class="new">  96     private char getGlyphFromCMAP(int charCode, int variationSelector) {

</span>It looks for the zero return and will handle that there (as well as the out of
range glyph) but I don't see why we can't handle that directly in the CMAP code ?

I'd really appreciate some comments pointing to the spec for these
</span></span></span>and describing them :<span class="new"><span class="new"><span class="new"><span class="new"><span class="new">
</span></span>
<span class="new"><span class="new"><span class="new">+    public static final int VS_START = 0xFE00;</span>
<span class="new">+    public static final int VS_END = 0xFE0F;</span>
<span class="new">+    public static final int VSS_START = 0xE0100;</span>
<span class="new">+    public static final int VSS_END = 0xE01FF;</span></span></span>
</span>

</span></span><a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~srl/8187100/webrev.01/src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java.sdiff.html">http://cr.openjdk.java.net/~srl/8187100/webrev.01/src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java.sdiff.html</a>
<span class="new"><span class="new"><span class="new"> 118         } catch(Exception e) {

missing space.

Look at this :

</span></span></span><span class="new">219             if (i < count - step &&</span>
<span class="new"> 220                 isVariationSelector(unicodes[i + step]) &&</span>
<span class="new"> 221                 isBaseChar(code)) {</span>
<span class="new"> 222                 variationSelector = unicodes[i + step];</span>
<span class="new"> 223                 glyphs[i] = getGlyphFromCMAP(code, variationSelector);</span>
<span class="new"> 224                 glyphs[i+step] = INVISIBLE_GLYPH_ID;</span>
<span class="new"> 225                 i += 1;</span>
<span class="new"> 226             } else if (i < count - step -1 &&</span>
<span class="new"> 227                        isVariationSelector(unicodes[i + step], </span>
<span class="new"> 228                                            unicodes[i + step + 1]) &&

It seems to me that except for the last code point, you will be calling
isVariationSelector() twice for every code point which in all likelihood is
always going to return false.

This is already adding gobs of overhead and we can't afford to be wasteful like this.

</span>
<span class="new"><span class="new">I think maybe the problem is that this is being inserted into the low level chars
to glyph used even by drawGlyphVector</span>

If this can't be made more efficient, then maybe it should be just part of the TextLayout path
as mentioned earlier.

can we have a 
boolean maybeVariationSelector(char c) {
  return c >= </span>HI_SURROGATE_START;<span class="new">
}

99.99% of the time that one test will return false and then you know
you will not see a variation selector, or a surrogate encoded variation  selector </span>
<span class="new"><span class="new">supplement</span> code point since all possibilities are excluded.

So you can write 
boolean maybe = </span><span class="new"> maybeVariationSelector(</span>unicodes[i+step]);
if (maybe) {
... only then do the detailed checking
}</pre>
    <br>
    <span class="changed">
    </span><br>
    <span class="changed"> 56 env->ExceptionDescribe();</span><br>
    This is for debugging. Don't use it in production code.<br>
    <br>
    Since we are trying to be careful about clearing exceptions, I am
    fairly sure<br>
    the allocation calls like this :<br>
    <pre><span class="changed">  65         unicodes = env->NewIntArray(2);
and
</span><span class="changed">  69         results = env->NewIntArray(2);

may throw OutOfMemoryError as well as returning NULL.


So cleanup: should start with
</span><span class="changed">if (env->ExceptionOccurred()) {
    </span><span class="changed">env->ExceptionClear();</span>
<span class="changed"> }


This :
</span><span class="changed">74         tmp = new jint[2];

would be more efficient as a stack allocation and then you don't need to free it either


-phil.
</span></pre>
    <div class="moz-cite-prefix">On 05/31/2018 03:19 PM, Steven R.
      Loomis wrote:<br>
    </div>
    <blockquote type="cite"
cite="mid:CAFYQx+BBmswPaKaFO1Q44czS4NAEv9a91gO8t-41m_yJKdU-Aw@mail.gmail.com">
      <div dir="ltr">Updated webrev:
        <div><br>
        </div>
        <div><a
            href="http://cr.openjdk.java.net/%7Esrl/8187100/webrev.01/"
            moz-do-not-send="true">http://cr.openjdk.java.net/~srl/8187100/webrev.01/</a><br>
        </div>
      </div>
      <div class="gmail_extra"><br>
        <div class="gmail_quote">On Fri, May 18, 2018 at 9:16 AM, Toshio
          5 Nakamura <span dir="ltr"><<a
              href="mailto:TOSHIONA@jp.ibm.com" target="_blank"
              moz-do-not-send="true">TOSHIONA@jp.ibm.com</a>></span>
          wrote:<br>
          <blockquote class="gmail_quote" style="margin:0 0 0
            .8ex;border-left:1px #ccc solid;padding-left:1ex">
            <div>
              <p><font size="2">Thank you for your review, Phil.</font><br>
                <font size="2">I'm working to handle your points.<br>
                  <br>
                  Toshio Nakamura<br>
                </font><br>
                <tt><font size="2">Philip Race <<a
                      href="mailto:philip.race@oracle.com"
                      target="_blank" moz-do-not-send="true">philip.race@oracle.com</a>>
                    wrote on 2018/05/18 13:39:35:<br>
                    <br>
                    > From: Philip Race <<a
                      href="mailto:philip.race@oracle.com"
                      target="_blank" moz-do-not-send="true">philip.race@oracle.com</a>></font></tt><br>
                <tt><font size="2">> To: Toshio 5 Nakamura <<a
                      href="mailto:TOSHIONA@jp.ibm.com" target="_blank"
                      moz-do-not-send="true">TOSHIONA@jp.ibm.com</a>></font></tt><br>
                <tt><font size="2">> Cc: 2d-dev <<a
                      href="mailto:2d-dev@openjdk.java.net"
                      target="_blank" moz-do-not-send="true">2d-dev@openjdk.java.net</a>></font></tt><br>
                <tt><font size="2">> Date: 2018/05/18 13:39</font></tt></p>
              <div>
                <div class="h5"><br>
                  <tt><font size="2">> Subject: Re: [OpenJDK 2D-Dev]
                      RFR: JDK-8187100: support Variation <br>
                      > Selectors(Resend)</font></tt><br>
                  <tt><font size="2">> <br>
                      > There's a lot to think about here but I have
                      some requests to first<br>
                      > clean up the proposed code to conform to
                      coding standards.<br>
                      > <br>
                      > I see lots of lines > 80 chars. Please fix<br>
                      > <br>
                      > I see if(foo){ and else{ which should be "if
                      (foo) {" and "else {"<br>
                      > <br>
                      > Basically always have a space before { and
                      after ) and around "=" and "=="<br>
                      > <br>
                      > One line that WAS</font></tt><br>
                  <tt><font size="2">>   51     hb_codepoint_t u =
                      (variation_selector==0) ? unicode : <br>
                      > variation_selector;<br>
                      > <br>
                      > has no spaces around "==" almost certainly to
                      avoid going over 80 chars.<br>
                      > Now you've broken the line you can fix that.<br>
                    </font></tt><br>
                  <tt><font size="2">> <br>
                      > Eliminate all wild card imports and import
                      exactly what is needed.<br>
                      > Maybe this is only about the test.<br>
                      > <br>
                      > remove what looks like SCCS style comments on
                      the @test line.<br>
                      > Make the test simply print a warning if the
                      person didn't install <br>
                      > fonts where you expected.<br>
                      > Why? Because @ignore defaults to .. not being
                      ignored.<br>
                      > <br>
                      > For the JNI methods calls read the spec and
                      see if calling them may <br>
                      > throw an exception<br>
                      > <br>
                      > I'm looking at sequences like</font></tt><br>
                  <tt><font size="2">> env->SetIntArrayRegion(<wbr>unicodes,
                      0, 2, tmp);<br>
                      >   71         env->CallVoidMethod(font2D, <br>
                      > sunFontIDs.<wbr>f2dCharsToGlyphsMID, 2,
                      unicodes, results);<br>
                      >   72         env->GetIntArrayRegion(<wbr>results,
                      0, 2, tmp);<br>
                      >   73         *glyph = tmp[0];<br>
                      >   74 cleanup:<br>
                      >   75         if (unicodes != NULL)
                      env->DeleteLocalRef(unicodes);<br>
                      >   76         if (results != NULL)
                      env->DeleteLocalRef(results);<br>
                      > <br>
                    </font></tt><br>
                  <tt><font size="2">> If call GetIntArrayRegion may
                      leave a pending exception it needs to <br>
                      > be checked and cleared<br>
                      > before you make another call.<br>
                      > <br>
                      > Look at the performance implications of
                      things like the extra<br>
                      > work done now in FontUtilities.isSimpleChar()
                      and see how to minimise<br>
                      > it since it could affect all text rendering
                      in a way that is measurable<br>
                      > in at least some way.<br>
                      > <br>
                      > I idly wonder if<br>
                      > <br>
                      > <br>
                      >        public static boolean isBaseChar(int
                      charCode){ ...<br>
                      > <br>
                      > might be more cleanly or efficiently
                      implemented with a switch ?<br>
                      > <br>
                      > Not sure.<br>
                      > <br>
                      > -phil.<br>
                      >   <br>
                      > On 5/14/18, 1:28 AM, Toshio 5 Nakamura wrote:
                    </font></tt><br>
                  <tt><font size="2">> Could someone review our
                      proposal to support Unicode Variation Selectors?<br>
                      > > Bug: <a
                        href="https://bugs.openjdk.java.net/browse/JDK-8187100"
                        target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/<wbr>browse/JDK-8187100</a><br>
                      > > Webrev: <a
                        href="http://cr.openjdk.java.net/%7Esrl/8187100/webrev.00/"
                        target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~<wbr>srl/8187100/webrev.00/</a><br>
                      > <br>
                      > Toshio Nakamura<br>
                      > <br>
                      > > From: "Steven R. Loomis" <<a
                        href="mailto:srl295@gmail.com" target="_blank"
                        moz-do-not-send="true">srl295@gmail.com</a>><br>
                      > > To: 2d-dev <<a
                        href="mailto:2d-dev@openjdk.java.net"
                        target="_blank" moz-do-not-send="true">2d-dev@openjdk.java.net</a>><br>
                      > > Date: 2018/05/03 03:27<br>
                      > > Subject: Re: [OpenJDK 2D-Dev] RFR:
                      JDK-8187100: support Variation <br>
                      > > Selectors (Resend)<br>
                      > > Sent by: "2d-dev" <<a
                        href="mailto:2d-dev-bounces@openjdk.java.net"
                        target="_blank" moz-do-not-send="true">2d-dev-bounces@openjdk.java.<wbr>net</a>><br>
                      > > <br>
                      > > I added a screenshot to <a
                        href="https://bugs.openjdk.java.net/browse/JDK-8187100"
                        target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/<wbr>browse/JDK-8187100</a><br>
                      > > if anyone wants to see what the impact
                      of this fix is<br>
                      > > <br>
                      > > On Wed, Apr 25, 2018 at 8:39 AM, Steven
                      R. Loomis <<a href="mailto:srl295@gmail.com"
                        target="_blank" moz-do-not-send="true">srl295@gmail.com</a>>
                      wrote:<br>
                      > > (Retrying as actual text)<br>
                      > > <br>
                      > > Support Unicode Variation Selectors.<br>
                      > >  <br>
                      > > Code by my colleague Toshio Nakamura,  I
                      added a simple test, and <br>
                      > > include a test that was part of JDK
                      8187100. (Both tests are run manually.) <br>
                      > >  <br>
                      > > Bug: <a
                        href="https://bugs.openjdk.java.net/browse/JDK-8187100"
                        target="_blank" moz-do-not-send="true">https://bugs.openjdk.java.net/<wbr>browse/JDK-8187100</a><br>
                      > > Webrev: <a
                        href="http://cr.openjdk.java.net/%7Esrl/8187100/webrev.00/"
                        target="_blank" moz-do-not-send="true">http://cr.openjdk.java.net/~<wbr>srl/8187100/webrev.00/</a><br>
                      > >  <br>
                      > > On 04/08/2018 11:46 PM, Toshio 5
                      Nakamura wrote:<br>
                      > > ><br>
                      > > > Hello<br>
                      > > ><br>
                      > > > IBM would like to propose Unicode
                      Variation Selector[1] capability to AWT<br>
                      > > > and Swing components.<br>
                      > > > (This proposal was posted to
                      i18n-dev first, and I got a suggestion to <br>
                      > > > discuss<br>
                      > > >  in 2d-dev.)<br>
                      > > ><br>
                      > > > This proposal changed the following
                      files:<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/CMap.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/<wbr>CharToGlyphMapper.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/<wbr>CompositeGlyphMapper.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/Font2D.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/<wbr>FontRunIterator.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/<wbr>FontUtilities.java<br>
                      > > > src/java.desktop/share/<wbr>classes/sun/font/<wbr>TrueTypeGlyphMapper.java<br>
                      > > > src/java.desktop/share/native/<wbr>common/font/sunfontids.h<br>
                      > > > src/java.desktop/share/native/<wbr>libfontmanager/hb-jdk-font.cc<br>
                      > > > src/java.desktop/share/native/<wbr>libfontmanager/sunFont.c<br>
                      > > > src/java.desktop/share/<wbr>classes/javax/swing/text/<wbr>DefaultEditorKit.java<br>
                      > > > 542 lines will be changed.<br>
                      > > ><br>
                      > > > There are three parts.<br>
                      > > > 1) Adding CMap format 14 support<br>
                      > > > 2) Adding CharsToGlyphs functions
                      support Variation Selector Sequences<br>
                      > > > 3) Swing text component's DEL and
                      BS key operations change<br>
                      > > ><br>
                      > > ><br>
                      > > > How would I go about obtaining a
                      sponsor?<br>
                      > > ><br>
                      > > > [1] _<a
                        href="http://www.unicode.org/versions/Unicode10.0.0/ch23.pdf_"
                        target="_blank" moz-do-not-send="true">http://www.unicode.org/<wbr>versions/Unicode10.0.0/ch23.<wbr>pdf_</a><br>
                      > > >      Chapter 23.4 Variation
                      Selectors<br>
                      > > ><br>
                      > > > Best regards,<br>
                      > > ><br>
                      > > > Toshio Nakamura<br>
                      > > > IBM Japan<br>
                      > >  </font></tt><br>
                </div>
              </div>
            </div>
          </blockquote>
        </div>
        <br>
      </div>
    </blockquote>
    <br>
  </body>
</html>