Complex Text Line Breaking with Uniscribe for Windows

RESOLVED FIXED

Status

()

Core
Internationalization
--
enhancement
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Vee Satayamas, Assigned: Vee Satayamas)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
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
(Assignee)

Comment 2

10 years ago
Created attachment 274365 [details] [diff] [review]
A patch for using Uniscribe to find line break position.

Comment 3

10 years ago
It's Bug #336959 insteadof Bug #336969.
Vee:

thank you for your work. you should request the review and superreview to roc.
Assignee: smontagu → vsatayamas
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: unspecified → Trunk
(Assignee)

Comment 5

10 years ago
Created attachment 274394 [details] [diff] [review]
A patch for using Uniscribe to find line break position.
Attachment #274365 - Attachment is obsolete: true
Attachment #274394 - Flags: superreview?
Attachment #274394 - Flags: review?
(Assignee)

Comment 6

10 years ago
Created attachment 274395 [details] [diff] [review]
 (optional) Check each existing attachment made obsolete by your new attachment.
Attachment #274394 - Attachment is obsolete: true
Attachment #274395 - Flags: superreview?
Attachment #274395 - Flags: review+
Attachment #274394 - Flags: superreview?
Attachment #274394 - Flags: review?
(Assignee)

Comment 7

10 years ago
Created attachment 274396 [details] [diff] [review]
A patch for using Uniscribe to find line break position
Attachment #274395 - Attachment is obsolete: true
Attachment #274396 - Flags: superreview?(roc)
Attachment #274396 - Flags: review+
Attachment #274395 - Flags: superreview?
(Assignee)

Comment 8

10 years ago
Created attachment 274398 [details] [diff] [review]
A patch for using Uniscribe to find line break position
Attachment #274396 - Attachment is obsolete: true
Attachment #274398 - Flags: superreview?(roc)
Attachment #274398 - Flags: review?(roc)
Attachment #274396 - Flags: superreview?(roc)
(Assignee)

Comment 9

10 years ago
(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.
(Assignee)

Comment 11

10 years ago
> 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.
(Assignee)

Comment 17

10 years ago
Created attachment 274447 [details] [diff] [review]
 A patch for using Uniscribe to find line break position.

This patch is made by Nattachai Ungsriwong.
Attachment #274398 - Attachment is obsolete: true
Attachment #274447 - Flags: superreview?(roc)
Attachment #274447 - Flags: review?(roc)
Attachment #274398 - Flags: superreview?(roc)
Attachment #274398 - Flags: review?(roc)
+    }
+
+  }
+  
+}

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 :-).

Comment 19

10 years ago
Created attachment 274554 [details] [diff] [review]
A patch for using Uniscribe to find line break position.

Yes, you are right. "+ startOffset" is in the wrong array. :P
Attachment #274554 - Flags: superreview?(roc)
Attachment #274554 - Flags: review?(roc)
Attachment #274554 - Flags: superreview?(roc)
Attachment #274554 - Flags: superreview+
Attachment #274554 - Flags: review?(roc)
Attachment #274554 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #274447 - Attachment is obsolete: true
Attachment #274447 - Flags: superreview?(roc)
Attachment #274447 - Flags: review?(roc)
Attachment #274554 - Flags: approval1.9?
er, what's '>' in each lines of the latest patch?

Comment 21

10 years ago
(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.

Comment 23

10 years ago
Created attachment 275528 [details] [diff] [review]
A patch for using Uniscribe to find line break position.
Attachment #275528 - Flags: approval1.9?

Updated

10 years ago
Attachment #274554 - Flags: approval1.9? → approval1.9+

Updated

10 years ago
Attachment #275528 - Flags: approval1.9? → approval1.9+
checked-in, thank you for your work.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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.
Created attachment 276823 [details] [diff] [review]
Bustage fix
Comment on attachment 276823 [details] [diff] [review]
Bustage fix

Bustage fix
Attachment #276823 - Flags: review?(benjamin)
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.,
Attachment #276823 - Flags: review?(benjamin) → review+
checked-in the additional patch, thanks!
You need to log in before you can comment on or make changes to this bug.