implement Ideographic Variation Sequences support

RESOLVED FIXED

Status

()

enhancement
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

(Depends on 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-opera], )

Attachments

(3 attachments, 12 obsolete attachments)

6.97 KB, patch
Details | Diff | Splinter Review
6.60 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
35.92 KB, patch
Details | Diff | Splinter Review
Assignee

Description

9 years ago
Posted patch patch (obsolete) — Splinter Review
Windows 7 supports Unicode Ideographic Variation Sequences rendering. However, Firefox fails to draw Ideographic Variations even if the operating system and fonts support IVS.
This is very important for some Japanese users. Opera already support it.
Attachment #432592 - Flags: review?(jdaggett)

Updated

9 years ago
Assignee: VYV03354 → nobody
Severity: normal → enhancement
Component: Layout: Text → Graphics
OS: Windows 7 → All
QA Contact: layout.fonts-and-text → thebes
Hardware: x86 → All
Summary: Ideographic Variation Sequences support → implement Ideographic Variation Sequences support
Assignee: nobody → VYV03354
I would definitely agree that we need to support this but I think we should be
supporting this on all platforms, not just Windows 7.  Does Uniscribe lack
support for IVS?  Or only certain versions of Uniscribe?  What about CoreText
under OSX?  

Harfbuzz seems like the logical place to support this, since it's a <char
space> ==> <glyph space> mapping, along with the obvious changes needed in the
font matching code.

It would also be interesting to see if the Chrome folks have plans for this.
Assignee

Comment 2

9 years ago
(In reply to comment #1)
> but I think we should be supporting this on all platforms,
Agree, but I don't know much about other platforms. Could you file new bugs for them? Also, this patch will give a good basis for cross platform IVS support. Only platform-dependent code (except boilerplate changes) is a hack for Uniscribe.
> Does Uniscribe lack support for IVS?  Or only certain versions of Uniscribe?
Uniscribe 1.626.7600.16385 (shipped with Win7 and Office 2010) or later will support IVS. Unfortunately, Microsoft doesn't allow redistribute Uniscribe. On Vista, DirectWrite backend may resolve the problem (I've confirmed DirectWrite supports IVS on Win7, but don't test on Vista yet).
> What about CoreText under OSX?
I heard Snow Leopard supported IVS. This patch may enable it on Snow Leopard automatically. but again, I can't confirm it myself.
> along with the obvious changes needed in the font matching code.
The font matching part is covered by this patch. It is platform-independent.
IVS support will be automatically enabled when platform-dependent backend supports IVS.
Let's keep things in this bug for now, no need for other bugs just yet I think.  

There's also a couple questions of what to do in the fallback case for IVS 
sequences:

(1) Should fallback occur if a font lacks IVS support or should the underlying non-variant glyph be used?

(2) When system fallback fails for the IVS what should be displayed?  A missing glyph hexbox or the underlying non-variant character?

For (1) I would probably say 'no' but not sure about what the correct behavior should be for (2).
Assignee

Comment 4

9 years ago
> (1) Should fallback occur if a font lacks IVS support or should the
> underlying non-variant glyph be used?
Non-variant glyph will be used only when there is no font which supports the IVS  on the system. This is a more advanced feature than Opera, but I think it is mandatory because IVS will become a yet another GAIJI if users need to care about which font supports it.

> (2) When system fallback fails for the IVS what should be displayed?  A 
> missing glyph hexbox or the underlying non-variant character?
The non-variant character with a hexbox for IV selector itself will be displayed. It is intentional because I think we should indicate displayed glyph may be inaccurate.

For example, if IPAex Mincho (which supports 845B+E0100 but not 845B+E0101) is installed on the system,
845B alone => Group font or pref font will be used.
845B+E0100 => variant glyph for 845B from IPAex Mincho.
845B+E0101 => non variant glyph plus [E0101] hexbox.
Although IPAex Mincho have an appropreate default glyph for 845B+E0101, we have no way to know it mechanically. It should be considered as a defect of IPAex Mincho.

If KozMin Pr6N (which supports both 845B+E0100 and 845B+E0101 but 845B+E0101 is the same glyph as dedault) is installed,
845B+E0100 => variant glyph for 845B from KozMin Pr6N.
845B+E0101 => default glyph without hexbox.
(In reply to comment #1)
> Harfbuzz seems like the logical place to support this, since it's a <char
> space> ==> <glyph space> mapping, along with the obvious changes needed in the
> font matching code.

Harfbuzz is supposed to include support for variation selectors (although I have not actually tried it yet).
(In reply to comment #4)
> > (2) When system fallback fails for the IVS what should be displayed?  A 
> > missing glyph hexbox or the underlying non-variant character?
> The non-variant character with a hexbox for IV selector itself will be
> displayed. It is intentional because I think we should indicate displayed glyph
> may be inaccurate.

I don't think we should ever display hexboxes for variation selectors; if a particular instance of <char + VS> is not explicitly standardized in Unicode *and* supported by the rendering system (software + font) in use, the correct behavior is to display the normal glyph for the character.

See http://unicode.org/faq/unsup_char.html:

"Q: Which characters should be displayed as invisible, if not supported?
A: A: All default-ignorable characters should be rendered as completely invisible .... These include:.... variation selectors."

"Q: Does this include unsupported variation selector sequences?
A: Yes,the expected rendering behavior for the sequence of character plus a variation selector (C+VS) is specified as follows:
If C + VS is listed in StandardizedVariants.txt or in the Ideographic Variation Database and is supported by the rendering system, then display with the specified glyph.
Otherwise, display with the normal glyph for C (with no visible rendering for the VS)."
(In reply to comment #4)
> > (1) Should fallback occur if a font lacks IVS support or should the
> > underlying non-variant glyph be used?
> Non-variant glyph will be used only when there is no font which supports the
> IVS  on the system. This is a more advanced feature than Opera, but I think it
> is mandatory because IVS will become a yet another GAIJI if users need to care
> about which font supports it.

I'm not sure if I agree with this. Rendering the normal glyph and ignoring the variation selector is not at all in the same category as displaying an entirely wrong character; variation sequences are intended to express "a certain significant glyphic variation of a character", but do not change the fundamental character identity of the original form. The Unicode standard itself says (p.511) "If a user _requires_ a visual distinction between a character and a particular variant of that character, then fonts must be used to make that distinction."

In general, falling back to a different font might easily cause a much greater change to the glyph than the variation selector was supposed to indicate.

(The fact that some distinctions that may be present in the text, and may be considered important by some users, are not visible in all fonts is neither a new issue nor limited to CJK characters or variation sequences.)
Assignee

Comment 8

9 years ago
(In reply to comment #6)
At present we don't handle default ignorable code point at all. Although it had been handled in pre-cairo age (see bug 205387), it was lost along with legacy gfx.
If we start to handle default ignorable characters again, we should handle *all* default ignorable code points and they should be handled by other bugs. So I don't think we should resolve it here.
Moreover, "invisible" requirement is not absolute. Per TUS,
> This does not imply that default ignorable code points must always be
> invisible. An implementation can, for example, show a visible glyph on 
> request, such as in a “Show Hidden”mode.
At least we should have an option to render unsupported variation selectors.
(In reply to comment #7)
The greatest benefit of IVS is plain text support. If we don't handle IVS fallback, we can't make plain text IVS-aware unless all IVSes are supported in one font.
Assignee

Comment 9

9 years ago
> we should handle
> *all* default ignorable code points and they should be handled by other bugs.
I've found it was already filed as bug 546013.
Depends on: 546013

Comment 10

9 years ago
Quick note to say that the harfbuzz API supports variation selectors if the font callbacks support it (the FreeType ones do).  However, it's upon the app to do the font selection.
Assignee

Comment 11

9 years ago
Posted patch patch without fallback (obsolete) — Splinter Review
I've separated a controversial part (IVS fallback stuff) from the patch. I'll re-examine it later.
Hexbox will not be displayed anymore due to a fix for bug 546013.
This patch will enable IVS if the rendering backend supports it.
Attachment #432592 - Attachment is obsolete: true
Attachment #442687 - Flags: review?(jfkthame)
Attachment #432592 - Flags: review?(jdaggett)
Jonathan landed the fix for bug 554820 which causes the patch here to no longer apply cleanly since it changed similar code in gfxFontUtils.cpp.
Assignee

Comment 13

9 years ago
Posted patch updated to tip (obsolete) — Splinter Review
Attachment #442687 - Attachment is obsolete: true
Attachment #442896 - Flags: review?(jfkthame)
Attachment #442687 - Flags: review?(jfkthame)
Kimura-san, does Windows 7 ship with fonts that contain format-14 cmaps?  I think it would make sense to load these more lazily when they exist, rather than always reading them during the general cmap load.  Read the format-14 offset when reading in cmap's but don't read in the full table until a UVS selector was hit on a page.  The code in gfxFontEntry::TestUVSMap would check to see if the offset was set, then load the format-14 cmap at that point.

I'm not sure about default fonts but I would imagine Adobe fonts that ship with Photoshop will have these cmaps.
Comment on attachment 442896 [details] [diff] [review]
updated to tip

>         mCharacterMap(aEntry.mCharacterMap), mUserFontData(aEntry.mUserFontData),
>         mFamily(aEntry.mFamily)
>-    { }
>+    { mUVSMaps = aEntry.mUVSMaps; }
> 
>+public:

Please add mUVSMaps to the initializer list rather than the function body, for consistency. Also, no need to repeat "public:" here, these declarations are already public.


>@@ -1994,16 +2004,25 @@ gfxFontGroup::FindFontForChar(PRUint32 a
>     // use the same font as the previous range if we can
>     if (gfxFontUtils::IsJoinCauser(aCh) || gfxFontUtils::IsJoinCauser(aPrevCh)) {
>         if (aPrevMatchedFont && aPrevMatchedFont->HasCharacter(aCh)) {
>             selectedFont = aPrevMatchedFont;
>             return selectedFont.forget();
>         }
>     }
> 
>+    // if this character is a variation selector,
>+    // use the same font as the previous range if we can
>+    if (gfxFontUtils::IsVarSelector(aCh)) {
>+        if (aPrevMatchedFont && aPrevMatchedFont->HasUVS(aPrevCh, aCh)) {
>+            selectedFont = aPrevMatchedFont;
>+            return selectedFont.forget();
>+        }
>+    }

In addition to testing for the specific UVS sequence using HasUVS, we should check whether the VS codepoint is directly supported by the standard cmap table. It is possible for a TrueType-based OT font, at least, to support variation sequences using standard OT mechanisms (e.g, in the ccmp feature) without needing a format-14 cmap subtable. So I'd suggest something like

+        if (aPrevMatchedFont &&
+            (aPrevMatchedFont->HasCharacter(aCh) ||
+             aPrevMatchedFont->HasUVS(aPrevCh, aCh))) {

as the test here.

Also, it cannot ever be useful to assign different fonts to the base character and the VS codepoint, as this causes a break in the font runs and therefore in shaping, and there is no possibility of the sequence actually being handled. Therefore, if aPrevMatchedFont does not support the UVS, I think we should skip the following font searches and just return null, leaving the default-ignorable code to handle it.

As John suggested, I think we should defer the actual loading of the format-14 table until it is actually needed in TestUVSMap().
Attachment #442896 - Flags: review?(jfkthame) → review-
Assignee

Comment 16

9 years ago
(In reply to comment #15)
> Please add mUVSMaps to the initializer list rather than the function body, for
> consistency. Also, no need to repeat "public:" here, these declarations are
> already public.
If I moves mUVSMaps to the initializer list, compilation fails with the following error:
| ../../../dist/include\gfxFont.h(188) : error C2558: class 'nsAutoPtr<T>' : no 
| copy constructor available or copy constructor is declared 'explicit'
|        with
|        [
|            T=nsTHashtable<gfxUVSHashEntry>
|        ]
How to fix this?
Assignee

Comment 17

9 years ago
(In reply to comment #14)
Windows 7 ships with Cambria Math which contains format-14 cmaps for some mathematical symbols.
It seems like the code was wrong in the old case, though:  shouldn't the copy-constructor copy rather than destroy the thing it's supposed to be copying from?  It seems that that requires cloning the hash table rather than stealing it from aEntry.
In gfxFontUtils::ReadCMAPTableFormat14:

+        SizeOfNonDefUVSTable = 5,
+        NonDefUVSOffsetUnicodeValue = 0,
+        NonDefUVSOffsetGlyphID = 3,
+    };

Trim the trailing comma.
Assignee

Comment 20

9 years ago
Posted patch v3 (obsolete) — Splinter Review
Changes from the previous patch:
* Resolved review comments.
* Added a support for older versions of Uniscribe.
Attachment #442896 - Attachment is obsolete: true
Attachment #445142 - Flags: review?(jfkthame)
Assignee

Comment 21

9 years ago
Posted patch Test (obsolete) — Splinter Review
I've enabled a test on Windows because this patch will work with all supported versions of Windows now.
Assignee

Comment 22

9 years ago
Posted patch v4 (obsolete) — Splinter Review
Treat variation sequences as atomic units. This will improve the behavior of selection and caret move.
I had to remove the HasCharacter() && HasUVS() check to prevent the text run divided.
Attachment #445142 - Attachment is obsolete: true
Attachment #445395 - Flags: review?(jfkthame)
Attachment #445142 - Flags: review?(jfkthame)
Assignee

Comment 23

9 years ago
Posted patch TestSplinter Review
Changed licesnse text's encding to UTF-8. Also added a Japanese original text.
Attachment #445143 - Attachment is obsolete: true
Assignee

Comment 24

9 years ago
Posted patch v4 (intl)Splinter Review
nsIUGenCategory didn't support plane 14 characters.
Attachment #445397 - Flags: review?(smontagu)
Attachment #445397 - Flags: review?(smontagu) → review+
Assignee

Comment 25

9 years ago
Posted patch updated to tip (obsolete) — Splinter Review
Attachment #445395 - Attachment is obsolete: true
Attachment #445602 - Flags: review?(jfkthame)
Attachment #445395 - Flags: review?(jfkthame)
Comment on attachment 445602 [details] [diff] [review]
updated to tip

Some low-level comments below, but as I think about this code I also have a more general comment: I'm wondering whether the overhead (in code complexity and memory use) of building the "hashtable of hashtables" here is worthwhile.

Looking at the definition of the Format 14 subtable, I think it would be feasible to simply cache the raw subtable data (perhaps after validating the various offsets etc for consistency), and look up variation sequences directly in the Variation Selector records and the Default and Non-Default lists. All of these lists will normally be fairly short, and they are all ordered, so binary searches would yield the final result in just a few steps.

I suspect the performance would not be measurably different, especially given that these will be fairly low-frequency characters, and the memory footprint would be smaller than creating the multiple hashtables. What do you think about this option?

Meanwhile, comments on the current patch:

+        if (aEntry.mUVSMaps) 
+            mUVSMaps = aEntry.mUVSMaps->Clone();

Nit: style guide calls for braces everywhere.
 
+        if (mUVSMaps && (entry = mUVSMaps->GetEntry(aVS)) && entry->map.test(aCh))
+            return PR_TRUE;

Same again.

As far as I can see, gfxFont::HasUVS() is not used anywhere, so could be removed. Then gfxFontEntry::HasUVS() and gfxFontEntry::TestUVSMap() can also be trimmed out.

+    virtual nsresult InitializeUVSMap();
+    virtual PRUint16 GetUVSGlyph(PRUint32 aCh, PRUint32 aVS);
+    virtual PRBool TestUVSMap(PRUint32 aCh, PRUint32 aVS);

Any reason for these to be virtual? I'd prefer to make them non-virtual unless we see a specific reason to want to override them; at the moment, I don't think that's likely.

(If anything, I'd be more inclined to make the gfxFont methods virtual, so that a platform could reimplement them at that level if there are native APIs that provide more efficient lookup. But for the time being, I don't think we need to bother with it at all.)

+    void CopyFrom(const gfxSparseBitSet& aBitset) {
+        reset();
+        PRUint32 len = aBitset.mBlocks.Length();
+        if (len > mBlocks.Length())
+            mBlocks.AppendElements(len - mBlocks.Length());

What about if len < mBlocks.Length(), should we try to release memory?

+        for (PRUint32 i = 0; i < len; ++i) {
+            Block *block = aBitset.mBlocks[i];
+            if (block)
+                mBlocks[i] = new Block(*block);
+        }
+    }

Braces for the body of the if() statements. (I know the surrounding code doesn't do this consistently, but I think we should try to follow the style rules in new methods, at least.)

+class gfxUVSGlyphs : public nsDataHashtable<nsUint32HashKey, PRUint16> {
+public:
+
+    void CopyFrom(const gfxUVSGlyphs& aGlyphs) {
+        this->Init();

I think it would make sense to pass aGlyphs.Count() to the Init() method, so that the appropriate amount of space is allocated.

+        const_cast<gfxUVSGlyphs&>(aGlyphs).EnumerateEntries(CopyEntry, this);
+    }

Can we avoid the const_cast<> here, by using the EnumerateRead method (defined on nsBaseHashtable)? That would seem cleaner (even though it ends up doing its own const_cast<> internally!)

+    static PLDHashOperator CopyEntry(gfxUVSGlyphs::EntryType *entry, void *userData) {

Line length > 80.

+        if (toCopy.glyphs.IsInitialized())
+            glyphs.CopyFrom(toCopy.glyphs);

Braces. There are a bunch more occurrences of this; I'm not going to annotate them one by one.


+    static inline bool IsVarSelector(PRUint32 ch) {
+        return ch >= 0xFE00 && ch <= 0xFE0F ||
+               ch >= 0xE0100 && ch <= 0xE01EF;
+    }

It would be nice to have named constants here.

 
+        mUVSMaps = new gfxUVSHashtable;
+        NS_ENSURE_TRUE(mUVSMaps, PR_FALSE);

"new" should be infallible so this check is redundant. But also, PR_FALSE is not an appropriate nsresult return code, I think; should be NS_ERROR_OUT_OF_MEMORY.

+        gfxFontUtils::ReadCMAPTableFormat14(buffer.Elements() + mUVSOffset,
+                                            buffer.Length() - mUVSOffset,
+                                            *mUVSMaps);

Should check the result of ReadCMAPTableFormat14 in case it detects a malformed table; if so, I think we should discard it entirely, not use whatever entries may have been put into mUVSMaps by the time the error is detected.

+        if (i+1 < numItems && aRunStart + iCharPosNext <= aRunLength - 2
+            && aString[aRunStart + iCharPosNext] == 0xDB40
+            && PRUint32(aString[aRunStart + iCharPosNext + 1]) - 0xDD00 < 240) 

Please use named constants and/or comment this to explain what the magic numbers mean.
Attachment #445602 - Flags: review?(jfkthame) → review-
Assignee

Comment 27

9 years ago
Posted patch v5 (obsolete) — Splinter Review
> Some low-level comments below, but as I think about this code I also have a
> more general comment: I'm wondering whether the overhead (in code complexity
> and memory use) of building the "hashtable of hashtables" here is worthwhile.
> 
> Looking at the definition of the Format 14 subtable, I think it would be
> feasible to simply cache the raw subtable data (perhaps after validating the
> various offsets etc for consistency), and look up variation sequences directly
> in the Variation Selector records and the Default and Non-Default lists. All of
> these lists will normally be fairly short, and they are all ordered, so binary
> searches would yield the final result in just a few steps.
> 
> I suspect the performance would not be measurably different, especially given
> that these will be fairly low-frequency characters, and the memory footprint
> would be smaller than creating the multiple hashtables. What do you think about
> this option?
It sounds reasonable. Changed "hashtable of hashtables" to raw cmap data.

> +    virtual nsresult InitializeUVSMap();
> +    virtual PRUint16 GetUVSGlyph(PRUint32 aCh, PRUint32 aVS);
> +    virtual PRBool TestUVSMap(PRUint32 aCh, PRUint32 aVS);
>
> Any reason for these to be virtual? I'd prefer to make them non-virtual unless
> we see a specific reason to want to override them; at the moment, I don't think
> that's likely.
I had considered implementing some GDIShaper-optimized methods.
They don't have to be virtual in the current patch, so removed.

> +    void CopyFrom(const gfxSparseBitSet& aBitset) {
> +        reset();
> +        PRUint32 len = aBitset.mBlocks.Length();
> +        if (len > mBlocks.Length())
> +            mBlocks.AppendElements(len - mBlocks.Length());
>
> What about if len < mBlocks.Length(), should we try to release memory?
This method is completely removed because it was required only for HasUVS()
which was also removed.

Fixed All other style nits.

Also fixed potential integer overflow in ReadCMAPTableFormat14.
Attachment #445602 - Attachment is obsolete: true
Attachment #447595 - Flags: review?(jfkthame)
Assignee

Comment 28

9 years ago
I found a bonehead error in copy constructor of gfxFontEntry (forgetting to call new!). However, no one seems to call the copy constructor in the first place. Anyway, the existing code didn't handle copying mUserData correctly.
I've made gfxFontEntry non-copyable.
Attachment #447595 - Attachment is obsolete: true
Attachment #447610 - Flags: review?(jfkthame)
Attachment #447595 - Flags: review?(jfkthame)
Assignee

Comment 29

9 years ago
Posted patch v5.2 (obsolete) — Splinter Review
Ah, gfxFT2Fonts.cpp wouldn't compile.
Attachment #447610 - Attachment is obsolete: true
Attachment #447631 - Flags: review?(jfkthame)
Attachment #447610 - Flags: review?(jfkthame)
From gfxFontUtils::MapUVSToGlyphFormat14()....

+    // binary search in uvsMappings
+    min = 0;
+    max = table->numUVSMappings;
+    while (min < max) {
+        PRUint32 index = (min + max) >> 1;
+        PRUint32 unicodeValue = table->uvsMappings[index].unicodeValue;
+        if (aCh == unicodeValue) {
+            return table->uvsMappings[index].glyphID;
+        }
+        if (aCh < unicodeValue) {
+            max = index;
+        } else {
+            min = index + 1;
+        }
+    }
+}

Doesn't this function need "return 0;" at the end, in case aCh was not actually present in the uvsMappings?
Assignee

Comment 31

9 years ago
Posted patch v5.3 (obsolete) — Splinter Review
> Doesn't this function need "return 0;" at the end, in case aCh was not actually
> present in the uvsMappings?
Oops, you're right. Fixed.
Attachment #447631 - Attachment is obsolete: true
Attachment #448186 - Flags: review?(jfkthame)
Attachment #447631 - Flags: review?(jfkthame)
Comment on attachment 448186 [details] [diff] [review]
v5.3

Looks good to me, thanks.

Have you tried this together with the reftest on tryserver? I think we should include a test, but I'm wondering how well it's supported on the other platforms. In local testing on OS X, I see the correct glyph being chosen but the positioning is slightly different between the testcase and reference, so I'm concerned it'll fail on tinderbox too. (May be a Core Text problem rather than ours, but we'll need to investigate and possibly file a separate bug.) I haven't tested on Linux yet.
Attachment #448186 - Flags: review?(jfkthame) → review+
Assignee

Comment 33

9 years ago
> In local testing on OS X, I see the correct glyph being chosen but
> the positioning is slightly different between the testcase and reference, so
> I'm concerned it'll fail on tinderbox too.
I don't enable reftest on Mac OS X yet because 10.5 doesn't support IVS glyph substitution at all.
> +skip-if(MOZ_WIDGET_TOOLKIT!="windows") HTTP(..) == ivs-1.html ivs-1-ref.html
Unfortunately I have no knowledge to add a support for other platforms.
Is it possible to land a reftest which is enabled only on Windows (as the current patch does), then file a followup bug for other platforms?
(In reply to comment #33)
> I don't enable reftest on Mac OS X yet because 10.5 doesn't support IVS glyph
> substitution at all.

Hmm, that's a pity. It works on 10.6, except for the possible positioning issue - and I've also seen a crash from within Core Text while processing the format 14 subtable. But that's not a problem with this patch, it's an Apple bug.

> Is it possible to land a reftest which is enabled only on Windows (as the
> current patch does), then file a followup bug for other platforms?

Yes, let's go ahead and do that. (Do you know if this is supported in Pango? But in any case, I think it's fine to land this and file followups.)

Before landing, though, a few more issues - the first one is important as it can give a compile error:

+    enum {
+        kUnicodeVS1 = 0xFE00,
+        kUnicodeVS16 = 0xFE0F,
+        kUnicodeVS17 = 0xE0100,
+        kUnicodeVS256 = 0xE01EF,
+    };

The last (unneeded) comma here causes a build failure on Linux (on tryserver), so please remove it.

And a couple of minor cleanup issues:

+    static inline bool IsVarSelector(PRUint32 ch) {
+        return ch >= kUnicodeVS1 && ch <= kUnicodeVS16 ||
+               ch >= kUnicodeVS17 && ch <= kUnicodeVS256;
+    }

Please add parentheses around the two && clauses, to avoid a warning from gcc.

-        mFamily(aEntry.mFamily)
+        mFamily(aFamily),
+        mUVSOffset(0), mUVSData(nsnull)

-        mFamily(nsnull)
+        mFamily(nsnull),
+        mUVSOffset(0), mUVSData(nsnull)

Please insert the new initializers before mFamily instead of after it, to avoid a warning about initialization order. (Order of initializers should match order of class members.)
Assignee

Comment 35

9 years ago
Posted patch v5.4 (for checkin) (obsolete) — Splinter Review
Resolved review comments

> Please insert the new initializers before mFamily instead of after it, to avoid
a warning about initialization order. (Order of initializers should match order
of class members.)
I moved them before mUserFontData to match order of class members.

Please land if there is no more problem (I have no commit priv).
Attachment #448186 - Attachment is obsolete: true
Assignee

Comment 36

9 years ago
Sorry, wrong file attached.
Attachment #448369 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/e481675132db (reftest)
http://hg.mozilla.org/mozilla-central/rev/d625ee923468 (unicode data in intl/)
http://hg.mozilla.org/mozilla-central/rev/a82a0bf474e8 (font support in gfx/)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Filed bug 569350 as followup re other platforms.
Shouldn't we write a document for MDC?
Depends on: 571160
Assignee

Updated

9 years ago
Depends on: 571372
Assignee

Updated

8 years ago
Depends on: 392588
For some reason, bug 854169 (not landed yet) seems to break this by causing a
REFTEST TEST-UNEXPECTED-PASS | http://10.250.49.156:30051/tests/layout/reftests/font-face/ivs-1.html | image comparison (==)
on Android (and, apparently, only on Android).

If anybody Cc-ed on this bug has an idea, I'm interested.
Flags: needinfo?
Just mark the test as passing and move on is my suggestion :-)
Flags: needinfo?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Just mark the test as passing and move on is my suggestion :-)

Is that serious?
We should at least look at the rendering of the test; if (for example) it's "passing" because it fails to render at all, or just renders .notdef boxes, that wouldn't be good.

Unfortunately, TBPL results from when bug 854169 landed (temporarily) on fx-team don't seem to be available, so I can't look in the logs from there.
I can do that. How do I check the rendering exactly? Should I just run the test?
If you run the test, and get an UNEXPECTED-PASS, I think it should record a screenshot in the reftest log as a data URL; so then you can load that in a browser to see how it actually rendered.
You need to log in before you can comment on or make changes to this bug.