100868 - Add GfxMac support for getting actual string height (GetStringDimensions)

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[rbs]

The fix for bug 99010 only has a fix-up patch for the Mac. Therefore, the Mac 
will be missing the newly added dynamic font heights -- until 
nsUnicodeRenderingToolkit::GetStringDimensions() is implemented. That's the 
piece that is lacking.

LXR gives the blame to ftang for nsUnicodeRenderingToolkit::GetWidth(). I think
he will not have that much trouble hooking GetStringDimensions() once/if he sets 
out to do it. Some of the i18n people have become quite familiar with what is 
going on too.

The code for nsUnicodeRenderingToolkit::GetStringDimensions() will be a slight 
extension to nsUnicodeRenderingToolkit::GetWidth(). At the place where 
GetTextSegmentWidth() is called, the maxAscent/maxDescent will also need to be 
sync:ed. This means that the ascent/descent of the "inner" fonts used in
GetTextSegmentWidth() may have to be cached when these fonts are created. This 
is basically the scenario that is happening on other platforms.

The tracker bug for GetStringDimensions() is bug 96609.

Comment 1

[Frank Tang]

Not sure when will I have time to deal with this. we are really under staff now.
Especially Mac resource on I18N. I cannot do 3 people's job anymore. 

Comment 2

[Frank Tang]

rbs- do we need to do anything with DrawString2 ?

Comment 3

[rbs]

No, no special action is needed there (apart from the simple renaming which is
due in bug 102088).

Comment 4

[Frank Tang]

mark m0.9.7

Comment 5

[Frank Tang]

Is there good test case I can use to valid my fix ?

Comment 6

[rbs]

I used MathML fonts for my testings -- they have widely different glyphs, but
since the Mac doesn't have MathML support, the composer problem (bug 103002)
might help. Also, you can take a string 'xyz...', where 'x', 'y', 'z', ..., are
Unicode points from different fonts to trigger font-switching and see how things
go. 

Comment 7

[Frank Tang]

Attachment: patch v1

Comment 8

[Frank Tang]

Before this patch, the Indic glyph (Devanagri, etc) always got chop the top in 
the composer, after the patch, it look fine.

pierre- can you r= this one ?

Comment 9

[Frank Tang]

In additional to add the dimension, I also add the following change with the 
patch:
1. fix tab/space and other formatting issue in surronding code.
2. remove unnecessary #ifdef IBMBIDI code, basically, we cannot swith back 
without IBMBIDI now. So... remove it to make it cleaner
3. change NS_IMETHOD and NS_IMPL_IMETHOD to nsresult for non virtual function. 
We can speed up a little bit by doing so since these places do not need vtable 
at all.

Comment 10

[Simon Fraser [no longer active]]

Can we have a unified diff please? That one is hard to read.

Comment 11

[Pierre Saslawsky]

Franck, to get a unified diff on the Mac, go to "Status | Advanced Diff..." and 
select "Unified Diff".  If you changed the identations or the text aligment, 
select "Ignore White Space" too.  On other platforms, enter "cvs diff -u2" or 
"cvs diff -w -u2".

Comment 12

[Frank Tang]

Attachment: v2 (fix some tab) and cvs diff -u with space ignored.

Comment 13

[Frank Tang]

pierre- can you review it ? :)

Comment 14

[rbs]

Comment on attachment 54528 [details] [diff] [review]
v2 (fix some tab) and cvs diff -u with space ignored.

r=rbs

Nits:
+  if (NS_SUCCEEDED(rv))
+    rv = mUnicodeRenderingToolkit.GetTextDimensions(aString, aLength, aDimensions, aFontID);
 }
>>> no return rv

+nsATSUIToolkit::GetDimensions()
>>>renamed to nsATSUIToolkit::GetTextDimensions()

Comment 15

[Pierre Saslawsky]

In nsRenderingContextMac::GetWidth() and GetTextDimensions(), you don't need to 
check mGS->mFontMetrics because SetPortTextState() does it for you.

Rename GetFontInfo() into ::GetFontInfo().

I tested what I could but I'm no unicode expert.  Do you have a testcase that 
goes through a couple of fallbacks in nsUnicodeRenderingToolkit:: 
GetTextSegmentDimensions()?

Comment 16

[Frank Tang]

>I tested what I could but I'm no unicode expert.  Do you have a testcase 
> that goes through a couple of fallbacks in nsUnicodeRenderingToolkit:: 

1. install Devenagri, Korean, Hewbrew and Arabic language kits from the MacOS 9 
CD (optional install)
2. open composer, and type some english, select the Arabic or Devangri and type 
some key. And then you selec the text. In particlar for Indic script like 
Devanagri, you will see some text chop off on the top without the fix and 
selection and rendering fine after the fix. 

Also, 
http://warp/u/ftang/utf8test/ncr.cgi?out=dncr&p=0x0&r=0xa&c=0xc
should exercise some fallback too.

rbs may have some other test page. 

Comment 17

[Frank Tang]

I will prepare a new patch to address pierre and rbs comment tomorrow. thanks

Comment 18

[Frank Tang]

found some bug in the v2 patch. IME does not work well. Need to debug.

Comment 19

[Frank Tang]

ok, find the problem, should call oDim.Clear() in one place. 

Comment 20

[Frank Tang]

Attachment: v5 of the patch

Comment 21

[Frank Tang]

pierre and rbs, I address your comment in the v5 of the patch. Please r= it. The
v2 have one bug which shown up on CJK text IME with composer. I forget to add a
oDim.Clear() in the beginning of one method. 

Comment 22

[Frank Tang]

I also clean up some tab / space and format issue in v5. The v5 patch may not 
show them since it is diff with Ignore case, but when I check in the white space 
change will also get in.

Comment 23

[Pierre Saslawsky]

Comment on attachment 55125 [details] [diff] [review]
v5 of the patch

r=pierre - good catch for oDim.Clear() in GetTextSegmentDimensions

Comment 24

[Frank Tang]

sfraser- can you sr= this one?

Comment 25

[Simon Fraser [no longer active]]

Comment on attachment 55125 [details] [diff] [review]
v5 of the patch

sr=sfraser

Comment 26

[Frank Tang]

fixed and check in.

Comment 27

[Frank Tang]

fixed and check in.