Test and generalize font configuration parser

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 cc2ca8cad92f7fe4fcc7e4e66a0c24833081635a..1ba25f5ac58f4e1a30ed45e6064b9a5901b3db50 100644
--- a/src/ports/SkFontConfigParser_android.cpp
+++ b/src/ports/SkFontConfigParser_android.cpp
@@ -20,6 +20,13 @@
 #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.
@@ -46,6 +53,45 @@ struct FamilyData {
     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.
@@ -61,11 +107,11 @@ static void textHandler(void *data, const char *s, int 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;
@@ -73,34 +119,11 @@ static void textHandler(void *data, const char *s, int len) {
     }
 }
 
-/** 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;
@@ -134,13 +157,27 @@ static void fontFileElementHandler(FamilyData *familyData, const char **attribut
 }
 
 /**
- * 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
@@ -188,6 +225,8 @@ static void endElementHandler(void *data, const char *tag) {
     }
 }
 
+} // namespace jbParser
+
 /**
  * This function parses the given filename and stores the results in the given
  * families array.
@@ -237,7 +276,8 @@ static void parseConfigFile(const char *filename, SkTDArray<FontFamily*> &famili
     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;
@@ -309,7 +349,9 @@ void SkFontConfigParser::GetTestFontFamilies(SkTDArray<FontFamily*> &fontFamilie
     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) {