RFR: 8183978 : Remove ICU layout code from OpenJDK

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8183978 : Remove ICU layout code from OpenJDK

Phil Race
Bug: https://bugs.openjdk.java.net/browse/JDK-8183978
Webrev: http://cr.openjdk.java.net/~prr/8183978/

This fix removes the obsolete ICU opentype layout code from JDK 10.

I've built this on all platforms (including open+closed builds).

-phil.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183978 : Remove ICU layout code from OpenJDK

Steven R. Loomis
approve

On Wed, Oct 11, 2017 at 9:59 AM, Phil Race <[hidden email]> wrote:
Bug: https://bugs.openjdk.java.net/browse/JDK-8183978
Webrev: http://cr.openjdk.java.net/~prr/8183978/

This fix removes the obsolete ICU opentype layout code from JDK 10.

I've built this on all platforms (including open+closed builds).

-phil.

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183978 : Remove ICU layout code from OpenJDK

Sergey Bylokhov
Looks fine.
I guess we should update TestLayoutVsICU.java test which was added as
part of harfbuzz integration. I assumed that it should compares behavior
against icu, but it seems it is never executed.

On 12/10/2017 10:45, Steven R. Loomis wrote:

> approve
>
> On Wed, Oct 11, 2017 at 9:59 AM, Phil Race <[hidden email]
> <mailto:[hidden email]>> wrote:
>
>     Bug: https://bugs.openjdk.java.net/browse/JDK-8183978
>     <https://bugs.openjdk.java.net/browse/JDK-8183978>
>     Webrev: http://cr.openjdk.java.net/~prr/8183978/
>     <http://cr.openjdk.java.net/~prr/8183978/>
>
>     This fix removes the obsolete ICU opentype layout code from JDK 10.
>
>     I've built this on all platforms (including open+closed builds).
>
>     -phil.
>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183978 : Remove ICU layout code from OpenJDK

Phil Race
I am not sure that the test is any less valid .. it has recorded
specific results of layout with ICU
and runs harfbuzz comparing it to the archived results.

But even as a manual test it will be a no-op without the right font
environment.

But I'll leave this up to Steven to decide as he provided us with these
tests.

One other issue with this test is that it uses JAXP classes which are
slated to be removed ..
Perhaps jtreg will still provide a way to access these classes on the
classpath without
test modifications  ???

-phil.

On 10/12/2017 02:22 PM, Sergey Bylokhov wrote:

> Looks fine.
> I guess we should update TestLayoutVsICU.java test which was added as
> part of harfbuzz integration. I assumed that it should compares
> behavior against icu, but it seems it is never executed.
>
> On 12/10/2017 10:45, Steven R. Loomis wrote:
>> approve
>>
>> On Wed, Oct 11, 2017 at 9:59 AM, Phil Race <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>>     Bug: https://bugs.openjdk.java.net/browse/JDK-8183978
>>     <https://bugs.openjdk.java.net/browse/JDK-8183978>
>>     Webrev: http://cr.openjdk.java.net/~prr/8183978/
>>     <http://cr.openjdk.java.net/~prr/8183978/>
>>
>>     This fix removes the obsolete ICU opentype layout code from JDK 10.
>>
>>     I've built this on all platforms (including open+closed builds).
>>
>>     -phil.
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183978 : Remove ICU layout code from OpenJDK

Steven R. Loomis
( So, Phil, TestLayoutVsICU is the test I was referring to the other week which implements a Graphics2D subclass for capturing glyphs. )

TestLayoutVsICU doesn't run against ICU dynamically, within Java or outside.   It has no dependency on 8183978 and should be left in.


What this test *does* do is to layout some text, and compare the resulting glyphs against an XML file containing the expected glyph results.  "ICU" here refers to a C++ test case in ICU which produced the original XML content.  The main problems with running this test manually are, 1.) it's font dependent— thus it is dependent on having a specific version of a font available (hence the checksums).  If the test case were include the font, now you have two problems, namely, legalities of redistributing the font.     2.) there are variations with where the glyphs are located exactly, and known differences between icu and harfbuzz as far as filler glyphs go. (Perhaps it's a job for machine learning!)

In short, it does produce useful results, but they need human checking right now. For example, it is useful to see especially if there is a difference in *which* glyphs are produced. 



On Fri, Oct 13, 2017 at 8:42 AM, Phil Race <[hidden email]> wrote:
I am not sure that the test is any less valid .. it has recorded specific results of layout with ICU
and runs harfbuzz comparing it to the archived results.

But even as a manual test it will be a no-op without the right font environment.

But I'll leave this up to Steven to decide as he provided us with these tests.

One other issue with this test is that it uses JAXP classes which are slated to be removed ..
Perhaps jtreg will still provide a way to access these classes on the classpath without
test modifications  ???

-phil.


On 10/12/2017 02:22 PM, Sergey Bylokhov wrote:
Looks fine.
I guess we should update TestLayoutVsICU.java test which was added as part of harfbuzz integration. I assumed that it should compares behavior against icu, but it seems it is never executed.

On 12/10/2017 10:45, Steven R. Loomis wrote:
approve

On Wed, Oct 11, 2017 at 9:59 AM, Phil Race <[hidden email] <mailto:[hidden email]>> wrote:

    Bug: https://bugs.openjdk.java.net/browse/JDK-8183978
    <https://bugs.openjdk.java.net/browse/JDK-8183978>
    Webrev: http://cr.openjdk.java.net/~prr/8183978/
    <http://cr.openjdk.java.net/~prr/8183978/>

    This fix removes the obsolete ICU opentype layout code from JDK 10.

    I've built this on all platforms (including open+closed builds).

    -phil.





Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8183978 : Remove ICU layout code from OpenJDK

Phil Race
In reply to this post by Phil Race
I need to correct myself here. The APIs being used in this test are not
part of the ee module
so are staying ..

-phil.

On 10/13/2017 08:42 AM, Phil Race wrote:
>
> One other issue with this test is that it uses JAXP classes which are
> slated to be removed ..
> Perhaps jtreg will still provide a way to access these classes on the
> classpath without
> test modifications  ???