372636 - "non-breaking hyphen" character turns into square at bmo

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[-fullmetaljacket-]

Attachment: screenshot

hypen character turns into square at bugzilla.mozilla.org specifically in the word "in-testsuite" which turns into "in‑testsuite" .

see screenshot...

WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070304 Minefield/3.0a3pre ID:2007030404 [cairo]

BROKEN
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070304 Minefield/3.0a3pre ID:2007030417 [cairo]

i will try to narrow it further but my guess is bug 370588.

Comment 1

[-fullmetaljacket-]

yes it was bug 370588

narrowed regression range:

WFM
20070304_0816_firefox-3.0a3pre.en-US.win32.zip

BROKEN
20070304_1322_firefox-3.0a3pre.en-US.win32.zip  

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1173024960&maxdate=1173043319

Comment 2

[Simon Montagu :smontagu]

The hyphen character in "in-testsuite" is U+2011 NON-BREAKING HYPHEN

Comment 3

[Justin Kerk]

On my system, it is not displayed at all (I just see "intestsuite" with no gap). The same happens for polytonic Greek, for example the omega in γλῶσσα disappears.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070304 Minefield/3.0a3pre ID:2007030417 [cairo]

Comment 4

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

Attachment: Patch rv1.0

We lost the fallback...

Comment 5

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

Comment on attachment 257336 [details] [diff] [review]
Patch rv1.0

or, roc is reviewable for this.

Comment 7

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

Comment on attachment 257336 [details] [diff] [review]
Patch rv1.0

Stuart will review this, thanks.

Comment 8

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

Is there no way to detect this from the GetCharacterPlacement results? E.g. do the missing glyphs have zero advance?

Comment 9

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

see the screenshot. the advance depends on the font.

Comment 10

[pekkle]

(In reply to comment #3)
> On my system, it is not displayed at all (I just see "intestsuite" with no
> gap). The same happens for polytonic Greek, for example the omega in
> γλῶσσα disappears.
> 
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070304
> Minefield/3.0a3pre ID:2007030417 [cairo]
> 

me too. I have 2 machines, winxp show "intestsuite" and CJK font have no problem. win2k show "in[]testsuite", and all CJK font shown as square, too.

Comment 11

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

Use nsAutoTArray instead of this malloc/free stuff. See the other uses of nsAutoTArray in gfx.

Also, let's not call IsCurrentFontHasAllGlyphs in the 8-bit case if aRun->GetFlags() & TEXT_IS_ASCII.

Comment 12

[Stuart Parmenter]

We still need to call the function even if TEXT_IS_ASCII set given that the font may not have ascii glyphs.

Comment 13

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

Alright, well, if we absolutely have to call GetGlyphIndices, maybe we should call GetTextExtentExPointI (or GetTextExtentExPoint if that's faster, although I don't why how it would be) instead of GetCharacterPlacement.

Comment 14

[Stuart Parmenter]

We should probably continue to use GetCharacterPlacement as we likely want to start using GCP_LIGATE.  We don't however want to have double allocations for the glyph arrays. i'll post a new patch shortly.

Comment 15

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

We can't use GCP_LIGATE unless you can figure out how to determine which characters correspond to which glyphs. I can't, just looking at the documentation. Also if GetTextExtentExPointI is faster than GetCharacterPlacement then I'd like to know that.We will want to know how much we're paying for ligatures.

Comment 16

[Stuart Parmenter]

Attachment: updated patch

Comment 17

[Stuart Parmenter]

roc: if you want to use GetTextExtentExPointI please write some test cases using vlad's framework and profile them.  Given that this patch brings us back to what we were doing before we know this isn't related to the perf regressions..

Comment 18

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

Comment on attachment 257448 [details] [diff] [review]
updated patch

This brings us back to the MeasureOrDrawFast path, but not the ReallyFast path. I guess it would be interesting to know what performance would have been like if we'd just disabled the ReallyFast path.

Comment 19

[Stuart Parmenter]

dig back to when it was checked in?  iirc it was 10-20%

Comment 20

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

Thank you  for the updating the patch.

We have ligature problem (bug 372732). I think that we should also fallback to Uniscribe if the |aLength| and the result of |GetGlyphIndices| are different (i.e., the text has ligature glyphs).

Comment 21

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

sorry, GetGlyphIndices doesn't support ligature, please ignore comment 20.

Comment 22

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

I post the patch for ligature problem in bug 372732. It and the patch of this bug has same code, we should merge these patches... Should we return PRBool value from |InitTextRunGDI|.

Comment 24

[-fullmetaljacket-]

marking VERIFIED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070306 Minefield/3.0a3pre ID:2007030616 [cairo]

and also 
bug 372644 -> now WFM
bug 372963 -> now WFM

Comment 25

[j.j.]

Fixing this brought back bug 371099 (as expexted)