On Thu, 16 Nov 2023 13:48:38 GMT, Prasanta Sadhukhan <psadhuk...@openjdk.org>
wrote:
>> Phil Race has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> indentation
>
> src/java.desktop/share/classes/sun/font/HBShaper.java line 628:
>
>> 626: MemorySegment glyphInfoArr =
>> glyphInfo.reinterpret(glyphInfoSize);
>> 627:
>> 628: for (int i=0; i<glyphCount; i++) {
>
> Spacing between operators
fixed
> src/java.desktop/share/classes/sun/font/HBShaper.java line 658:
>
>> 656: startPt.x = advX;
>> 657: startPt.y = advY;
>> 658: startPt.x = advX;
>
> duplicate assignment of startPt.x to advX...
fixed
> src/java.desktop/share/classes/sun/font/SunLayoutEngine.java line 167:
>
>> 165: }
>> 166:
>> 167: static boolean useFFM = true;
>
> So, we want to enable FFM by default?
yes
> src/java.desktop/share/classes/sun/font/SunLayoutEngine.java line 184:
>
>> 182: FontStrike strike = font.getStrike(desc);
>> 183: if (useFFM) {
>> 184: java.lang.foreign.MemorySegment face =
>> HBShaper.getFace(font);
>
> Guess import at start will be more nicer...
ok .. although I do sometimes do this pattern when there's only one usage in
the file.
> src/java.desktop/share/native/libfontmanager/HBShaper_Panama.c line 141:
>
>> 139: hb_buffer_destroy (buffer);
>> 140: hb_font_destroy(hbfont);
>> 141: if (features != NULL) free(features);
>
> Guess coding style warrants braces { and next statement in separate line...
fixed
> src/java.desktop/share/native/libfontmanager/hb-jdk-font-p.cc line 238:
>
>> 236: HBFloatToFixed(ptSize*devScale),
>> 237: HBFloatToFixed(ptSize*devScale));
>> 238: return font;
>
> indentation is off..
fixed
> test/jdk/java/awt/font/GlyphVector/LayoutCompatTest.java line 29:
>
>> 27: /*
>> 28: @test
>> 29: @summary verify JNI and FFM harfbuzz OpenType layout implementations
>> are equivalent.
>
> bug id is missing
Because there isn't a "bug". It is just a test for new functionality.
> test/jdk/java/awt/font/GlyphVector/LayoutCompatTest.java line 45:
>
>> 43: public class LayoutCompatTest {
>> 44:
>> 45: static String jni = "jni.txt";
>
> Seems test is failing without fix with Exception in jtreg
> java.io.FileNotFoundException: jni.txt (The system cannot find the file
> specified)
>
> Also in standalone mode. I was expecting it will fail with RuntimeException
> "files differ byte offset"
I'm not sure why it matters what this test does in a JDK without the fix,
although logically, since the new system property isn't known, both cases would
end up using JNI, and I'd expect the test to pass. I am not sure why you say
it should fail.
And I can't reproduce your first problem, I ran this test in jtreg on an
unmodified JDK22 and it passed, as I expected, for the reason given above.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399596923
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597001
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597224
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399597892
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598026
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598126
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399598208
PR Review Comment: https://git.openjdk.org/jdk/pull/15476#discussion_r1399596012
