378695 - SetupClusterBoundaries in gfxPangoFonts.cpp misinterprets attrs from pango_break

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

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

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch v1.1

Fixes a comment re g_utf8_strlen and makes some code fit within 80 columns.

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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.

Comment 3

[Karl Tomlinson (:karlt)]

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 5

[Karl Tomlinson (:karlt)]

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

Thanks for [checkin needed] but don't we need an sr+?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

patch for thebes doesn't need the sr.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

mozilla/gfx/thebes/src/gfxPangoFonts.cpp 	1.67