Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1138073002:
Extensible Android font configuration parsing. (Closed)
|
Created:
5 years, 7 months ago by bungeman-skia Modified:
5 years, 7 months ago Reviewers:
djsollen CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionExtensible Android font configuration parsing.
The xml parsing of the Android font configuration is quite lax in what
it allows. This has lead to issues with forward compatibility and
extending the existing format. This has lead to some confusion about
what the actual format is and how a given proposed change will affect
existing files and readers.
The main issue this fixes is containment. Tags are now only recognized
at the correct levels in the correct containing tags. Tags which are
not recognized are properly skipped. Tags which accumulate character
data now only accumulate the character data in their own element as
opposed to all child elements.
Committed: https://skia.googlesource.com/skia/+/10b063cb91c52fd1f570ee63307fe7e68c1501f1
Patch Set 1 #
Total comments: 1
Patch Set 2 : Const, comments, length. #
Total comments: 4
Patch Set 3 : Add comments, allow NULL. #Messages
Total messages: 21 (7 generated)
bungeman@google.com changed reviewers: + djsollen@google.com
https://codereview.chromium.org/1138073002/diff/1/src/ports/SkFontConfigParse... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/1138073002/diff/1/src/ports/SkFontConfigParse... src/ports/SkFontConfigParser_android.cpp:154: static TagHandler fontHandler = { I find this formatting really hard to parse. Why function ptrs and not just virtual methods on the TagHandler class?
On 2015/05/11 17:49:54, djsollen wrote: > https://codereview.chromium.org/1138073002/diff/1/src/ports/SkFontConfigParse... > File src/ports/SkFontConfigParser_android.cpp (right): > > https://codereview.chromium.org/1138073002/diff/1/src/ports/SkFontConfigParse... > src/ports/SkFontConfigParser_android.cpp:154: static TagHandler fontHandler = { > I find this formatting really hard to parse. Why function ptrs and not just > virtual methods on the TagHandler class? The disadvantages of using classes here: Must name the classes, even though I don't ever want the names to be used. I just need named vtables. Must ensure the classes remain trivial so that the global named instances don't cause startup constructors. Must document that TagHandler should *never* have state or use 'this' (if you're not supposed to use 'this' why be a class?). Cannot directly contain the XML_CharacterHandler, requires extra level of runtime abstraction. The advantages of using classes here: People are more familiar reading them. Must state the names of the functions when defining them (if c++ had designated initializers this wouldn't be so bad). Esentially, this looks like 'c' because expat has a 'c' style interface. It seems like a lot fewer lines of code this way as well. If I put the names of the fields in comments in the definitions, would that be enough?
yes, comments would be sufficient.
Cleaned it up, added comments.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138073002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... src/ports/SkFontConfigParser_android.cpp:52: struct TagHandler { comments for the purpose of each of these methods. https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... src/ports/SkFontConfigParser_android.cpp:296: /*end*/[](FamilyData* self, const char* tag) { }, why not a null function ptr?
btw...those are the last of my comments so after they are addressed lgtm.
https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... File src/ports/SkFontConfigParser_android.cpp (right): https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... src/ports/SkFontConfigParser_android.cpp:52: struct TagHandler { On 2015/05/13 14:27:33, djsollen wrote: > comments for the purpose of each of these methods. Done. https://codereview.chromium.org/1138073002/diff/20001/src/ports/SkFontConfigP... src/ports/SkFontConfigParser_android.cpp:296: /*end*/[](FamilyData* self, const char* tag) { }, On 2015/05/13 14:27:33, djsollen wrote: > why not a null function ptr? Tags which are just containers without attributes often don't care about 'start'. Obviously, there are a number of handlers with no special work to do at the 'end'. 'Expected leaf elements' always skip any nested tags so those could also signal this by having 'tag' be NULL. Allowing the 'chars' to be NULL is easy, because it can just be passed directly into XML_SetCharacterDataHandler which already takes 'NULL' to mean 'no handler'. This means doing more NULL checks in start_element_handler / end_element_handler to avoid these trivial indirect function calls. I've written this up. Take a look and see if you think it's worth the trade off. Now that I've written it, I'm kind of neutral about it.
The CQ bit was checked by bungeman@google.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from djsollen@google.com Link to the patchset: https://codereview.chromium.org/1138073002/#ps40001 (title: "Add comments, allow NULL.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138073002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bungeman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1138073002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/10b063cb91c52fd1f570ee63307fe7e68c1501f1 |
