<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>