Test and generalize font configuration parser

src/ports/SkFontConfigParser_android.cpp

 /*
  * Copyright 2011 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkFontConfigParser_android.h"
 #include "SkTDArray.h"
 #include "SkTSearch.h"
 #include "SkTypeface.h"
 
 #include <expat.h>
 #include <stdio.h>
 #include <sys/system_properties.h>
 
 #include <limits>
 
 #define SYSTEM_FONTS_FILE "/system/etc/system_fonts.xml"
 #define FALLBACK_FONTS_FILE "/system/etc/fallback_fonts.xml"
 #define VENDOR_FONTS_FILE "/vendor/etc/fallback_fonts.xml"
 
+/**
+ * This file contains TWO parsers: one for JB and earlier (system_fonts.xml /
+ * fallback_fonts.xml), one for LMP and later (fonts.xml).
+ * We start with the JB parser, and if we detect a <familyset> tag with
+ * version 21 or higher we switch to the LMP parser.
+ */
+
 // These defines are used to determine the kind of tag that we're currently
 // populating with data. We only care about the sibling tags nameset and fileset
 // for now.
 #define NO_TAG 0
 #define NAMESET_TAG 1
 #define FILESET_TAG 2
 
 /**
  * The FamilyData structure is passed around by the parser so that each handler
  * can read these variables that are relevant to the current parsing.
  */
 struct FamilyData {
     FamilyData(XML_Parser *parserRef, SkTDArray<FontFamily*> &familiesRef) :
         parser(parserRef),
         families(familiesRef),
         currentFamily(NULL),
         currentFontInfo(NULL),
         currentTag(NO_TAG) {};
 
     XML_Parser *parser;                // The expat parser doing the work
     SkTDArray<FontFamily*> &families;  // The array that each family is put into as it is parsed
     FontFamily *currentFamily;         // The current family being created
     FontFileInfo *currentFontInfo;     // The current fontInfo being created
     int currentTag;                    // A flag to indicate whether we're in nameset/fileset tags
 };
 
+/** http://www.w3.org/TR/html-markup/datatypes.html#common.data.integer.non-negative-def */
+template <typename T> static bool parseNonNegativeInteger(const char* s, T* value) {

[tomhudson, 2014/08/04 17:33:48]

Moved to start of file so everybody can see it.

+    SK_COMPILE_ASSERT(std::numeric_limits<T>::is_integer, T_must_be_integer);
+    const T nMax = std::numeric_limits<T>::max() / 10;
+    const T dMax = std::numeric_limits<T>::max() - (nMax * 10);
+    T n = 0;
+    for (; *s; ++s) {
+        // Check if digit
+        if (*s < '0' || '9' < *s) {
+            return false;
+        }
+        int d = *s - '0';
+        // Check for overflow
+        if (n > nMax || (n == nMax && d > dMax)) {
+            return false;
+        }
+        n = (n * 10) + d;
+    }
+    *value = n;
+    return true;
+}
+
+namespace lmpParser {

[tomhudson, 2014/08/04 17:33:48]

Will fill in with cleaned-up version of prototype CL now that we know this
version detection method works.

[djsollen, 2014/08/04 18:13:35]

I'm ok with putting both of them in this file for now if you think it makes it
easier, but eventually I would like to see the old and new parsers in a separate
file.  Perhaps they could inherit from the same base class so they can both use
functions like the parseNonNegativeInteger that you have above. 

[tomhudson, 2014/08/04 21:03:56]

Acknowledged.

+
+void startElementHandler(void* data, const char* tag,
+                                   const char** attributes) {
+    //SkDebugf("lmp started %s", tag);
+}
+
+void endElementHandler(void* data, const char* tag) {
+
+    //SkDebugf("lmp ended %s", tag);
+
+}
+
+} // lmpParser
+
+namespace jbParser {
+
 /**
  * Handler for arbitrary text. This is used to parse the text inside each name
  * or file tag. The resulting strings are put into the fNames or FontFileInfo arrays.
  */
 static void textHandler(void *data, const char *s, int len) {
     FamilyData *familyData = (FamilyData*) data;
     // Make sure we're in the right state to store this name information
     if (familyData->currentFamily &&
             (familyData->currentTag == NAMESET_TAG || familyData->currentTag == FILESET_TAG)) {
         switch (familyData->currentTag) {
         case NAMESET_TAG: {
             SkAutoAsciiToLC tolc(s, len);
             familyData->currentFamily->fNames.push_back().set(tolc.lc(), len);
             break;
         }
-        case FILESET_TAG:
+        case FILESET_TAG: {

[bungeman-skia, 2014/08/04 18:02:12]

nit: The extra braces seem extraneous, normally this is only done when needed to
introduce a scope to declare a local.

[tomhudson, 2014/08/04 21:03:56]

Done.

             if (familyData->currentFontInfo) {
                 familyData->currentFontInfo->fFileName.set(s, len);
             }
-            break;
+            } break;
         default:
             // Noop - don't care about any text that's not in the Fonts or Names list
             break;
         }
     }
 }
 
-/** http://www.w3.org/TR/html-markup/datatypes.html#common.data.integer.non-negative-def */
-template <typename T> static bool parseNonNegativeInteger(const char* s, T* value) {
-    SK_COMPILE_ASSERT(std::numeric_limits<T>::is_integer, T_must_be_integer);
-    const T nMax = std::numeric_limits<T>::max() / 10;
-    const T dMax = std::numeric_limits<T>::max() - (nMax * 10);
-    T n = 0;
-    for (; *s; ++s) {
-        // Check if digit
-        if (*s < '0' || '9' < *s) {
-            return false;
-        }
-        int d = *s - '0';
-        // Check for overflow
-        if (n > nMax || (n == nMax && d > dMax)) {
-            return false;
-        }
-        n = (n * 10) + d;
-    }
-    *value = n;
-    return true;
-}
-
 /**
  * Handler for font files. This processes the attributes for language and
  * variants then lets textHandler handle the actual file name
  */
 static void fontFileElementHandler(FamilyData *familyData, const char **attributes) {
-
     FontFileInfo& newFileInfo = familyData->currentFamily->fFontFiles.push_back();
     if (attributes) {
         int currentAttributeIndex = 0;
         while (attributes[currentAttributeIndex]) {
             const char* attributeName = attributes[currentAttributeIndex];
             const char* attributeValue = attributes[currentAttributeIndex+1];
             int nameLength = strlen(attributeName);
             int valueLength = strlen(attributeValue);
             if (strncmp(attributeName, "variant", nameLength) == 0) {
                 if (strncmp(attributeValue, "elegant", valueLength) == 0) {
@@ skipping 13 matching lines @@
             }
             //each element is a pair of attributeName/attributeValue string pairs
             currentAttributeIndex += 2;
         }
     }
     familyData->currentFontInfo = &newFileInfo;
     XML_SetCharacterDataHandler(*familyData->parser, textHandler);
 }
 
 /**
- * Handler for the start of a tag. The only tags we expect are family, nameset,
- * fileset, name, and file.
+ * Handler for the start of a tag. The only tags we expect are familyset, family,
+ * nameset, fileset, name, and file.
  */
 static void startElementHandler(void *data, const char *tag, const char **atts) {
     FamilyData *familyData = (FamilyData*) data;
     int len = strlen(tag);
-    if (strncmp(tag, "family", len)== 0) {
+    if (len == 9 && strncmp(tag, "familyset", len) == 0) {

[tomhudson, 2014/08/04 17:33:48]

Note to self: remember to check len explicitly, otherwise this pattern is
fragile.

[bungeman-skia, 2014/08/04 18:02:12]

This is an interesting quirk, in that with strncmp you want 'len+1' in order to
compare against the '\0' at the end of the tag. I like your code better, since
you're checking length first and only computing it once.

nit: On the other hand, it seems that now just using strcmp is faster and no
less good, since you already know there is a '\0', and there's no real point in
the extra count check.

I'm not sure of a good readable way to do this, but 9 is of course somewhat
magic. It would be nice to somehow link this with strlen("familyset").

[tomhudson, 2014/08/04 18:14:49]

The len == MAGIC && !strncmp() pattern was present in the earlier versions of
this file, but only on some of the if() statements; I got bitten by not having
it more widespread, and so used it more widely - but not, as you point out,
everywhere.

There are some apparent benign failures because of how they hadn't used it in
every case: endElementHandler() for family ought to trigger on both family and
familyset. In the latter case, it'd append NULL to the families array, which I
suppose is safe.

+        // The familyset tag has an optional "version" attribute with an integer value >= 0
+        for (int i = 0; atts[i] != NULL; i += 2) {
+            int nameLen = strlen(atts[i]);
+            if (strncmp(atts[i], "version", nameLen)) continue;

[bungeman-skia, 2014/08/04 18:02:12]

Add the 'nameLen = strlen("version")' thing here too? Prevent collision with
"version-new" or something down the road.

[tomhudson, 2014/08/04 21:03:56]

Done.

Added it to every strlen() in the file, including the JB endElementHandler's
"family", which was also catching "familyset" - so this is a behavior change,
but should be invisible/benign.

+            const char* valueString = atts[i+1];
+            int version;
+            int len = sscanf(valueString, "%d", &version);

[bungeman-skia, 2014/08/04 18:02:12]

It seems you put parseNonNegativeInteger up top in order to use it, but didn't
here? sscanf supports all sorts of bogus sillyness that probably shouldn't be
allowed here.

[tomhudson, 2014/08/04 18:14:49]

qv <family> order=; for some reason the previous version of this file only used
parseNonNegativeInteger() in a few contexts. I'll try using it more widely.

[tomhudson, 2014/08/04 21:03:56]

Done.

+            if (len > 0 && version >= 21) {
+                XML_SetElementHandler(*familyData->parser,
+                                      lmpParser::startElementHandler,
+                                      lmpParser::endElementHandler);
+            }
+        }
+    } else if (len == 6 && strncmp(tag, "family", len) == 0) {
         familyData->currentFamily = new FontFamily();
         familyData->currentFamily->order = -1;
         // The Family tag has an optional "order" attribute with an integer value >= 0
         // If this attribute does not exist, the default value is -1
         for (int i = 0; atts[i] != NULL; i += 2) {
             const char* valueString = atts[i+1];
             int value;
             int len = sscanf(valueString, "%d", &value);
             if (len > 0) {
                 familyData->currentFamily->order = value;
@@ skipping 27 matching lines @@
         familyData->currentTag = NO_TAG;
     } else if (len == 7 && strncmp(tag, "fileset", len) == 0) {
         familyData->currentTag = NO_TAG;
     } else if ((strncmp(tag, "name", len) == 0 && familyData->currentTag == NAMESET_TAG) ||
             (strncmp(tag, "file", len) == 0 && familyData->currentTag == FILESET_TAG)) {
         // Disable the arbitrary text handler installed to load Name data
         XML_SetCharacterDataHandler(*familyData->parser, NULL);
     }
 }
 
+} // namespace jbParser
+
 /**
  * This function parses the given filename and stores the results in the given
  * families array.
  */
 static void parseConfigFile(const char *filename, SkTDArray<FontFamily*> &families) {
 
     FILE* file = NULL;
 
 #if !defined(SK_BUILD_FOR_ANDROID_FRAMEWORK)
     // if we are using a version of Android prior to Android 4.2 (JellyBean MR1
@@ skipping 29 matching lines @@
 
     // Some of the files we attempt to parse (in particular, /vendor/etc/fallback_fonts.xml)
     // are optional - failure here is okay because one of these optional files may not exist.
     if (NULL == file) {
         return;
     }
 
     XML_Parser parser = XML_ParserCreate(NULL);
     FamilyData *familyData = new FamilyData(&parser, families);
     XML_SetUserData(parser, familyData);
-    XML_SetElementHandler(parser, startElementHandler, endElementHandler);
+    // Start parsing oldschool; switch these in flight if we detect a newer version of the file.
+    XML_SetElementHandler(parser, jbParser::startElementHandler, jbParser::endElementHandler);
 
     char buffer[512];
     bool done = false;
     while (!done) {
         fgets(buffer, sizeof(buffer), file);
         int len = strlen(buffer);
         if (feof(file) != 0) {
             done = true;
         }
         XML_Parse(parser, buffer, len, done);
@@ skipping 51 matching lines @@
         *fontFamilies.append() = fallbackFonts[i];
     }
 }
 
 void SkFontConfigParser::GetTestFontFamilies(SkTDArray<FontFamily*> &fontFamilies,
                                              const char* testMainConfigFile,
                                              const char* testFallbackConfigFile) {
     parseConfigFile(testMainConfigFile, fontFamilies);
 
     SkTDArray<FontFamily*> fallbackFonts;
-    parseConfigFile(testFallbackConfigFile, fallbackFonts);
+    if (NULL != testFallbackConfigFile) {

[tomhudson, 2014/08/04 17:33:48]

For v22, we only have one file; instead of adding a separate entry point, I made
the second argument effectively optional.

[djsollen, 2014/08/04 18:13:35]

Acknowledged.

+        parseConfigFile(testFallbackConfigFile, fallbackFonts);
+    }
 
     // Append all fallback fonts to system fonts
     for (int i = 0; i < fallbackFonts.count(); ++i) {
         fallbackFonts[i]->fIsFallbackFont = true;
         *fontFamilies.append() = fallbackFonts[i];
     }
 }
 
 /**
  * Read the persistent locale.
@@ skipping 17 matching lines @@
     SkString locale(6);
     char* localeCStr = locale.writable_str();
 
     strncpy(localeCStr, propLang, 2);
     localeCStr[2] = '-';
     strncpy(&localeCStr[3], propRegn, 2);
     localeCStr[5] = '\0';
 
     return locale;
 }