Additional cleanups to Android config parsing.

Additional cleanups to Android config parsing.

Properly labels several methods as static.
Use XML_GetBuffer to avoid an extra copy.
Set the memory allocators to Skia's.
Set define in 'defines' instead of cflags.
Update debug dumper.

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

[bungeman-skia, 2015-02-09 23:33:18]

bungeman@google.com changed reviewers:
+ djsollen@google.com, scroggo@google.com

[scroggo, 2015-02-10 13:59:42]

lgtm

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
File platform_tools/android/gyp/dependencies.gypi (right):

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
platform_tools/android/gyp/dependencies.gypi:49: 'HAVE_EXPAT_CONFIG_H',
IIRC, the difference here is that defines will get put in SkUserConfig, which is
our hacky way of exporting compile flags. Is that the intent here?

https://codereview.chromium.org/915443002/diff/1/src/ports/SkFontConfigParser...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/915443002/diff/1/src/ports/SkFontConfigParser...
src/ports/SkFontConfigParser_android.cpp:455: void* buffer =
XML_GetBuffer(parser, bufferSize);
I assume XML_GetBuffer owns buffer?

https://codereview.chromium.org/915443002/diff/1/tests/FontConfigParser.cpp
File tests/FontConfigParser.cpp (right):

https://codereview.chromium.org/915443002/diff/1/tests/FontConfigParser.cpp#n...
tests/FontConfigParser.cpp:40: SkDebugf("  basePath %s\n",
fontFamilies[i]->fBasePath.c_str());
I'm guessing this bit-rotted due to SK_DEBUG_FONTS not being defined? Should we
define it for some builder?

[scroggo, 2015-02-10 15:11:26]

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
File platform_tools/android/gyp/dependencies.gypi (right):

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
platform_tools/android/gyp/dependencies.gypi:49: 'HAVE_EXPAT_CONFIG_H',
On 2015/02/10 13:59:41, scroggo wrote:
> IIRC, the difference here is that defines will get put in SkUserConfig, which
is
> our hacky way of exporting compile flags. Is that the intent here?

Discussed this in person:
I was referring to the Android framework build, which this does not affect.

Ben actually wanted it to show up in his IDE (or some other tool) as a define,
which this allows.

Back to the Android framework build, perhaps a better method would be to be more
explicit, and list defines we want to export in export_dependent_settings (which
may be all of them?). This will probably be necessary anyway once the Android
build system starts supporting exported flags. I've filed skbug.com/3415 to
track that.

[bungeman-skia, 2015-02-10 15:33:10]

New patchsets have been uploaded after l-g-t-m from scroggo@google.com

[bungeman-skia, 2015-02-10 15:34:13]

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
File platform_tools/android/gyp/dependencies.gypi (right):

https://codereview.chromium.org/915443002/diff/1/platform_tools/android/gyp/d...
platform_tools/android/gyp/dependencies.gypi:49: 'HAVE_EXPAT_CONFIG_H',
On 2015/02/10 13:59:41, scroggo wrote:
> IIRC, the difference here is that defines will get put in SkUserConfig, which
is
> our hacky way of exporting compile flags. Is that the intent here?

So the idea was to just have all the defines in 'defines' which makes it easier
in my editor (since it's looking at the 'defines' for defines) to see which
macros are defined so that my editor can say, for example, which code is live
and which is defined out. This came up with expat, as it will behave quite
differently if XML_CONTEXT_BYTES isn't defined.

Turns out after discussion that this target is only used by Skia tool builds and
is skipped by the Android makefile generation, so this shouldn't make a
difference there.

https://codereview.chromium.org/915443002/diff/1/src/ports/SkFontConfigParser...
File src/ports/SkFontConfigParser_android.cpp (right):

https://codereview.chromium.org/915443002/diff/1/src/ports/SkFontConfigParser...
src/ports/SkFontConfigParser_android.cpp:455: void* buffer =
XML_GetBuffer(parser, bufferSize);
On 2015/02/10 13:59:41, scroggo wrote:
> I assume XML_GetBuffer owns buffer?

Yes, the parser manages its own buffer (and does anyway under the hood, even if
you call XML_Parse). It will be freed when XML_ParserFree is called (from the
SkAutoTCallVProc above).

https://codereview.chromium.org/915443002/diff/1/tests/FontConfigParser.cpp
File tests/FontConfigParser.cpp (right):

https://codereview.chromium.org/915443002/diff/1/tests/FontConfigParser.cpp#n...
tests/FontConfigParser.cpp:40: SkDebugf("  basePath %s\n",
fontFamilies[i]->fBasePath.c_str());
On 2015/02/10 13:59:41, scroggo wrote:
> I'm guessing this bit-rotted due to SK_DEBUG_FONTS not being defined? Should
we
> define it for some builder?

I think it might be more due to the fact that I didn't add this change to my
previous CL. We should re-work this to avoid bit rot, for sure. I already have a
'--verboseFontMgr' flag which should work with this, with the benefit of
removing these 'ifdefs' and avoiding the dreaded random 'unreachable code'
warnings.

[scroggo, 2015-02-10 15:35:00]

LGTM

[bungeman-skia, 2015-02-10 15:45:03]

The CQ bit was checked by bungeman@google.com

[commit-bot: I haz the power, 2015-02-10 15:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/915443002/20001

[commit-bot: I haz the power, 2015-02-10 15:51:17]

Committed patchset #2 (id:20001) as
https://skia.googlesource.com/skia/+/eb2be7fa4413a566212782d8efae5dc225002821