Complex Text Line Breaking with ATSUI/Carbon for Mac

RESOLVED FIXED

Status

()

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

People

(Reporter: Theppitak Karoonboonyanan, Assigned: smontagu)

Tracking

({intl})

Trunk
PowerPC
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: intl
Version: unspecified → Trunk

Comment 1

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

Comment 2

10 years ago
It's Bug #336959 not Bug #336969.

Updated

10 years ago
Attachment #273761 - Flags: superreview?(roc)
Attachment #273761 - Flags: review?(roc)
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

Comment 4

10 years ago
Created attachment 274411 [details] [diff] [review]
A patch for using Carbon to find line break position
Attachment #273761 - Attachment is obsolete: true
Attachment #274411 - Flags: superreview?(roc)
Attachment #274411 - Flags: review?(roc)
Attachment #273761 - Flags: superreview?(roc)
Attachment #273761 - Flags: review?(roc)
+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".

Comment 6

10 years ago
Created attachment 274431 [details] [diff] [review]
A patch for using Carbon to find line break position

roc: Thank you for your comments. :-)
Attachment #274411 - Attachment is obsolete: true
Attachment #274431 - Flags: superreview?(roc)
Attachment #274431 - Flags: review?(roc)
Attachment #274411 - Flags: superreview?(roc)
Attachment #274411 - Flags: review?(roc)
Comment on attachment 274431 [details] [diff] [review]
A patch for using Carbon to find line break position

great!
Attachment #274431 - Flags: superreview?(roc)
Attachment #274431 - Flags: superreview+
Attachment #274431 - Flags: review?(roc)
Attachment #274431 - Flags: review+

Comment 8

10 years ago
the lastest patch is work fine on my mac box.
Attachment #274431 - Flags: approval1.9?

Updated

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

Comment 9

10 years ago
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.
Keywords: checkin-needed
This patch is broken by the checked-in for bug 390048, please update the Makefile.in.

Comment 11

10 years ago
Created attachment 276819 [details] [diff] [review]
 A patch for using Carbon to find line break position   

A patch for using Carbon to find line break position as Masayuki's suggestion.
Attachment #276819 - Flags: approval1.9?

Comment 12

10 years ago
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.
Attachment #276819 - Flags: approval1.9?

Updated

10 years ago
Attachment #276819 - Flags: approval1.9?

Comment 14

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

10 years ago
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.)
Attachment #276819 - Flags: approval1.9?
checked-in, thanks!
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 17

10 years ago
Verified with hourly build 2007081523 on Mac OS X 10.4
Please close this bug.
You need to log in before you can comment on or make changes to this bug.