[OpenJDK 2D-Dev] Add optional support for using the system libicu

[Steven R. Loomis]

Omair,
  I didn't hear back from you on this list, but I will reply here with 
more details.   See diffs below.  I would be very suspicious of how this 
could run without crashing, if the tests I mentioned were run (anything 
with 'kerning' in its name, using a font that actually uses kerning pairs).

JDK pre-swaps the kerning pairs, but ICU re-swaps them every time a pair 
is hit.

getKerningAdjustment is a hook that has to do with JDK's transform 
matrix. It could probably be implemented as a no-op 
getKerningAdjustment(LEPoint &adjustment)const{}; in the base class.

Steven

----
* < ICU (trunk) LEFontInstance.h
* > JDK (2d/8) LEFontInstance.h

159,162d183
<      * Note that implementing this function does not allow for range 
checking.
<      * Subclasses that desire the safety of range checking must 
implement the
<      * variation which has a length parameter.
<      *
182,183d202
<      * Note that range checking can only be accomplished if this 
function is
<      * implemented in subclasses.
192a212,214
 >     virtual void *getKernPairs() const = 0;
 >     virtual void  setKernPairs(void *pairs) const = 0;
 >
306a329,330
 >     virtual void getKerningAdjustment(LEPoint &adjustment) const = 0;




On 6/24/13 5:24 p.m., Steven R. Loomis wrote:
> Omair,
>  I'm sorry I did not see this before. I probably would not have 
> noticed it except I happened to notice this reply.
>
>  I'm responsible for maintaining the version of ICU in JDK, and also 
> (read: main day job) IBM's lead for ICU for C/C++.  ( But I speak for 
> myself here, of course. )
>
>  I think this patch is certainly a good direction (especially with 
> regards HarfBuzz - I wrote that recommendation you cite), at least 
> longer term, however, I have quite a number of concerns with this patch.
>
> • The big one - is that the ICU and JDK versions of the LE are 
> divergent, so much so that, to put it mildly, I am a little surprised 
> that this patch builds at all.
>  Now, I would like - love - for enough upstream changes to ICU to 
> happen so that the APIs are compatible enough for this change to be 
> possible.  But we aren't there yet.
>   • … and when we are there, it won't be compatible with any shipping 
> ICU, so you will need to wait until the future ICU is picked up.
>
> • You mentioned the issues about which version of the headers to 
> compile against. You MUST compile against the same version of the 
> headers you are linking against.  Period. This is C++, not Java or 
> even C.  The vtable expected in the ICU version versus the JDK version 
> of the LE are different, have different parentage.
>
> • As to tests, I would start with everything in 
> jdk/test/java/awt/font/TextLayout
>
>  ( NB Phil-  I had started a port of ICU's letest.xml conformance test 
> to the JDK- this would be helpful in making sure we get the same 
> results.)
>
>
> I'm sorry I didn't see this earlier. I don't always monitor this 
> address or even 2d-dev as closely as I could.
>
> Regards,
> Steven
>
> ( If you don't get a response from me, you can always ping me at 
> srl at icu-project.org …  )
>
> On 6/24/13 3:14 p.m., Omair Majid wrote:
>> Hi Phil,
>>
>> Updated webrev:
>> http://cr.openjdk.java.net/~omajid/webrevs/system-icu/01/
>>
>> It's still against jdk8/build and missing support for the old build 
>> system.
>>
>> On 06/05/2013 02:02 PM, Phil Race wrote:
>>> Since this entirely affects a 2D component, please include 2d-dev in
>>> this discussion.
>>> I would have been 'surprised' to see this change if I hadn't just
>>> spotted this thread.
>> Mea culpa. I pushed a few similar system-library patches using this
>> identical process and no one corrected me. So I assumed this was right.
>> For the future, I will ensure the relevant groups are CC'd.
>>
>>> And I believe this change should be integrated via the 2D forest.
>> Okay. Are there any guidelines for this? It's not obvious to me when a
>> change is more appropriate for build vs 2D.
>>
>>> I am not sure what, if any, runtime problems might occur from using a
>>> different
>>> version. We've generally been able to swap in newer versions of ICU 
>>> without
>>> much trouble, but I recommend to run appropriate tests before 
>>> shipping ..
>> Thanks for the suggestions. Do you have any particular tests in mind?
>>
>> For some background, the goal with this change is two fold:
>>
>> 1. Gain benefits from system-installed libraries. This is one library
>> where OpenJDK does not lag behind in security updates, but in some other
>> cases, embedded libraries can lag behind. It also makes it easier to use
>> the distributions preferred policies (hardening flags on libraries and
>> so on).
>>
>> 2. Make it easier to switch to HarfBuzz at some later point. ICU itself
>> strongly recommends switching [1]. Through ice-le-hb [2], only a
>> recompile should be needed to switch (hopefully).
>>
>>> Note that JDK8 in fact has a very "current" ICU with some security 
>>> fixes.
>>> So I would not recommend using the native lib on a system with an older
>>> ICU.
>> Thanks for pointing it out. I see that ICU recently released a security
>> update [1] too, but probably many distributions will not have this
>> up-to-date version (my current distro, a little out-of-date, does not 
>> :( ).
>>
>> Thanks,
>> Omair
>>
>> [1] http://site.icu-project.org/download/51
>> [2] https://github.com/behdad/icu-le-hb
>>
>