Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
446473003:
Parser for new fonts.xml format (Closed)
|
Created:
6 years, 4 months ago by Tom Hudson Modified:
6 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@fcparse-lmp-2 Project:
skia Visibility:
Public. |
DescriptionParses sample code provided by Android project. Attempts to keep FontFamily data structures produced consistent with expectations of previous versions of Skia.
R=bungeman@google.com,djsollen@google.com
BUG=400801
Committed: https://skia.googlesource.com/skia/+/07544757c9fcf0f359f1686a3779eb2e75dd5b36
Committed: https://skia.googlesource.com/skia/+/d3ddea284ec6611a93a6b75e64de39d0bc7e083c
Patch Set 1 #Patch Set 2 : Complete basic implementation #
Total comments: 8
Patch Set 3 : Derek's review #
Total comments: 3
Patch Set 4 : Do you speak-a my language? #
Total comments: 27
Patch Set 5 : Palmer review #Patch Set 6 : Rebase and add missing && #
Messages
Total messages: 24 (0 generated)
PTAL. I don't like purgeUndesiredWeights() and its helpers, but I think this is necessary for the data structures Skia expects; we can discuss if there's another way.
https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:110: family->fFontFiles[i].fPaintOptions.setLanguage(family->language); no immediate change here, but post m37 let's remove fPaintOptions from the fontFiles and just use the newly add fLanguage and fVariant on the family. https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:215: void purgeUndesiredWeights(FontFamily* family) { so after tracing a few examples though my head, I'm roughly convinced that this works, but will be more than happy to delete this post M37. https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:23: int weight; // only used internally by SkFontConfigParser in LMP+ fWeight instead of weight. Also why is it internal only? Can't we use it externally in future versions of our font manager? https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:40: SkString language; // only used internally by SkFontConfigParser in LMP+ If we made this an SkLanguage param then I don't see why we would need to say that it is internal only. When I've been reading over this code I think that it is odd that we get SkLanguage and FontVariant from the individual font files and not the family. I would like to see both fLanguage and fVariant be first class members on this struct, not just internal only.
https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:110: family->fFontFiles[i].fPaintOptions.setLanguage(family->language); On 2014/08/05 17:46:15, djsollen wrote: > no immediate change here, but post m37 let's remove fPaintOptions from the > fontFiles and just use the newly add fLanguage and fVariant on the family. Acknowledged. http://skbug.com/2803 https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:215: void purgeUndesiredWeights(FontFamily* family) { On 2014/08/05 17:46:15, djsollen wrote: > so after tracing a few examples though my head, I'm roughly convinced that this > works, but will be more than happy to delete this post M37. Acknowledged. http://skbug.com/2804 https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:23: int weight; // only used internally by SkFontConfigParser in LMP+ On 2014/08/05 17:46:15, djsollen wrote: > fWeight instead of weight. Also why is it internal only? Can't we use it > externally in future versions of our font manager? Done. https://codereview.chromium.org/446473003/diff/20001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:40: SkString language; // only used internally by SkFontConfigParser in LMP+ PS2 has fLanguage and fVariant, but missed the SkString -> SkLanguage change.
lgtm with a few nits. https://codereview.chromium.org/446473003/diff/40001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/446473003/diff/40001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:45: bool fIsFallbackFont; nit, why reorder fIsFallbackFont? https://codereview.chromium.org/446473003/diff/40001/tests/FontConfigParser.cpp File tests/FontConfigParser.cpp (right): https://codereview.chromium.org/446473003/diff/40001/tests/FontConfigParser.c... tests/FontConfigParser.cpp:63: extra space? If you were trying to provide separation between pre17 and v17 why not just put a comment that this block is testing the v17 path instead of the extra space. https://codereview.chromium.org/446473003/diff/40001/tests/FontConfigParser.c... tests/FontConfigParser.cpp:74: same here.
On 2014/08/05 19:44:08, djsollen wrote: > lgtm with a few nits. > > https://codereview.chromium.org/446473003/diff/40001/src/ports/SkFontConfigPa... > File src/ports/SkFontConfigParser_android.h (right): > > https://codereview.chromium.org/446473003/diff/40001/src/ports/SkFontConfigPa... > src/ports/SkFontConfigParser_android.h:45: bool > fIsFallbackFont; > nit, why reorder fIsFallbackFont? Knee-jerk reordering the struct to minimize interior padding. > https://codereview.chromium.org/446473003/diff/40001/tests/FontConfigParser.cpp > File tests/FontConfigParser.cpp (right): > > https://codereview.chromium.org/446473003/diff/40001/tests/FontConfigParser.c... > tests/FontConfigParser.cpp:63: > extra space? If you were trying to provide separation between pre17 and v17 why > not just put a comment that this block is testing the v17 path instead of the > extra space. IMO, that comment would be noise. The space suggests a break without shoving it in the reader's face. PatchSet 4 switched fLanguage to SkLanguage - is that what you'd intended? Or did you think of it as future work?
Since all my nits had logical rebuttals I see no reason to delay. LGTM at this patch set as well.
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/446473003/60001
Message was sent while issue was closed.
Change committed as 07544757c9fcf0f359f1686a3779eb2e75dd5b36
Message was sent while issue was closed.
https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:83: for (int i = 0; attributes[i] != NULL; i += 2) { 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.) https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:86: int nameLen = strlen(name); strlen returns size_t, not int. (The 2 types differ in signedness and possibly in width.) https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:103: void fontFileNameHandler(void *data, const char *s, int len) { 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. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:104: FamilyData *familyData = (FamilyData*) data; Prefer C++ style cast? https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:105: familyData->currentFontInfo->fFileName.set(s, len); If |set| is defined to take an int length argument, that's a bug. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:109: for (int i = 0; i < family->fFontFiles.count(); i++) { size_t, and if |count| returns int, that's a bug https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:119: for (int i = 0; attributes[i] != NULL; i += 2) { Same int and += 2 comments as above https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:124: parseNonNegativeInteger(value, &file->fWeight); What if parsing fails? https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:131: unsigned int nameLen = strlen(familyName); size_t, not unsigned int https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:29: * font names that alias to a font family. fontFileArray is the list of information "fontFileArray" should be "fFontFiles"? (Which itself is a a bad variable name; why not |fFontFileInfos|?) https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:30: * about each file. Order is the priority order for the font. This is It's |order|, not |Order|; but also, why not |fOrder|? Overall, better names would obviate this documentation, and then you wouldn't have inconsistencies between the docs and the code. Does |fVariant| need explanation? Does it need to be initialized in the constructor?
Message was sent while issue was closed.
https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:83: for (int i = 0; attributes[i] != NULL; i += 2) { On 2014/08/06 21:05:30, Chromium Palmer wrote: > 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.) Skia uses int instead of size_t for most counters and indices. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:85: const char* value = attributes[i+1]; could assert that value != NULL, to address the concern about skipping past the sentinel. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:86: int nameLen = strlen(name); On 2014/08/06 21:05:29, Chromium Palmer wrote: > strlen returns size_t, not int. (The 2 types differ in signedness and possibly > in width.) +1 https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:105: familyData->currentFontInfo->fFileName.set(s, len); On 2014/08/06 21:05:30, Chromium Palmer wrote: > If |set| is defined to take an int length argument, that's a bug. +1 https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:109: for (int i = 0; i < family->fFontFiles.count(); i++) { On 2014/08/06 21:05:29, Chromium Palmer wrote: > size_t, and if |count| returns int, that's a bug count() returns an int.
Message was sent while issue was closed.
> Skia uses int instead of size_t for most counters and indices. Is there a bug open to fix that? I seem to recall one, once...
On 2014/08/06 22:38:20, Chromium Palmer wrote: > > Skia uses int instead of size_t for most counters and indices. > > Is there a bug open to fix that? I seem to recall one, once... Google3 and Skia, by design, used signed ints for loops, and hence for indices and counts.
Renaming struct fields pulled in a couple of other files for mechanical renaming. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:83: for (int i = 0; attributes[i] != NULL; i += 2) { On 2014/08/06 21:05:30, Chromium Palmer wrote: > 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. 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. > (Also, size_t is the proper size for object counters, lengths, and indices.) UNDone; see http://skbug.com/2811. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:86: int nameLen = strlen(name); On 2014/08/06 21:05:29, Chromium Palmer wrote: > strlen returns size_t, not int. (The 2 types differ in signedness and possibly > in width.) 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. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:103: void fontFileNameHandler(void *data, const char *s, int len) { On 2014/08/06 21:05:30, Chromium Palmer wrote: > 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. 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. > |len| should be a size_t, as usual. Unfortunately, expat is third-party and specifies an int there: typedef void (XMLCALL *XML_CharacterDataHandler)(void *userData, const XML_Char *s, int len); https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:105: familyData->currentFontInfo->fFileName.set(s, len); On 2014/08/06 21:05:30, Chromium Palmer wrote: > If |set| is defined to take an int length argument, that's a bug. SkString::set() takes size_t, but is happy to implicitly cast an int. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:109: for (int i = 0; i < family->fFontFiles.count(); i++) { On 2014/08/06 21:05:29, Chromium Palmer wrote: > size_t, and if |count| returns int, that's a bug 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. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:119: for (int i = 0; attributes[i] != NULL; i += 2) { On 2014/08/06 21:05:29, Chromium Palmer wrote: > Same int and += 2 comments as above Done. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:124: parseNonNegativeInteger(value, &file->fWeight); On 2014/08/06 21:05:30, Chromium Palmer wrote: > What if parsing fails? Done. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:131: unsigned int nameLen = strlen(familyName); On 2014/08/06 21:05:29, Chromium Palmer wrote: > size_t, not unsigned int Done. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.cpp:357: familyData->currentFamily->order = -1; This default value is set in the FontFamily constructor. https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... File src/ports/SkFontConfigParser_android.h (right): https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:29: * font names that alias to a font family. fontFileArray is the list of information On 2014/08/06 21:05:30, Chromium Palmer wrote: > "fontFileArray" should be "fFontFiles"? (Which itself is a a bad variable name; > why not |fFontFileInfos|?) Comment fix done. I disagree on the naming as a general rule: we wouldn't praise "int fInt", we'd want the name to tell us something about the meaning of the variable, not just its types (and this is a bad pattern ALL OVER CHROME). Here we have a struct with a reasonable name (personally I'd consider shortening the struct name to FileInfo but move it into the SkFontConfigParser namespace?), so the field name is going to echo it some; I find |fFonts| to be clearer than fFontInfos. (We expect in a future Android release to support multiple fonts inside the same file, anyway, which is what fIndex is for.) https://codereview.chromium.org/446473003/diff/60001/src/ports/SkFontConfigPa... src/ports/SkFontConfigParser_android.h:30: * about each file. Order is the priority order for the font. This is On 2014/08/06 21:05:30, Chromium Palmer wrote: > It's |order|, not |Order|; but also, why not |fOrder|? I suspect this is code archaeology: in my experience with older Skia code, protected class members get fCamelCase, while public struct members haveNoLeadingTag. That's not what the style guide says, though, so fixed. (Policy aside: this was a last minute change for Android functionality, so we were focused on minimal changes, not even style fix-ups. It was bad to not update the comment, however). > Overall, better names would obviate this documentation, and then you wouldn't > have inconsistencies between the docs and the code. I'd argue that it's bad doc, but that good doc is not replaced by good naming. Heavily revised. > Does |fVariant| need explanation? Does it need to be initialized in the > constructor? Was already initialized in constructor?
palmer@: Ping - the only thing I think you need to see is that some of what you're noting is API/contract from third-party/expat/; do you want to add that package to the queue for a security review?
> palmer@: Ping - the only thing I think you need to see is that some of what > you're noting is API/contract from third-party/expat/; do you want to add that > package to the queue for a security review? No, it's outside of my scope for the time being. It has a bad track record, but if your code adheres to its contract then that has to be OK for now.
lgtm at PS5
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/446473003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for src/ports/SkFontMgr_android.cpp:
While running git apply --index -p1;
error: patch failed: src/ports/SkFontMgr_android.cpp:131
error: src/ports/SkFontMgr_android.cpp: patch does not apply
Patch: src/ports/SkFontMgr_android.cpp
Index: src/ports/SkFontMgr_android.cpp
diff --git a/src/ports/SkFontMgr_android.cpp b/src/ports/SkFontMgr_android.cpp
index
1ab2f29642a1147be1bc0404e43faf951f3af113..511f4818a640e7a3109c7c2f8800cd37b6e9e3ad
100644
--- a/src/ports/SkFontMgr_android.cpp
+++ b/src/ports/SkFontMgr_android.cpp
@@ -131,8 +131,8 @@ public:
cannonicalFamilyName = &family.fNames[0];
}
// TODO? make this lazy
- for (int i = 0; i < family.fFontFiles.count(); ++i) {
- const FontFileInfo& fontFile = family.fFontFiles[i];
+ for (int i = 0; i < family.fFonts.count(); ++i) {
+ const FontFileInfo& fontFile = family.fFonts[i];
SkString pathName;
get_path_for_sys_fonts(&pathName, fontFile.fFileName);
The CQ bit was checked by tomhudson@google.com
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/tomhudson@chromium.org/446473003/100001
Message was sent while issue was closed.
Change committed as d3ddea284ec6611a93a6b75e64de39d0bc7e083c |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
