Phil,
In the HBShaper.c in the init_JNI_IDs function I think CHECK_NULL is
missing for the gvdIndicesFID.
I guess it's just a copy from SunLayoutEngine.cpp but still.
Calling init_JNI_IDs each time seems unneeded (not very harmful though),
could it be somehow merged with the Java_sun_font_SunLayoutEngine_initGVIDs?
What's the point of this super optimized euclidianDistance if it's
called only 2 times while creating a layout?
pScaler is not used anywhere it seems, can it be removed altogether?
typo in the hb-jdk-font.cc:124 * which *be could* based on some
on-the fly glyph analysis.
Thanks,
Vadim
On 21.11.2015 2:09, Phil Race wrote:
On 11/19/2015 12:51 PM, Steven Loomis wrote:
I have reviewed the non-harfbuzz portion (outside of the /harfbuzz/
directory) and it looks good so far.
Thanks. The harfbuzz code itself probably does not need careful
reviewing as it is just
the upstream harfbuzz. I have updated it to remove the gpl text from
the hb src files but
I haven't changed anything else :-
http://cr.openjdk.java.net/~prr/8143177.1/
Any takers to be 2nd reviewer ?
-phil.
-s
On 11/17/2015 4:05 PM, Philip Race wrote:
Webrev : http://cr.openjdk.java.net/~prr/8143177/
Bug : https://bugs.openjdk.java.net/browse/JDK-8143177
This webrev contains the changes accumulated in the harfbuzz project
forest
that migrate JDK from using ICU for opentype font layout to harfbuzz
for the same.
The change does not introduce API. It is mostly adding the harfbuzz
library.
The ICU library remains there but is no longer the default.
Eventually (not necessarily in 9) it may be removed once we are
comfortable
with harfbuzz.
You may select ICU by using -Dsun.font.layoutengine=icu
You may see which layout engine is in use by using
-Dsun.font.layoutengine.verbose=true
it will print either "Using harfbuzz", or "Using ICU".
The change has no impact on typical Latin script rendering but is a
big advance
for complex scripts and also applies if you select kerning or
ligatures for Latin.
However the latter is only detectable if the font implements support
for these.
By "big advance" I mean that ICU has not been updated to recognise
recent opentype features.
So harfbuzz should fix a number of things but unexpected changes
that look wrong
should be reported so we can investigate.
To use harfbuzz we invoke its shaper and we provide a way to get jdk
font information.
This means that we do not need a different layer depending on
whether freetype or t2k
is used. It also enables some caching in the JDK font layer.
On macosx harfbuzz does not natively read the AAT tables but will
invoke CoreText.
This does mean that an AAT font installed on Linux would not be
processed but
this is not a significant issue since AAT fonts are not found other
than on macosx.
The majority of the files in the webrev are harfbuzz itself, changed
only to comply
with JDK whitespace rules. Reviewers should probably concentrate on
all of the rest.
I've listed it so that all those files are at the beginning and you
can ignore all those
that follow that are in the "harfbuzz" directory.
The harfbuzz version used here is 1.0.6 which is the latest source
release (at the time of writing).
We expect to update this to keep reasonably current.
I would like to push this in on Friday, but at the very latest
Monday because of the
upcoming Feature Complete date so there are a couple of days to make
small
tweaks for serious problems but there will be plenty of time after
integration to fix
remaining problems.
-phil.
