On Wed, 5 Mar 2025 15:54:49 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Changed the StubTextLayout to use product PrismTextLayout with much >> simplified glyph layout (via stubbed fonts). The new layout assumes all the >> glyphs are squares of font size, while the bold type face produces wider >> glyphs (by 1 pixel). The default font size has changed from 10 to 12 to >> make it closer to win/linux. >> >> This brings the test environment closer to the product configuration and >> expands the capabilities of our headless testing pipeline, which will be >> useful for upcoming behavior tests. >> >> Existing tests have been adjusted/reworked mainly due to default font size >> change. > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 24 additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into 8342565.stub.text.layout > - Merge remote-tracking branch 'origin/master' into 8342565.stub.text.layout > - Merge remote-tracking branch 'origin/master' into 8342565.stub.text.layout > - review comments > - Merge remote-tracking branch 'origin/master' into 8342565.stub.text.layout > - atomic boolean > - Merge branch 'master' into 8342565.stub.text.layout > - cleanup > - better test > - cleanup > - ... and 14 more: https://git.openjdk.org/jfx/compare/302a3dce...3650989a Code looks good to me, some minor convention related things from my side. modules/javafx.controls/src/test/addExports line 38: > 36: --add-exports=javafx.graphics/com.sun.javafx.menu=ALL-UNNAMED > 37: --add-exports=javafx.graphics/com.sun.javafx.text=ALL-UNNAMED > 38: minor: extra added newline here modules/javafx.graphics/src/main/java/com/sun/javafx/text/GlyphLayoutManager.java line 43: > 41: public class GlyphLayoutManager { > 42: private static GlyphLayout reusableGL = newInstance(); > 43: private static final AtomicBoolean guard = new AtomicBoolean(false); if we follow the java standard, then this should be named `GUARD` modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayoutFactory.java line 34: > 32: > 33: public class PrismTextLayoutFactory implements TextLayoutFactory { > 34: private static final PrismTextLayoutFactory factory = new > PrismTextLayoutFactory(); Again, if we want to follow the practise this should be uppercase modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayoutFactory.java line 37: > 35: /* Same strategy as GlyphLayout */ > 36: private static final TextLayout reusableTL = factory.createLayout(); > 37: private static final AtomicBoolean guard = new AtomicBoolean(false); same here ------------- Marked as reviewed by mhanl (Committer). PR Review: https://git.openjdk.org/jfx/pull/1667#pullrequestreview-2664025448 PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r1983109268 PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r1983105350 PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r1983107627 PR Review Comment: https://git.openjdk.org/jfx/pull/1667#discussion_r1983107282
