User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a7pre) Gecko/2007072821 Minefield/3.0a7pre Build Identifier: Trunk A similar solution to <span class="bz_closed"><a href="show_bug.cgi?id=336969" title="RESOLVED FIXED - Security error when loading a data url in a new tab">Bug #336969</a></span> and <a href="https://bugzilla.mozilla.org/show_bug.cgi?id=389520">Bug #389520</a> is needed to have proper line breaking support for complex text on Windows. Currently, some people have been working together at a Scratchpad [1] and a solution based on Uniscribe has been proposed. It should be time to bring the patch here for discussion. [1] <a href="http://scratchpad.wikia.com/wiki/Firefox_Thai">http://scratchpad.wikia.com/wiki/Firefox_Thai</a> Reproducible: Always Steps to Reproduce: 1. 2. 3.

Complex Text Line Breaking with Uniscribe for Windows A similar solution to Bug #336969 and Bug #389520 is needed to have proper line breaking support for complex text on Windows. Currently, some people have been working together at a Scratchpad [1] and a solution based on Uniscribe has been proposed. It should be time to bring the patch here for discussion. [1] http://scratchpad.wikia.com/wiki/Firefox_Thai

It's Bug #336959 insteadof Bug #336969.

Vee: thank you for your work. you should request the review and superreview to roc.

(In reply to comment #4) > Vee: > > thank you for your work. you should request the review and superreview to roc. Thank you for your suggestions. I'm sorry to do too many posts. My work is submitting the patch that has been written by the team to Bugzilla. Should I request the review and super review to roc for Carbon patch (Bug #389520) too?

(In reply to comment #9) > Should I request the review and super review to roc for Carbon patch (Bug > #389520) too? Of course, and you can update the r/sr flags from 'Details' link of the attachment (see 'Attachments' table). You don't need to post same patch.

> Of course, and you can update the r/sr flags from 'Details' link of the > attachment (see 'Attachments' table). You don't need to post same patch. Thank you so much. :-D

+ifeq ($(MOZ_WIDGET_TOOLKIT), cocoa) CPPSRCS += \ - nsRuleBreaker.cpp \ + nsCarbonBreaker.cpp \ Take this out; put the reference to nsCarbonBreaker.cpp in the Carbon patch. + PRBool itemize_fail = PR_FALSE; We generally prefer the style "itemizeFail". + if(result == E_OUTOFMEMORY) Space between "if" (or "for") and "(". Also we generally put the first "{" on the line with the "if" or "for". + } while(result == E_OUTOFMEMORY && itemize_fail == PR_FALSE); Use !itemize_fail, don't compare booleans. + for (PRUint32 i = 0; i < aLength; ++i) { + aBreakBefore[i] = PR_FALSE; + } This could be written as "memset(aBreakBefore, PR_FALSE, aLength);" Also, this code would be simpler if you just did this at the beginning. Then instead of setting your _fail variables, you can just "return" any time you detect an error. You can eliminate some braces and write it as if (...) return; + PRUint32 i,j; + for (i=start_offset, j=0; i < end_offset; ++i, ++j) Just use one variable (j) and write "aBreakBefore[j + startOffset]".

(In reply to comment #12) > Also, this code would be simpler if you just did this at the beginning. Then > instead of setting your _fail variables, you can just "return" any time you > detect an error. Note that the difference here is that it's all-or-none approach in the proposed patch, while pre-initializing and returning on error means partial returned result. I'm not sure which one is better, though. Probably, having NS_GetComplexLineBreaks() return a boolean status indicating success should clarify things? Then, the fallback PR_FALSE filling can be pushed to the caller (that is, nsJISx4051LineBreaker::GetJISx4051LineBreaks()).

The memset to PR_FALSE is so simple I don't think it's worth pushing that up to the caller. Partially initialized results are OK in this case, I think.

It probably would be a slightly good idea to return nsresult from NS_GetComplexLineBreaks, but I don't think it's all that important, at least right now.

You're right. It would appear as if there were no complex line breaker in the rest substring after the failed point. So, please go on with the review. Sorry for interruption.

This patch is made by Nattachai Ungsriwong.

+ } + + } + +} Lose the blank lines. + aBreakBefore[j] = sla[j+startOffset].fSoftBreak; Isn't "+ startOffset" in the wrong array? Almost done though! Thanks for these patches. Some time I want to find out more about your Thai open source group :-).

Yes, you are right. "+ startOffset" is in the wrong array. :P

er, what's '>' in each lines of the latest patch?

(In reply to comment #20) > er, what's '>' in each lines of the latest patch? > Sorry. It's my mistake. I forgot to remove '>' from the patch file. Do I need to upload new file ? Thanks.

please upload new file. and request approval1.9 like I did.

checked-in, thank you for your work.

Masayuki/Nattachai: Your patch broke the SeaMonkey Windows tinderbox. I think you also need to put the usp10.lib reference in intl/build/Makefile.in.

Why does it break only seamonkey??

Lightning is also broken. FF is not broken because it uses libxul, see http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/toolkit/library/libxul-rules.mk&rev=1.17&mark=74-76#70.

Comment on attachment 276823 [details] [diff] [review] Bustage fix Bustage fix

Frank: For the review term, should I backout the patch?

I think the review shouldn't take that long, so leave it in for now :).

Masayuki: Can you check this bustage fix in if it gets review in the next few hours? I cannot do it in the next few hours :)

o.k.,

checked-in the additional patch, thanks!