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
s...@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
