User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en; rv:1.8.1.5) Gecko/20070712 (Debian-1.8.1.5-1thai1) Epiphany/2.18 Build Identifier: A similar solution to Bug #336969 is needed to have proper line breaking support for complex text on Mac. Currently, some people have been working together at a Scratchpad [1] and a solution based on Carbon API has been proposed. It should be time to bring the patch here for discussion. [1] http://scratchpad.wikia.com/wiki/Firefox_Thai Reproducible: Always

It's Bug #336959 not Bug #336969.

Similar comments apply to the ones I just gave about the Uniscribe patch. In particular, + for(UniCharArrayOffset i = 0; i < aLength; ++i) + { + aBreakBefore[i] = PR_FALSE; + } Use memset + if(status == noErr) + { Fix space after if. Also there's less indenting if you write it as if (status != noErr) return; + for(position = 0; position < aLength;) Declare 'position' here. + (const UniChar *)aText, Is this needed? I suspect we might compile without this. If it is needed, use reinterpret_cast. + if(status != noErr) + { + break; + } Drop the braces

+ifeq ($(MOZ_WIDGET_TOOLKIT),windows) +CPPSRCS += \ + nsUniscribeBreaker.cpp \ + $(NULL) +else Now you've dragged this in from your other patch. Please keep them separate. + OSStatus status = noErr; Declare this where you first set it. You don't need this line. + if(status != noErr) Space after "if". + UniCharArrayOffset offset; Make this local to the loop body. + for(UniCharArrayOffset position = 0; position < aLength;) Space after "for". + for(UniCharArrayOffset position = 0; position < aLength;) + { Brace on the line with the "for". + if(status != noErr) Space after "if".

roc: Thank you for your comments. :-)

Comment on attachment 274431 [details] [diff] [review] A patch for using Carbon to find line break position great!

the lastest patch is work fine on my mac box.

Note that patches don't get checked in of their own accord. In the future, please mark bugs with the checkin-needed keyword so patches don't get lost.

This patch is broken by the checked-in for bug 390048, please update the Makefile.in.

A patch for using Carbon to find line break position as Masayuki's suggestion.

Oops, i did something wrong?

Comment on attachment 276819 [details] [diff] [review] A patch for using Carbon to find line break position a1.9 should be carried over from previous patch, because this is not changing the actual behavior/logic.

(In reply to comment #13) > (From update of attachment 276819 [details] [diff] [review]) > a1.9 should be carried over from previous patch, because this is not changing > the actual behavior/logic. > I see, thanks for your suggestion.

Comment on attachment 276819 [details] [diff] [review] A patch for using Carbon to find line break position I think you missed the point... you don't need to re-request approval because there are no changes, so just don't set the flag. (There are enough approval requests in the queue already.)

checked-in, thanks!

Verified with hourly build 2007081523 on Mac OS X 10.4 Please close this bug.