618870 - CompressedGlyph::SetMissing should not overwrite FLAG_NOT_CLUSTER_START (missing glyph hexboxes do not group as clusters)

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #617869 +++

Even when glyphs are not available, clusters are useful for text selection.
For example selecting only a combining character and pasting elsewhere may
give unexpected results.

This is also causing layout/reftests/first-letter/329069-2.html to fail on
WINNT 5.2.  gfxPlatform::SetupClusterBoundaries has set the cluster boundaries
correctly but CompressedGlyph::SetMissing undoes that work.

Comment 1

[Jonathan Kew [:jfkthame]]

This also causes the letter-spacing issue described in bug 619286.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: patch, don't mess with the cluster-start flag in CompressedGlyph::SetMissing

Seems like this should be all we need here. It resolves bug 619286, too.

Comment 3

[Jonathan Kew [:jfkthame]]

This causes a mochitest failure on tryserver because of bug 332636, which I've just re-opened: the patch there was not really correct, but SetMissing was hiding the problem on our test boxes by killing the cluster information; if there had been font coverage for the test case, it would have been failing all along.

So to land this, we'll need to re-fix bug 332636 or else disable the test for now.

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: patch, v2 - updated to fix warnings

Turns out that when we fix SetMissing() to leave the cluster-start flag alone, we also need to update CopyGlyphDataFrom() so that it properly clears the glyph data when it is "stealing". Otherwise we end up getting lots of spurious warnings about font runs starting/ending in the middle of a cluster while copying words from a textrun that is being gradually cannibalized.

Comment 5

[Jonathan Kew [:jfkthame]]

Bug 619286, which is a blocking-2.0 regression, depends on this; hence requesting blocking here.

Comment 6

[Jonathan Kew [:jfkthame]]

And once we land both bug 615445 and this patch, we can also mark the reftests first-letter/329069-2.html and first-letter/329069-3.html as passing on all platforms.

Comment 7

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

This doesn't itself block but we can approve it once it's ready.

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 498120 [details] [diff] [review]
patch, v2 - updated to fix warnings

>-            aSource->mCharacterGlyphs[i + aStart].SetMissing(0);
>+            CompressedGlyph gg;
>+            aSource->mCharacterGlyphs[i + aStart] = gg.SetMissing(0);

How about "aSource->mCharacterGlyphs[i + aStart] = CompressedGlyph()" as the default initialization already makes gg "missing".

r+ if you are happy with that.

Comment 9

[Jonathan Kew [:jfkthame]]

Comment on attachment 498120 [details] [diff] [review]
patch, v2 - updated to fix warnings

(In reply to comment #8)
> How about "aSource->mCharacterGlyphs[i + aStart] = CompressedGlyph()" as the
> default initialization already makes gg "missing".
> 
> r+ if you are happy with that.

Yup, sounds good to me, thanks.

Requesting approval-2.0. This is needed to fix bug 619286 (which is marked blocking-2.0:final).

Comment 10

[Jonathan Kew [:jfkthame]]

http://hg.mozilla.org/mozilla-central/rev/d4215e592aca