Updated webrev 02 http://cr.openjdk.java.net/~srl/8187100/webrev.02/
On Thu, May 31, 2018 at 3:19 PM, Steven R. Loomis <s...@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.r...@oracle.com> wrote on 2018/05/18 13:39:35: >> >> > From: Philip Race <philip.r...@oracle.com> >> > To: Toshio 5 Nakamura <toshi...@jp.ibm.com> >> > Cc: 2d-dev <2d-dev@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 >> > env->SetIntArrayRegion(unicodes, 0, 2, tmp); >> > 71 env->CallVoidMethod(font2D, >> > sunFontIDs.f2dCharsToGlyphsMID, 2, unicodes, results); >> > 72 env->GetIntArrayRegion(results, 0, 2, tmp); >> > 73 *glyph = tmp[0]; >> > 74 cleanup: >> > 75 if (unicodes != NULL) env->DeleteLocalRef(unicodes); >> > 76 if (results != NULL) env->DeleteLocalRef(results); >> > >> >> > If call GetIntArrayRegion may leave a pending exception it needs to >> > be checked and cleared >> > before you make another call. >> > >> > Look at the performance implications of things like the extra >> > work done now in FontUtilities.isSimpleChar() and see how to minimise >> > it since it could affect all text rendering in a way that is measurable >> > in at least some way. >> > >> > I idly wonder if >> > >> > >> > public static boolean isBaseChar(int charCode){ ... >> > >> > might be more cleanly or efficiently implemented with a switch ? >> > >> > Not sure. >> > >> > -phil. >> > >> > On 5/14/18, 1:28 AM, Toshio 5 Nakamura wrote: >> > Could someone review our proposal to support Unicode Variation >> Selectors? >> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8187100 >> > > Webrev: http://cr.openjdk.java.net/~srl/8187100/webrev.00/ >> > >> > Toshio Nakamura >> > >> > > From: "Steven R. Loomis" <srl...@gmail.com> >> > > To: 2d-dev <2d-dev@openjdk.java.net> >> > > Date: 2018/05/03 03:27 >> > > Subject: Re: [OpenJDK 2D-Dev] RFR: JDK-8187100: support Variation >> > > Selectors (Resend) >> > > Sent by: "2d-dev" <2d-dev-boun...@openjdk.java.net> >> > > >> > > I added a screenshot to https://bugs.openjdk.java.net/ >> browse/JDK-8187100 >> > > if anyone wants to see what the impact of this fix is >> > > >> > > On Wed, Apr 25, 2018 at 8:39 AM, Steven R. Loomis <srl...@gmail.com> >> wrote: >> > > (Retrying as actual text) >> > > >> > > Support Unicode Variation Selectors. >> > > >> > > Code by my colleague Toshio Nakamura, I added a simple test, and >> > > include a test that was part of JDK 8187100. (Both tests are run >> manually.) >> > > >> > > Bug: https://bugs.openjdk.java.net/browse/JDK-8187100 >> > > Webrev: http://cr.openjdk.java.net/~srl/8187100/webrev.00/ >> > > >> > > On 04/08/2018 11:46 PM, Toshio 5 Nakamura wrote: >> > > > >> > > > Hello >> > > > >> > > > IBM would like to propose Unicode Variation Selector[1] capability >> to AWT >> > > > and Swing components. >> > > > (This proposal was posted to i18n-dev first, and I got a suggestion >> to >> > > > discuss >> > > > in 2d-dev.) >> > > > >> > > > This proposal changed the following files: >> > > > src/java.desktop/share/classes/sun/font/CMap.java >> > > > src/java.desktop/share/classes/sun/font/CharToGlyphMapper.java >> > > > src/java.desktop/share/classes/sun/font/CompositeGlyphMapper.java >> > > > src/java.desktop/share/classes/sun/font/Font2D.java >> > > > src/java.desktop/share/classes/sun/font/FontRunIterator.java >> > > > src/java.desktop/share/classes/sun/font/FontUtilities.java >> > > > src/java.desktop/share/classes/sun/font/TrueTypeGlyphMapper.java >> > > > src/java.desktop/share/native/common/font/sunfontids.h >> > > > src/java.desktop/share/native/libfontmanager/hb-jdk-font.cc >> > > > src/java.desktop/share/native/libfontmanager/sunFont.c >> > > > src/java.desktop/share/classes/javax/swing/text/DefaultEdito >> rKit.java >> > > > 542 lines will be changed. >> > > > >> > > > There are three parts. >> > > > 1) Adding CMap format 14 support >> > > > 2) Adding CharsToGlyphs functions support Variation Selector >> Sequences >> > > > 3) Swing text component's DEL and BS key operations change >> > > > >> > > > >> > > > How would I go about obtaining a sponsor? >> > > > >> > > > [1] _http://www.unicode.org/versions/Unicode10.0.0/ch23.pdf_ >> > > > Chapter 23.4 Variation Selectors >> > > > >> > > > Best regards, >> > > > >> > > > Toshio Nakamura >> > > > IBM Japan >> > > >> >> >
