Refactor manual font table parsing in CFX_FolderFontInfo [pdfium : main]
Tom Sepez (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 9 comments![]( "Open in Gerrit")
parsing logic in `CFX_FolderFontInfo` when scanning system fonts and retrieving font data.wrap
uint32_t name_tag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');Was fine as a constexpr value?
auto loc = FindFontTable(tables.unsigned_span(), name_tag);Can't figure out what this is without looking it up. Same for other FindFontTable() callers.
FX_SAFE_FILESIZE safe_os2_end = os2_loc->offset;
safe_os2_end += os2_loc->size;
if (safe_os2_end.IsValid() && safe_os2_end.ValueOrDie() <= filesize) {
auto os2_data = FixedSizeDataVector<uint8_t>::Uninit(os2_loc->size);
if (fseek(pFile, os2_loc->offset, SEEK_SET) >= 0 &&
fxcrt::spanread(os2_data.span(), pFile).size() == os2_loc->size) {Looks like lines 214-222. Refactor into helper?
// TODO(tsepez): iterate over span.Did you want to fulfill this in FindFontTable()?
pdfium::span<const uint8_t> GetFontTable(pdfium::span<const uint8_t> font_data,Only called from unit tests?
std::optional<FontTableLocation> FindFontTable(Has no unit test coverage.
size_t nTables = table_dir.size() / 16u;Might as well update C++ style since it's now in a different file and looks like new code.
Related details
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 9 comments![]( "Open in Gerrit")
parsing logic in `CFX_FolderFontInfo` when scanning system fonts and retrieving font data.Tom Sepezwrap
Done
Leave out random newline changes.
Done
uint32_t name_tag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');Was fine as a constexpr value?
Done. Bud didn't need to be static.
auto loc = FindFontTable(tables.unsigned_span(), name_tag);Can't figure out what this is without looking it up. Same for other FindFontTable() callers.
Done
FX_SAFE_FILESIZE safe_os2_end = os2_loc->offset;
safe_os2_end += os2_loc->size;
if (safe_os2_end.IsValid() && safe_os2_end.ValueOrDie() <= filesize) {
auto os2_data = FixedSizeDataVector<uint8_t>::Uninit(os2_loc->size);
if (fseek(pFile, os2_loc->offset, SEEK_SET) >= 0 &&
fxcrt::spanread(os2_data.span(), pFile).size() == os2_loc->size) {Looks like lines 214-222. Refactor into helper?
Done
// TODO(tsepez): iterate over span.Did you want to fulfill this in FindFontTable()?
moved comment.
pdfium::span<const uint8_t> GetFontTable(pdfium::span<const uint8_t> font_data,Only called from unit tests?
Done
Has no unit test coverage.
called from GetFontTable().
Moved to tests.
Might as well update C++ style since it's now in a different file and looks like new code.
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Commit-Queue | +1 |
2 comments
pdfium::span<const uint8_t> GetFontTable(pdfium::span<const uint8_t> font_data,This helper duplicates logic found in non-test code (`cfx_folderfontinfo.cpp`):\n1. Parsing `nTables` from the font header.\n2. Checking table payload bounds against the data size (similar to `DataVectorAtLocation`).\n\nSince `CFX_FolderFontInfo` streams the font file from disk rather than loading it entirely into a memory span, we cannot easily share a common `GetFontTable` helper. However, we can avoid this duplication entirely by deleting this helper and testing `FindFontTableLocation` directly.
TEST(FXFontTest, GetFontTable) {If we drop the `GetFontTable` helper, we can simplify this test to evaluate `FindFontTableLocation` directly. We only need to pass a mock table directory array rather than a full TTF header:\n\n```cpp\nTEST(FXFontTest, FindFontTableLocation) {\n std::vector<uint8_t> table_dir = {\n // Table Directory Entry 1 ('head')\n 'h', 'e', 'a', 'd', 0x00, 0x00, 0x00, 0x00, // checksum\n 0x00, 0x00, 0x00, 0x2C, // offset = 44\n 0x00, 0x00, 0x00, 0x0A, // length = 10\n\n // Table Directory Entry 2 ('name')\n 'n', 'a', 'm', 'e', 0x00, 0x00, 0x00, 0x00, // checksum\n 0x00, 0x00, 0x00, 0x36, // offset = 54\n 0x00, 0x00, 0x00, 0x14, // length = 20\n };\n\n uint32_t head_tag = CFX_FontMapper::MakeTag('h', 'e', 'a', 'd');\n std::optional<FontTableLocation> head_loc = FindFontTableLocation(table_dir, head_tag);\n ASSERT_TRUE(head_loc.has_value());\n EXPECT_EQ(44u, head_loc->offset);\n EXPECT_EQ(10u, head_loc->size);\n\n // Add similar tests for the 'name' tag, 'gasp' tag (not found), and a truncated `table_dir` span.\n}\n```
Related details
- Lei Zhang
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
2 comments
Introduce generic FindFontTableLocation() helper functionssingular, also line 11.
constexpr uint32_t name_tag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');Could have remained as kFoo.
Related details
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 2 comments![]( "Open in Gerrit")
Votes added by Tom Sepez
| Commit-Queue | +2 |
2 comments
Introduce generic FindFontTableLocation() helper functionsTom Sepezsingular, also line 11.
Done
constexpr uint32_t name_tag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');Could have remained as kFoo.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: core/fxge/cfx_folderfontinfo.cpp
Insertions: 2, Deletions: 2.
@@ -222,9 +222,9 @@
return;
}
- constexpr uint32_t name_tag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');
+ constexpr uint32_t kNameTag = CFX_FontMapper::MakeTag('n', 'a', 'm', 'e');
std::optional<FontTableLocation> loc =
- FindFontTableLocation(tables.unsigned_span(), name_tag);
+ FindFontTableLocation(tables.unsigned_span(), kNameTag);
if (!loc) {
return;
}
```
Change information
Refactor manual font table parsing in CFX_FolderFontInfo
Introduce generic FindFontTableLocation() helper function
in fx_font.h / fx_font.cpp to parse TrueType/OpenType table
directories. This helper replaces duplicated and unsafe manual
parsing logic in `CFX_FolderFontInfo` when scanning system fonts and
retrieving font data.
-- Remove one UNSAFE_TODO from the code.
-- Back-fill unit tests for in `fx_font_unittest.cpp`.
TAG=agy
CONV=5b35af9a-640b-4f89-b828-d1d410d70943
- M core/fxge/cfx_folderfontinfo.cpp
- M core/fxge/cfx_folderfontinfo.h
- M core/fxge/fx_font.cpp
- M core/fxge/fx_font.h
- M core/fxge/fx_font_unittest.cpp
Code-Review: +1 by Lei Zhang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |