Parser for new fonts.xml format

src/ports/SkFontConfigParser_android.cpp

Index: src/ports/SkFontConfigParser_android.cpp
diff --git a/src/ports/SkFontConfigParser_android.cpp b/src/ports/SkFontConfigParser_android.cpp
index fbbb6a627a2b21f62d52abebaca152fd8fd7875d..d1169b99fb60759b5ecfedf8e224f2085fbaea82 100644
--- a/src/ports/SkFontConfigParser_android.cpp
+++ b/src/ports/SkFontConfigParser_android.cpp
@@ -77,15 +77,190 @@ template <typename T> static bool parseNonNegativeInteger(const char* s, T* valu
 
 namespace lmpParser {
 
-void startElementHandler(void* data, const char* tag,
-                                   const char** attributes) {
-    //SkDebugf("lmp started %s", tag);
+void familyElementHandler(FontFamily* family, const char** attributes) {
+    // A non-fallback <family> tag must have a canonical name attribute.
+    // A (fallback) <family> tag may have lang and variant attributes.
+    for (int i = 0; attributes[i] != NULL; i += 2) {

[palmer, 2014/08/06 21:05:30]

Is there a guarantee that |attributes| always has an even # of elements? i += 2
could cause this loop to skip past the sentinel and keep looping until a crash.

(Also, size_t is the proper size for object counters, lengths, and indices.)

[reed1, 2014/08/06 21:20:05]

Skia uses int instead of size_t for most counters and indices. 

[tomhudson, 2014/08/07 14:39:42]

We expect that the XML Parser (expat) guarantees that for us; if not, it
probably needs a security review?

From
skia/platform_tools/android/third_party/externals/expat/doc/reference.html#XML_SetStartElementHandler:

Set handler for start (and empty) tags. Attributes are passed to the start
handler as a pointer to a vector of char pointers. Each attribute seen in a
start (or empty) tag occupies 2 consecutive places in this vector: the attribute
name followed by the attribute value. These pairs are terminated by a null
pointer.

Added an extra NULL-check because we can afford it, but it's pretty ugly - if
you're happy with the expat contract, let's pull it back out.
UNDone; see http://skbug.com/2811.

+        const char* name = attributes[i];
+        const char* value = attributes[i+1];

[reed1, 2014/08/06 21:20:05]

could assert that value != NULL, to address the concern about skipping past the
sentinel.

+        int nameLen = strlen(name);

[palmer, 2014/08/06 21:05:29]

strlen returns size_t, not int. (The 2 types differ in signedness and possibly
in width.)

[reed1, 2014/08/06 21:20:05]

+1

[tomhudson, 2014/08/07 14:39:43]

Done ~ 16x.
(Old code.)

There are many other instances of this in Skia, although only one looks to be in
the library code (others are in tests); there also seem to be some instances in
our third-party code. Filed http://skbug.com/2810.

+        int valueLen = strlen(value);
+        if (nameLen == 4 && !strncmp("name", name, nameLen)) {
+            family->fNames.push_back().set(value);
+        } else if (nameLen == 4 && !strncmp("lang", name, nameLen)) {
+            family->fLanguage = SkLanguage (value);
+        } else if (nameLen == 7 && !strncmp("variant", name, nameLen)) {
+            // Value should be either elegant or compact.
+            if (valueLen == 7 && !strncmp("elegant", value, valueLen)) {
+                family->fVariant = SkPaintOptionsAndroid::kElegant_Variant;
+            } else if (valueLen == 7 && !strncmp("compact", value, valueLen)) {
+                family->fVariant = SkPaintOptionsAndroid::kCompact_Variant;
+            }
+        }
+    }
 }
 
-void endElementHandler(void* data, const char* tag) {
+void fontFileNameHandler(void *data, const char *s, int len) {

[palmer, 2014/08/06 21:05:30]

Nit: "void *data" conflicts with the "FontFamily* family" style above. I don't
know which is standard for Skia code, but it should be consistent.

|len| should be a size_t, as usual.

[tomhudson, 2014/08/07 14:39:42]

Done. Unfortunately,
https://sites.google.com/site/skiadocs/developer-documentation/contributing-c...
implies the former style, but isn't explicit. It looks like the first dozen
public headers are also consistent with the former style.
Unfortunately, expat is third-party and specifies an int there:

typedef void
(XMLCALL *XML_CharacterDataHandler)(void *userData,
                                    const XML_Char *s,
                                    int len);

+    FamilyData *familyData = (FamilyData*) data;

[palmer, 2014/08/06 21:05:30]

Prefer C++ style cast?

+    familyData->currentFontInfo->fFileName.set(s, len);

[palmer, 2014/08/06 21:05:30]

If |set| is defined to take an int length argument, that's a bug.

[reed1, 2014/08/06 21:20:05]

+1

[tomhudson, 2014/08/07 14:39:43]

SkString::set() takes size_t, but is happy to implicitly cast an int.

+}
 
-    //SkDebugf("lmp ended %s", tag);
+void familyElementEndHandler(FontFamily* family) {
+    for (int i = 0; i < family->fFontFiles.count(); i++) {

[palmer, 2014/08/06 21:05:29]

size_t, and if |count| returns int, that's a bug

[reed1, 2014/08/06 21:20:05]

count() returns an int.

[tomhudson, 2014/08/07 14:39:42]

Acknowledged. Filed http://skbug.com/2811. Since we warn-as-error on type
mismatch, this seems to be best done as a single huge change all over the
codebase.

+        family->fFontFiles[i].fPaintOptions.setLanguage(family->fLanguage);
+        family->fFontFiles[i].fPaintOptions.setFontVariant(family->fVariant);
+    }
+}
+
+void fontElementHandler(XML_Parser* parser, FontFileInfo* file, const char** attributes) {
+    // A <font> should have weight (integer) and style (normal, italic) attributes.
+    // NOTE: we ignore the style.
+    // The element should contain a filename.
+    for (int i = 0; attributes[i] != NULL; i += 2) {

[palmer, 2014/08/06 21:05:29]

Same int and += 2 comments as above

[tomhudson, 2014/08/07 14:39:43]

Done.

+        const char* name = attributes[i];
+        const char* value = attributes[i+1];
+        int nameLen = strlen(name);
+        if (nameLen == 6 && !strncmp("weight", name, nameLen)) {
+            parseNonNegativeInteger(value, &file->fWeight);

[palmer, 2014/08/06 21:05:30]

What if parsing fails?

[tomhudson, 2014/08/07 14:39:42]

Done.

+        }
+    }
+    XML_SetCharacterDataHandler(*parser, fontFileNameHandler);
+}
+
+FontFamily* findFamily(FamilyData* familyData, const char* familyName) {
+    unsigned int nameLen = strlen(familyName);

[palmer, 2014/08/06 21:05:29]

size_t, not unsigned int

[tomhudson, 2014/08/07 14:39:42]

Done.

+    for (int i = 0; i < familyData->families.count(); i++) {
+        FontFamily* candidate = familyData->families[i];
+        for (int j = 0; j < candidate->fNames.count(); j++) {
+            if (!strncmp(candidate->fNames[j].c_str(), familyName, nameLen) &&
+                nameLen == strlen(candidate->fNames[j].c_str())) {
+                return candidate;
+            }
+        }
+    }
 
+    return NULL;
+}
+
+void aliasElementHandler(FamilyData* familyData, const char** attributes) {
+    // An <alias> must have name and to attributes.
+    //   It may have weight (integer).
+    // If it *does not* have a weight, it is a variant name for a <family>.
+    // If it *does* have a weight, it names the <font>(s) of a specific weight
+    //   from a <family>.
+
+    SkString aliasName;
+    SkString to;
+    int weight = 0;
+    for (int i = 0; attributes[i] != NULL; i += 2) {
+        const char* name = attributes[i];
+        const char* value = attributes[i+1];
+        int nameLen = strlen(name);
+        if (nameLen == 4 && !strncmp("name", name, nameLen)) {
+            aliasName.set(value);
+        } else if (nameLen == 2 && !strncmp("to", name, nameLen)) {
+            to.set(value);
+        } else if (nameLen == 6 && !strncmp("weight", name, nameLen)) {
+            parseNonNegativeInteger(value, &weight);
+        }
+    }
+
+    // Assumes that the named family is already declared
+    FontFamily* targetFamily = findFamily(familyData, to.c_str());
+    if (!targetFamily) {
+        SkDebugf("---- Font alias target %s (NOT FOUND)", to.c_str());
+        return;
+    }
+
+    if (weight) {
+        FontFamily* family = new FontFamily();
+        family->fNames.push_back().set(aliasName);
+
+        for (int i = 0; i < targetFamily->fFontFiles.count(); i++) {
+            if (targetFamily->fFontFiles[i].fWeight == weight) {
+                family->fFontFiles.push_back(targetFamily->fFontFiles[i]);
+            }
+        }
+        *familyData->families.append() = family;
+    } else {
+        targetFamily->fNames.push_back().set(aliasName);
+    }
+}
+
+bool findWeight400(FontFamily* family) {
+    for (int i = 0; i < family->fFontFiles.count(); i++) {
+        if (family->fFontFiles[i].fWeight == 400) {
+            return true;
+        }
+    }
+    return false;
+}
+
+bool desiredWeight(int weight) {
+    return (weight == 400 || weight == 700);
+}
+
+int countDesiredWeight(FontFamily* family) {
+    int count = 0;
+    for (int i = 0; i < family->fFontFiles.count(); i++) {
+        if (desiredWeight(family->fFontFiles[i].fWeight)) {
+            count++;
+        }
+    }
+    return count;
+}
+
+// To meet Skia's expectations, any family that contains weight=400
+// fonts should *only* contain {400,700}
+void purgeUndesiredWeights(FontFamily* family) {
+    int count = countDesiredWeight(family);
+    for (int i = 1, j = 0; i < family->fFontFiles.count(); i++) {
+        if (desiredWeight(family->fFontFiles[j].fWeight)) {
+            j++;
+        }
+        if ((i != j) && desiredWeight(family->fFontFiles[i].fWeight)) {
+            family->fFontFiles[j] = family->fFontFiles[i];
+        }
+    }
+    family->fFontFiles.resize_back(count);
+}
+
+void familysetElementEndHandler(FamilyData* familyData) {
+    for (int i = 0; i < familyData->families.count(); i++) {
+        if (findWeight400(familyData->families[i])) {
+            purgeUndesiredWeights(familyData->families[i]);
+        }
+    }
+}
+
+void startElementHandler(void* data, const char* tag,
+                         const char** attributes) {
+    FamilyData* familyData = (FamilyData*) data;
+    int len = strlen(tag);
+    if (len == 6 && !strncmp(tag, "family", len)) {
+        familyData->currentFamily = new FontFamily();
+        familyElementHandler(familyData->currentFamily, attributes);
+    } else if (len == 4 && !strncmp(tag, "font", len)) {
+        FontFileInfo* file = &familyData->currentFamily->fFontFiles.push_back();
+        familyData->currentFontInfo = file;
+        fontElementHandler(familyData->parser, file, attributes);
+    } else if (len == 5 && !strncmp(tag, "alias", len)) {
+        aliasElementHandler(familyData, attributes);
+    }
+}
+
+void endElementHandler(void* data, const char* tag) {
+    FamilyData *familyData = (FamilyData*) data;
+    int len = strlen(tag);
+    if (len == 9 && strncmp(tag, "familyset", len) == 0) {
+        familysetElementEndHandler(familyData);
+    } else if (len == 6 && strncmp(tag, "family", len) == 0) {
+        familyElementEndHandler(familyData->currentFamily);
+        *familyData->families.append() = familyData->currentFamily;
+        familyData->currentFamily = NULL;
+    } else if (len == 4 && !strncmp(tag, "font", len)) {
+        XML_SetCharacterDataHandler(*familyData->parser, NULL);
+    }
 }
 
 } // lmpParser