<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<a moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eprr/8143177.1/">http://cr.openjdk.java.net/~prr/8143177.2</a><br>
<br>
is uploaded. See comments below.<br>
<br>
On 11/21/15, 2:41 AM, Vadim Pakhnushev wrote:
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
<div class="moz-cite-prefix">Phil,<br>
<br>
In the HBShaper.c in the init_JNI_IDs function I think
CHECK_NULL is missing for the gvdIndicesFID.<br>
I guess it's just a copy from SunLayoutEngine.cpp but still.<br>
</div>
</blockquote>
<br>
Actually this points out that I only return from the init function<br>
and don't return from the calling function. I will restructure this
a bit so that we<br>
return a success value and the caller checks it.<br>
<br>
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<div class="moz-cite-prefix"> Calling init_JNI_IDs each time seems
unneeded (not very harmful though), could it be somehow merged
with the Java_sun_font_SunLayoutEngine_initGVIDs?<br>
</div>
</blockquote>
<br>
They are static and I have added a static flag that indicates they
have all been initialised.<br>
If it fails then we will not proceed. In the normal case overhead
should be down to<br>
near zero.<br>
<br>
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<div class="moz-cite-prefix"> What's the point of this super
optimized euclidianDistance if it's called only 2 times while
creating a layout?<br>
</div>
</blockquote>
<br>
I just copied this from ICU code so we are doing the same thing.<br>
<br>
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<div class="moz-cite-prefix"> pScaler is not used anywhere it
seems, can it be removed altogether?<br>
</div>
</blockquote>
It was used when I was directly testing freetype (using harfbuzz's
hb-ft.cc)<br>
That was where I started out but I moved on to a different approach.<br>
It is left it there in case I wanted to test ft to isolate any bugs.<br>
<br>
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
typo in the hb-jdk-font.cc:124 * which <b>be could</b> based
on some on-the fly glyph analysis.<br>
</div>
</blockquote>
<br>
ok<br>
<br>
I had to make one additional change. The hb call to create a
coretext font<br>
was being handed the pt size in device pixels as used elsewhere but
in<br>
the coretext path we must pass the user pt size so I had to pass
that<br>
down and in .. it does not affect anything except the coretext path
for<br>
AAT fonts on OS X. I spotted this when testing with a transform in<br>
that code path.<br>
<br>
-phil.<br>
<blockquote cite="mid:56504A4B.6000709@oracle.com" type="cite">
<div class="moz-cite-prefix"> <br>
Thanks,<br>
Vadim<br>
<br>
On 21.11.2015 2:09, Phil Race wrote:<br>
</div>
<blockquote cite="mid:564FA847.5040404@oracle.com" type="cite"> <br>
On 11/19/2015 12:51 PM, Steven Loomis wrote: <br>
<blockquote type="cite">I have reviewed the non-harfbuzz portion
(outside of the /harfbuzz/ directory) and it looks good so
far. <br>
</blockquote>
<br>
Thanks. The harfbuzz code itself probably does not need careful
reviewing as it is just <br>
the upstream harfbuzz. I have updated it to remove the gpl text
from the hb src files but <br>
I haven't changed anything else :- <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eprr/8143177.1/">http://cr.openjdk.java.net/~prr/8143177.1/</a>
<br>
<br>
Any takers to be 2nd reviewer ? <br>
<br>
-phil. <br>
<br>
<blockquote type="cite"> <br>
-s <br>
<br>
On 11/17/2015 4:05 PM, Philip Race wrote: <br>
<blockquote type="cite">Webrev : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eprr/8143177/">http://cr.openjdk.java.net/~prr/8143177/</a>
<br>
Bug : <a moz-do-not-send="true"
class="moz-txt-link-freetext"
href="https://bugs.openjdk.java.net/browse/JDK-8143177">https://bugs.openjdk.java.net/browse/JDK-8143177</a>
<br>
<br>
This webrev contains the changes accumulated in the harfbuzz
project forest <br>
that migrate JDK from using ICU for opentype font layout to
harfbuzz for the same. <br>
<br>
The change does not introduce API. It is mostly adding the
harfbuzz library. <br>
The ICU library remains there but is no longer the default.
<br>
Eventually (not necessarily in 9) it may be removed once we
are comfortable <br>
with harfbuzz. <br>
You may select ICU by using -Dsun.font.layoutengine=icu <br>
<br>
You may see which layout engine is in use by using
-Dsun.font.layoutengine.verbose=true <br>
it will print either "Using harfbuzz", or "Using ICU". <br>
<br>
The change has no impact on typical Latin script rendering
but is a big advance <br>
for complex scripts and also applies if you select kerning
or ligatures for Latin. <br>
However the latter is only detectable if the font implements
support for these. <br>
By "big advance" I mean that ICU has not been updated to
recognise recent opentype features. <br>
So harfbuzz should fix a number of things but unexpected
changes that look wrong <br>
should be reported so we can investigate. <br>
<br>
To use harfbuzz we invoke its shaper and we provide a way to
get jdk font information. <br>
This means that we do not need a different layer depending
on whether freetype or t2k <br>
is used. It also enables some caching in the JDK font layer.
<br>
<br>
On macosx harfbuzz does not natively read the AAT tables but
will invoke CoreText. <br>
This does mean that an AAT font installed on Linux would not
be processed but <br>
this is not a significant issue since AAT fonts are not
found other than on macosx. <br>
<br>
The majority of the files in the webrev are harfbuzz itself,
changed only to comply <br>
with JDK whitespace rules. Reviewers should probably
concentrate on all of the rest. <br>
I've listed it so that all those files are at the beginning
and you can ignore all those <br>
that follow that are in the "harfbuzz" directory. <br>
<br>
The harfbuzz version used here is 1.0.6 which is the latest
source release (at the time of writing). <br>
We expect to update this to keep reasonably current. <br>
<br>
I would like to push this in on Friday, but at the very
latest Monday because of the <br>
upcoming Feature Complete date so there are a couple of days
to make small <br>
tweaks for serious problems but there will be plenty of time
after integration to fix <br>
remaining problems. <br>
<br>
-phil. <br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>