<div dir="ltr">Updated webrev:<div><div class="gmail-yj6qo gmail-ajU"></div></div><div><br></div><div><a href="http://cr.openjdk.java.net/~srl/8187100/webrev.03">http://cr.openjdk.java.net/~srl/8187100/webrev.03</a><br></div><div><br></div><div><div>- Use TextLayout (Harfbuzz) if VS appears.</div><div>- Composite font's behavior was changed to not change the font by VS.</div><div>These changes made the patch so simplified.</div></div><div>- add comment about suggested DejaVu version</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, May 31, 2018 at 3:19 PM Steven R. Loomis <<a href="mailto:srl@icu-project.org">srl@icu-project.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Updated webrev:<div><br></div><div><a href="http://cr.openjdk.java.net/~srl/8187100/webrev.01/" target="_blank">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">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">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">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">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">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="m_1929004908681635661h5"><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(unicodes, 0, 2, tmp);<br>>   71         env->CallVoidMethod(font2D, <br>> sunFontIDs.f2dCharsToGlyphsMID, 2, unicodes, results);<br>>   72         env->GetIntArrayRegion(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">https://bugs.openjdk.java.net/browse/JDK-8187100</a><br>> > Webrev: <a href="http://cr.openjdk.java.net/~srl/8187100/webrev.00/" target="_blank">http://cr.openjdk.java.net/~srl/8187100/webrev.00/</a><br>> <br>> Toshio Nakamura<br>> <br>> > From: "Steven R. Loomis" <<a href="mailto:srl295@gmail.com" target="_blank">srl295@gmail.com</a>><br>> > To: 2d-dev <<a href="mailto:2d-dev@openjdk.java.net" target="_blank">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">2d-dev-bounces@openjdk.java.net</a>><br>> > <br>> > I added a screenshot to <a href="https://bugs.openjdk.java.net/browse/JDK-8187100" target="_blank">https://bugs.openjdk.java.net/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">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">https://bugs.openjdk.java.net/browse/JDK-8187100</a><br>> > Webrev: <a href="http://cr.openjdk.java.net/~srl/8187100/webrev.00/" target="_blank">http://cr.openjdk.java.net/~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/classes/sun/font/CMap.java<br>> > > src/java.desktop/share/classes/sun/font/CharToGlyphMapper.java<br>> > > src/java.desktop/share/classes/sun/font/CompositeGlyphMapper.java<br>> > > src/java.desktop/share/classes/sun/font/Font2D.java<br>> > > src/java.desktop/share/classes/sun/font/FontRunIterator.java<br>> > > src/java.desktop/share/classes/sun/font/FontUtilities.java<br>> > > src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java<br>> > > src/java.desktop/share/native/common/font/sunfontids.h<br>> > > src/java.desktop/share/native/libfontmanager/hb-jdk-font.cc<br>> > > src/java.desktop/share/native/libfontmanager/sunFont.c<br>> > > src/java.desktop/share/classes/javax/swing/text/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">http://www.unicode.org/versions/Unicode10.0.0/ch23.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><p></p></div>
</blockquote></div><br></div>
</blockquote></div>