Closed Bug 618870 Opened 9 years ago Closed 9 years ago

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

Categories

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

All
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: karlt, Assigned: jfkthame)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

+++ 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.
This also causes the letter-spacing issue described in bug 619286.
Blocks: 619286
Seems like this should be all we need here. It resolves bug 619286, too.
Assignee: nobody → jfkthame
Attachment #497910 - Flags: review?(karlt)
Depends on: 332636
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.
Attachment #497910 - Attachment is obsolete: true
Attachment #498120 - Flags: review?(karlt)
Attachment #497910 - Flags: review?(karlt)
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?
http://hg.mozilla.org/mozilla-central/rev/d4215e592aca
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.