SetupClusterBoundaries in gfxPangoFonts.cpp misinterprets attrs from pango_break

RESOLVED FIXED in mozilla1.9alpha5

Status

()

Core
Graphics
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: karlt, Assigned: karlt)

Tracking

Trunk
mozilla1.9alpha5
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 years ago
Created attachment 262718 [details] [diff] [review]
patch to SetupClusterBoundaries

SetupClusterBoundaries in gfxPangoFonts.cpp expects the pango_break argument
attrs to have one PangoLogAttr per UTF8 byte but pango_break sets one
PangoLogAttr per (unicode) character.

I checked the pango_break behaviour in the source code and in its runtime output.

pango_default_break in pango-1.16.2/pango/break.c uses g_utf8_next_char to
increment the text pointer each iteration but ++ to increment the index into
attrs.  (Similarly tailor_segment uses g_utf8_strlen.)

The attached patch corrects SetupClusterBoundaries.

I haven't noticed any side effects from the misinterpretation of the data, at
least partially because Bug #332739 is not yet implemented.
Attachment #262718 - Flags: review?(roc)
(Assignee)

Comment 1

11 years ago
Created attachment 262725 [details] [diff] [review]
patch v1.1

Fixes a comment re g_utf8_strlen and makes some code fit within 80 columns.
Attachment #262718 - Attachment is obsolete: true
Attachment #262725 - Flags: review?(roc)
Attachment #262718 - Flags: review?(roc)
(In reply to comment #1)
> Created an attachment (id=262725) [details]
> patch v1.1
> 
> Fixes a comment re g_utf8_strlen and makes some code fit within 80 columns.

you can use 120 columns in thebes.
(Assignee)

Comment 3

11 years ago
Comment on attachment 262725 [details] [diff] [review]
patch v1.1

I don't yet have a CVS account, and roc is traveling so if you are willing
and able to checkin this patch, that would be much appreciated, thanks.
Attachment #262725 - Flags: superreview?(vladimir)
It doesn't need to be vlad, it could be anyone. You could ask on IRC, someone would probably be able to help. Also, put [checkin needed] on the status whiteboard, some people search for that.
Whiteboard: [checkin needed]
(Assignee)

Comment 5

11 years ago
Comment on attachment 262725 [details] [diff] [review]
patch v1.1

Thanks for [checkin needed] but don't we need an sr+?
Attachment #262725 - Flags: superreview?(roc)
patch for thebes doesn't need the sr.
(Assignee)

Updated

11 years ago
Attachment #262725 - Flags: superreview?(roc)
mozilla/gfx/thebes/src/gfxPangoFonts.cpp 	1.67
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha5
You need to log in before you can comment on or make changes to this bug.