Refactor Skrifa integration to use member methods. [pdfium : main]
0 views
Skip to first unread message
Tom Sepez (Gerrit)
Jun 22, 2026, 11:31:34 AM (3 days ago) Jun 22
to Lei Zhang, Chad Brokaw, Dominik Röttsches, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw, Dominik Röttsches and Lei Zhang
New activity on the change![]( "Open in Gerrit")
Related details
Attention is currently required from:
- Chad Brokaw
- Dominik Röttsches
- Lei Zhang
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Jun 22, 2026, 7:39:34 PM (2 days ago) Jun 22
to Tom Sepez, Lei Zhang, Chad Brokaw, Dominik Röttsches, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw, Dominik Röttsches and Tom Sepez
Lei Zhang added 6 comments![]( "Open in Gerrit")
File core/fxge/cfx_face.h
Line 27, Patchset 12 (Latest):
#endif // defined(PDF_USE_SKIA)Lei Zhang . unresolved
Just leave this out and avoid struggling with the auto-formatter.
File core/fxge/cfx_face.cpp
Line 369, Patchset 12 (Latest):
const char* skrifa_format = "";Lei Zhang . unresolved
Assign in the default case instead?
Line 900, Patchset 12 (Latest):
static std::unique_ptr<CFX_Path> ConvertOutline(Lei Zhang . unresolved
Move into anonymous namespace.
File core/fxge/skrifa/src/outlines.cpp
Line 31, Patchset 12 (Latest):
reinterpret_cast<const uint8_t*>(bytes.data()), bytes.size());Lei Zhang . unresolved
Can this just take `bytes` as 1 argument like fx_skrifa_unittest.cpp?
Line 41, Patchset 12 (Latest):
auto name = std::string(font->postscript_name());Lei Zhang . unresolved
Can this be: `std::string name(...);` ?
Related details
Attention is currently required from:
- Chad Brokaw
- Dominik Röttsches
- Tom Sepez
Submit Requirements:
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Jun 22, 2026, 7:42:24 PM (2 days ago) Jun 22
to Tom Sepez, Lei Zhang, Chad Brokaw, Dominik Röttsches, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw, Dominik Röttsches and Tom Sepez
Lei Zhang added 2 comments![]( "Open in Gerrit")
Commit Message
Line 19, Patchset 12 (Latest):
Change-Id: Ib0f280596579ba702194e3bb73e5b7cd2ac8d32eLei Zhang . unresolved
Add Bug:
File core/fxge/cfx_face.cpp
Line 1392, Patchset 12 (Latest):
[[maybe_unused]] std::unique_ptr<SkrifaFontHolder> skrifa_font)Lei Zhang . unresolved
Is this needed? What build config cares if the parameter is unused?
Dominik Röttsches (Gerrit)
Jun 23, 2026, 2:57:37 AM (2 days ago) Jun 23
to Tom Sepez, Lei Zhang, Chad Brokaw, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw and Tom Sepez
Dominik Röttsches voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Röttsches
| Code-Review | +1 |
2 comments
Patchset-level comments
File-level comment, Patchset 12 (Latest):
Dominik Röttsches . resolved
LGTM, with Lei's feedback addressed. One question on the wrapper type below.
File core/fxge/cfx_face.cpp
Line 298, Patchset 12 (Latest):
struct SkrifaFontHolder {Dominik Röttsches . unresolved
No objections to using a wrapper type, but would it be enought to use a typedef to `rust::Box<skrifa::SkrifaFont`?
Related details
Attention is currently required from:
- Chad Brokaw
- Tom Sepez
Submit Requirements:
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)
Jun 23, 2026, 12:07:57 PM (2 days ago) Jun 23
to Dominik Röttsches, Lei Zhang, Chad Brokaw, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw and Lei Zhang
Tom Sepez added 9 comments![]( "Open in Gerrit")
Commit Message
Line 19, Patchset 12:
Change-Id: Ib0f280596579ba702194e3bb73e5b7cd2ac8d32eLei Zhang . resolved
Tom SepezAdd Bug:
Done
File core/fxge/cfx_face.h
Just leave this out and avoid struggling with the auto-formatter.
Tom Sepez
Done
File core/fxge/cfx_face.cpp
No objections to using a wrapper type, but would it be enought to use a typedef to `rust::Box<skrifa::SkrifaFont`?
Tom Sepez
This helps when building without Fontations, eg. no rust:: symbols as we just have a dummy struct in that case.
Line 326, Patchset 12 (Parent):
// Private ctor.Lei Zhang . resolved
Tom SepezPreserve comment.
Done
Assign in the default case instead?
Tom Sepez
Done
Line 900, Patchset 12:
static std::unique_ptr<CFX_Path> ConvertOutline(Lei Zhang . resolved
Tom SepezMove into anonymous namespace.
Done
Line 1392, Patchset 12:
[[maybe_unused]] std::unique_ptr<SkrifaFontHolder> skrifa_font)Lei Zhang . resolved
Is this needed? What build config cares if the parameter is unused?
Tom Sepez
Think it was a bot suggestion. Namely that although we always have this parameter in the constructor to avoid ugly ifdef's, it matches up with a member that's ifdef'd out in some cases.
File core/fxge/skrifa/src/outlines.cpp
Line 31, Patchset 12:
reinterpret_cast<const uint8_t*>(bytes.data()), bytes.size());Lei Zhang . resolved
Can this just take `bytes` as 1 argument like fx_skrifa_unittest.cpp?
Tom Sepez
Done
Can this be: `std::string name(...);` ?
Attention is currently required from:
- Chad Brokaw
- Lei Zhang
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Jun 23, 2026, 12:08:16 PM (2 days ago) Jun 23
to Dominik Röttsches, Lei Zhang, Chad Brokaw, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com
Attention needed from Chad Brokaw and Lei Zhang
Tom Sepez voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
Jun 23, 2026, 1:09:36 PM (2 days ago) Jun 23
to Tom Sepez, Dominik Röttsches, Lei Zhang, Chad Brokaw, pdfium-...@googlegroups.com
pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: core/fxge/skrifa/src/outlines.cpp
Insertions: 4, Deletions: 5.
@@ -23,12 +23,11 @@
// Load the font data
std::string path_str(font_path.data(), font_path.size());
std::ifstream input(path_str, std::ios::binary);
- std::vector<char> bytes((std::istreambuf_iterator<char>(input)),
- (std::istreambuf_iterator<char>()));
+ std::vector<uint8_t> bytes((std::istreambuf_iterator<char>(input)),
+ (std::istreambuf_iterator<char>()));
input.close();
- rust::Slice<const uint8_t> slice(
- reinterpret_cast<const uint8_t*>(bytes.data()), bytes.size());
+ rust::Slice<const uint8_t> slice(bytes);
// Load the font. Note that the bytes vector must live as long as the font
auto font = skrifa::new_font(slice, 0);
@@ -38,7 +37,7 @@
}
// Get the PostScript name
- auto name = std::string(font->postscript_name());
+ std::string name(font->postscript_name());
// And the family name
auto family_name = std::string(font->family_name());
```
```
The name of the file: core/fxge/cfx_face.h
Insertions: 1, Deletions: 1.
@@ -24,7 +24,7 @@
#if defined(PDF_USE_SKIA)
#include "third_party/skia/include/core/SkRefCnt.h" // nogncheck
-#endif // defined(PDF_USE_SKIA)
+#endif
#if defined(PDF_ENABLE_FONTATIONS)
#include "third_party/rust/cxx/v1/cxx.h"
```
```
The name of the file: core/fxge/cfx_face.cpp
Insertions: 59, Deletions: 60.
@@ -172,6 +172,62 @@
return 0;
}
+#if defined(PDF_ENABLE_FONTATIONS)
+std::unique_ptr<CFX_Path> ConvertOutline(const skrifa::Outline& outline) {
+ auto skrifa_path = std::make_unique<CFX_Path>();
+ auto point_idx = 0;
+ CFX_PointF current_point(0, 0);
+ for (auto verb : outline.verbs) {
+ switch (verb) {
+ case skrifa::PathVerb::MoveTo: {
+ auto p = outline.points[point_idx++];
+ current_point = CFX_PointF(p.x, p.y);
+ skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kMove);
+ break;
+ }
+ case skrifa::PathVerb::LineTo: {
+ auto p = outline.points[point_idx++];
+ current_point = CFX_PointF(p.x, p.y);
+ skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kLine);
+ break;
+ }
+ case skrifa::PathVerb::QuadTo: {
+ auto c0 = outline.points[point_idx++];
+ auto p = outline.points[point_idx++];
+ // Convert quadratic to cubic bezier to match FreeType
+ // decomposition.
+ skrifa_path->AppendPoint(
+ CFX_PointF(current_point.x + (c0.x - current_point.x) * 2 / 3,
+ current_point.y + (c0.y - current_point.y) * 2 / 3),
+ CFX_Path::Point::Type::kBezier);
+ skrifa_path->AppendPoint(
+ CFX_PointF(c0.x + (p.x - c0.x) / 3, c0.y + (p.y - c0.y) / 3),
+ CFX_Path::Point::Type::kBezier);
+ current_point = CFX_PointF(p.x, p.y);
+ skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kBezier);
+ break;
+ }
+ case skrifa::PathVerb::CurveTo: {
+ auto c0 = outline.points[point_idx++];
+ auto c1 = outline.points[point_idx++];
+ auto p = outline.points[point_idx++];
+ skrifa_path->AppendPoint(CFX_PointF(c0.x, c0.y),
+ CFX_Path::Point::Type::kBezier);
+ skrifa_path->AppendPoint(CFX_PointF(c1.x, c1.y),
+ CFX_Path::Point::Type::kBezier);
+ current_point = CFX_PointF(p.x, p.y);
+ skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kBezier);
+ break;
+ }
+ case skrifa::PathVerb::Close:
+ skrifa_path->ClosePath();
+ break;
+ }
+ }
+ return skrifa_path;
+}
+#endif // defined(PDF_ENABLE_FONTATIONS)
+
FT_Encoding ToFTEncoding(fxge::FontEncoding encoding) {
switch (encoding) {
case fxge::FontEncoding::kAdobeCustom:
@@ -300,8 +356,6 @@
: font(std::move(f)) {}
rust::Box<skrifa::SkrifaFont> font;
};
-
-static std::unique_ptr<CFX_Path> ConvertOutline(const skrifa::Outline& outline);
#endif // defined(PDF_ENABLE_FONTATIONS)
// static
@@ -329,6 +383,7 @@
}
#endif
+ // Private ctor.
RetainPtr<CFX_Face> result = pdfium::WrapRetain(
new CFX_Face(std::move(cache_entry), std::move(font_stream), face_rec,
std::move(skrifa_font)));
@@ -366,7 +421,7 @@
#if defined(PDF_ENABLE_SKIA_TYPEFACE_CHECKS)
#if defined(PDF_ENABLE_FONTATIONS)
if (skrifa_font_ && skrifa_font_->font->is_ok()) {
- const char* skrifa_format = "";
+ const char* skrifa_format;
switch (skrifa_font_->font->font_type()) {
case skrifa::FaceFormat::TrueType:
skrifa_format = "TrueType";
@@ -378,6 +433,7 @@
skrifa_format = "CFF";
break;
default:
+ skrifa_format = "";
break;
}
CHECK_EQ(ft_result, ByteString(skrifa_format));
@@ -896,63 +952,6 @@
return pPath;
}
-#if defined(PDF_ENABLE_FONTATIONS)
-static std::unique_ptr<CFX_Path> ConvertOutline(
- const skrifa::Outline& outline) {
- auto skrifa_path = std::make_unique<CFX_Path>();
- auto point_idx = 0;
- CFX_PointF current_point(0, 0);
- for (auto verb : outline.verbs) {
- switch (verb) {
- case skrifa::PathVerb::MoveTo: {
- auto p = outline.points[point_idx++];
- current_point = CFX_PointF(p.x, p.y);
- skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kMove);
- break;
- }
- case skrifa::PathVerb::LineTo: {
- auto p = outline.points[point_idx++];
- current_point = CFX_PointF(p.x, p.y);
- skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kLine);
- break;
- }
- case skrifa::PathVerb::QuadTo: {
- auto c0 = outline.points[point_idx++];
- auto p = outline.points[point_idx++];
- // Convert quadratic to cubic bezier to match FreeType
- // decomposition.
- skrifa_path->AppendPoint(
- CFX_PointF(current_point.x + (c0.x - current_point.x) * 2 / 3,
- current_point.y + (c0.y - current_point.y) * 2 / 3),
- CFX_Path::Point::Type::kBezier);
- skrifa_path->AppendPoint(
- CFX_PointF(c0.x + (p.x - c0.x) / 3, c0.y + (p.y - c0.y) / 3),
- CFX_Path::Point::Type::kBezier);
- current_point = CFX_PointF(p.x, p.y);
- skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kBezier);
- break;
- }
- case skrifa::PathVerb::CurveTo: {
- auto c0 = outline.points[point_idx++];
- auto c1 = outline.points[point_idx++];
- auto p = outline.points[point_idx++];
- skrifa_path->AppendPoint(CFX_PointF(c0.x, c0.y),
- CFX_Path::Point::Type::kBezier);
- skrifa_path->AppendPoint(CFX_PointF(c1.x, c1.y),
- CFX_Path::Point::Type::kBezier);
- current_point = CFX_PointF(p.x, p.y);
- skrifa_path->AppendPoint(current_point, CFX_Path::Point::Type::kBezier);
- break;
- }
- case skrifa::PathVerb::Close:
- skrifa_path->ClosePath();
- break;
- }
- }
- return skrifa_path;
-}
-#endif // defined(PDF_ENABLE_FONTATIONS)
-
int CFX_Face::GetGlyphTTWidth() const {
const auto* fontglyph = GetRec()->glyph;
const int ft_result =
```
Change information
Commit message:
Refactor Skrifa integration to use member methods.
Migrate from free functions that take font data slices to member
methods on SkrifaFont. This allows caching the parsed font state and
prevents repeated passing of raw data slices across the FFI boundary.
Update CFX_Face to manage the SkrifaFont lifecycle and delegate
queries to it.
TAG=agy
CONV=53beacdb-cb1c-42a8-ae3f-b599eabe37b6
Bug: 42271123
Change-Id: Ib0f280596579ba702194e3bb73e5b7cd2ac8d32e
Commit-Queue: Tom Sepez <tse...@chromium.org>
Reviewed-by: Dominik Röttsches <dr...@chromium.org>
Files:
- M core/fxge/cfx_face.cpp
- M core/fxge/cfx_face.h
- M core/fxge/skrifa/fx_skrifa_unittest.cpp
- M core/fxge/skrifa/src/main.rs
- M core/fxge/skrifa/src/outlines.cpp
Change size: L
Delta: 5 files changed, 446 insertions(+), 375 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Dominik Röttsches
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Jun 23, 2026, 1:35:12 PM (2 days ago) Jun 23
to pdfium...@luci-project-accounts.iam.gserviceaccount.com, Tom Sepez, Dominik Röttsches, Lei Zhang, Chad Brokaw, pdfium-...@googlegroups.com
Lei Zhang added 1 comment![]( "Open in Gerrit")
File core/fxge/skrifa/src/outlines.cpp
Line 43, Patchset 15 (Latest):
auto family_name = std::string(font->family_name());Lei Zhang . resolved
Same as line 40. Another time...
Related details
Attention set is empty
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages