Test and generalize font configuration parser

Test and generalize font configuration parser

Add a basic unit test for the Android font configuration parser.
Add a check for the new LMP file format; on detection, switch to a new
(as-yet unwritten) parser.

R=djsollen@google.com,bungeman@google.com

Committed: https://skia.googlesource.com/skia/+/f79673bbae0a662c1428755e2719dadf944e4ba1

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

PTAL; I'd like to consider landing this while I fill in the rest of the
implementation.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:57: template <typename T> static bool
parseNonNegativeInteger(const char* s, T* value) {
Moved to start of file so everybody can see it.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:78: namespace lmpParser {
Will fill in with cleaned-up version of prototype CL now that we know this
version detection method works.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:166: if (len == 9 && strncmp(tag,
"familyset", len) == 0) {
Note to self: remember to check len explicitly, otherwise this pattern is
fragile.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:352: if (NULL !=
testFallbackConfigFile) {
For v22, we only have one file; instead of adding a separate entry point, I made
the second argument effectively optional.

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

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:110: case FILESET_TAG: {
nit: The extra braces seem extraneous, normally this is only done when needed to
introduce a scope to declare a local.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:166: if (len == 9 && strncmp(tag,
"familyset", len) == 0) {
On 2014/08/04 17:33:48, tomhudson wrote:
> Note to self: remember to check len explicitly, otherwise this pattern is
> fragile.

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").

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:170: if (strncmp(atts[i], "version",
nameLen)) continue;
Add the 'nameLen = strlen("version")' thing here too? Prevent collision with
"version-new" or something down the road.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:173: int len = sscanf(valueString,
"%d", &version);
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.

[djsollen, 2014-08-04 18:13:36]

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:78: namespace lmpParser {
On 2014/08/04 17:33:48, tomhudson wrote:
> Will fill in with cleaned-up version of prototype CL now that we know this
> version detection method works.

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.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:352: if (NULL !=
testFallbackConfigFile) {
On 2014/08/04 17:33:48, tomhudson wrote:
> For v22, we only have one file; instead of adding a separate entry point, I
made
> the second argument effectively optional.

Acknowledged.

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.cpp
File tests/FontConfigParser.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:13: SkDebugf(" .. validating:");
remove

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:23: void DumpLoadedFonts(SkTDArray<FontFamily*>
fontFamilies) {
if you want to keep this function around put it behind a debugging define and
default the define to 0 at the top of the file.

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:37:
"/data/local/tmp/resources/android_fonts/pre_v17/system_fonts.xml",
I think there is some kind of GetResourceDir function so we don't hardcode
/data/local/tmp.

[tomhudson, 2014-08-04 18:14:50]

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:166: if (len == 9 && strncmp(tag,
"familyset", len) == 0) {
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.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:173: int len = sscanf(valueString,
"%d", &version);
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:57]

Still needs to be updated with revised fonts.xml from Android.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:78: namespace lmpParser {
On 2014/08/04 18:13:35, djsollen wrote:
> On 2014/08/04 17:33:48, tomhudson wrote:
> > Will fill in with cleaned-up version of prototype CL now that we know this
> > version detection method works.
> 
> 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. 

Acknowledged.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:110: case FILESET_TAG: {
On 2014/08/04 18:02:12, bungeman1 wrote:
> nit: The extra braces seem extraneous, normally this is only done when needed
to
> introduce a scope to declare a local.

Done.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:170: if (strncmp(atts[i], "version",
nameLen)) continue;
On 2014/08/04 18:02:12, bungeman1 wrote:
> Add the 'nameLen = strlen("version")' thing here too? Prevent collision with
> "version-new" or something down the road.

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.

https://codereview.chromium.org/439813002/diff/20001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:173: int len = sscanf(valueString,
"%d", &version);
On 2014/08/04 18:02:12, bungeman1 wrote:
> 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.

Done.

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.cpp
File tests/FontConfigParser.cpp (right):

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:13: SkDebugf(" .. validating:");
On 2014/08/04 18:13:36, djsollen wrote:
> remove

Done.

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:23: void DumpLoadedFonts(SkTDArray<FontFamily*>
fontFamilies) {
On 2014/08/04 18:13:35, djsollen wrote:
> if you want to keep this function around put it behind a debugging define and
> default the define to 0 at the top of the file.

Done.

Reused SK_DEBUG_FONTS instead of creating a custom define?

https://codereview.chromium.org/439813002/diff/20001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:37:
"/data/local/tmp/resources/android_fonts/pre_v17/system_fonts.xml",
Done, after much argumentation.

It is now necessary to pass  --resourcePath /data/local/tmp/resources/  to the
test runner.

[bungeman-skia, 2014-08-04 21:27:56]

lgtm with one possible nit

https://codereview.chromium.org/439813002/diff/40001/src/ports/SkFontConfigPa...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/439813002/diff/40001/src/ports/SkFontConfigPa...
src/ports/SkFontConfigParser_android.cpp:175: if (success && version >= 21) {
nit: I think there's enough room here for

if (parseNonNegativeInteger(valueString, &version) && version >= 21) {

which avoids the extra 'success' name.

[tomhudson, 2014-08-05 13:25:04]

The CQ bit was checked by tomhudson@google.com

[commit-bot: I haz the power, 2014-08-05 13:25:25]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/439813002/60001

[commit-bot: I haz the power, 2014-08-05 13:36:23]

Change committed as f79673bbae0a662c1428755e2719dadf944e4ba1

[djsollen, 2014-08-05 15:52:01]

https://codereview.chromium.org/439813002/diff/60001/tests/FontConfigParser.cpp
File tests/FontConfigParser.cpp (right):

https://codereview.chromium.org/439813002/diff/60001/tests/FontConfigParser.c...
tests/FontConfigParser.cpp:38:
GetResourcePath("android_fonts/pre_v17/system_fonts.xml").c_str(),
so these tests fail if you don't provide a resource path?  What is the standard
procedure if no path is provided?  In other testes do we pass with a warning or
fail?