Closed Bug 618870 Opened 11 years ago Closed 11 years ago
Glyph::Set Missing should not overwrite FLAG _NOT _CLUSTER _START (missing glyph hexboxes do not group as clusters)
+++ 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.
Seems like this should be all we need here. It resolves bug 619286, too.
Assignee: nobody → jfkthame
Attachment #497910 - Flags: review?(karlt)
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.
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.
Bug 619286, which is a blocking-2.0 regression, depends on this; hence requesting blocking here.
blocking2.0: --- → ?
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.
This doesn't itself block but we can approve it once it's ready.
blocking2.0: ? → -
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.
Attachment #498120 - Flags: review?(karlt) → review+
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).
Attachment #498120 - Flags: approval2.0?
Attachment #498120 - Flags: approval2.0? → approval2.0+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.