Tom Sepez (Gerrit)

unread,

May 29, 2026, 7:45:21 PM (6 days ago) May 29

to Lei Zhang, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Lei Zhang

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:

Lei Zhang

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Lei Zhang (Gerrit)

unread,

May 29, 2026, 7:54:11 PM (6 days ago) May 29

to Tom Sepez, Lei Zhang, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Tom Sepez

Lei Zhang voted and added 2 comments

Votes added by Lei Zhang

Code-Review

+1

2 comments

File core/fxge/android/cfpf_skiafont.cpp

Line 14, Patchset 4 (Latest): : face_(std::move(face)), charset_(uCharset) {}

Lei Zhang . unresolved

Isn't `face_` always going to be non-null here? CHECK() that and delete the null check on line 19 and 27?

File core/fxge/android/cfpf_skiafontmgr.cpp

Line 389, Patchset 4 (Latest):std::unique_ptr<CFPF_SkiaFontMgr::Entry> CFPF_SkiaFontMgr::ReportFace(

Lei Zhang . unresolved

Pre-existing. Since ReportFace() never returns nullptr, I wonder if it can just return an Entry directly.

Open in Gerrit

Related details

Attention is currently required from:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Aryan Krishnan (Gerrit)

unread,

May 30, 2026, 3:47:15 AM (6 days ago) May 30

to Tom Sepez, Lei Zhang, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Tom Sepez

Aryan Krishnan added 2 comments

Commit Message

Line 9, Patchset 4 (Latest):Nest Entry privately in CFPF_SkiaFontMgr and pass CFX_Face directly to

Aryan Krishnan . unresolved

Lowercase E

Line 12, Patchset 4 (Latest):Bypass-Check-License: rename below in CL chain.

Aryan Krishnan . unresolved

Might as well rebase and remove it since all the other ones have landed.

Open in Gerrit

Related details

Attention is currently required from:

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.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 1, 2026, 1:20:08 PM (3 days ago) Jun 1

to Aryan Krishnan, Lei Zhang, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Aryan Krishnan

Tom Sepez added 4 comments

Commit Message

Line 9, Patchset 4:Nest Entry privately in CFPF_SkiaFontMgr and pass CFX_Face directly to

Aryan Krishnan . resolved

Lowercase E

Tom Sepez

Actually its a class, clarified.

Line 12, Patchset 4:Bypass-Check-License: rename below in CL chain.

Aryan Krishnan . resolved

Might as well rebase and remove it since all the other ones have landed.

Tom Sepez

Done.

File core/fxge/android/cfpf_skiafont.cpp

Line 14, Patchset 4: : face_(std::move(face)), charset_(uCharset) {}

Lei Zhang . resolved

Isn't `face_` always going to be non-null here? CHECK() that and delete the null check on line 19 and 27?

Tom Sepez

Done

File core/fxge/android/cfpf_skiafontmgr.cpp

Line 389, Patchset 4:std::unique_ptr<CFPF_SkiaFontMgr::Entry> CFPF_SkiaFontMgr::ReportFace(

Lei Zhang . resolved

Pre-existing. Since ReportFace() never returns nullptr, I wonder if it can just return an Entry directly.

Tom Sepez

std::vector<std::unique_ptr<Entry>> font_faces_ means that a resize is more expensive as it copies approx 64 bytes per entry.

Open in Gerrit

Related details

Attention is currently required from:

Aryan Krishnan

Submit Requirements:

Code-Owners
Code-Review

Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

Tom Sepez (Gerrit)

unread,

Jun 1, 2026, 1:20:21 PM (3 days ago) Jun 1

to Aryan Krishnan, Lei Zhang, pdfium...@luci-project-accounts.iam.gserviceaccount.com, pdfium-...@googlegroups.com

Attention needed from Aryan Krishnan

Tom Sepez voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention is currently required from:

Aryan Krishnan

Submit Requirements:

Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

open

diffy

pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

unread,

Jun 1, 2026, 2:00:59 PM (3 days ago) Jun 1

to Tom Sepez, Aryan Krishnan, Lei Zhang, pdfium-...@googlegroups.com

pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes

Unreviewed changes

4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: core/fxge/android/cfpf_skiafont.cpp
Insertions: 4, Deletions: 7.

@@ -8,24 +8,21 @@
 
 #include <utility>
 
+#include "core/fxcrt/check.h"
 #include "core/fxcrt/numerics/safe_conversions.h"
 
 CFPF_SkiaFont::CFPF_SkiaFont(RetainPtr<CFX_Face> face, FX_Charset uCharset)
-    : face_(std::move(face)), charset_(uCharset) {}
+    : face_(std::move(face)), charset_(uCharset) {
+  CHECK(face_);
+}
 
 CFPF_SkiaFont::~CFPF_SkiaFont() = default;
 
 ByteString CFPF_SkiaFont::GetFamilyName() {
-  if (!face_) {
-    return ByteString();
-  }
   return face_->GetFamilyName();
 }
 
 uint32_t CFPF_SkiaFont::GetFontData(uint32_t dwTable,
                                     pdfium::span<uint8_t> pBuffer) {
-  if (!face_) {
-    return 0;
-  }
   return pdfium::checked_cast<uint32_t>(face_->GetSfntTable(dwTable, pBuffer));
 }
```

Change information

Commit message:

[Android] Nest font metadata privately in manager

Replace CFPF_SkiaPathFont with a new nested class called `Entry` private
to CFPF_SkiaFontMgr, and pass CFX_Face directly to CFPF_SkiaFont. Delete
cfpf_skiapathfont.h and cfpf_skiapathfont.cpp.

Change-Id: Icff26018b9e02c480415c68b442e425a49c46b96

Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/148730

Commit-Queue: Tom Sepez <tse...@chromium.org>

Reviewed-by: Lei Zhang <the...@chromium.org>

Files:

M core/fxge/BUILD.gn
M core/fxge/android/cfpf_skiafont.cpp
M core/fxge/android/cfpf_skiafont.h
M core/fxge/android/cfpf_skiafontmgr.cpp
M core/fxge/android/cfpf_skiafontmgr.h
D core/fxge/android/cfpf_skiapathfont.cpp
D core/fxge/android/cfpf_skiapathfont.h

Change size: M

Delta: 7 files changed, 51 insertions(+), 118 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Lei Zhang

Open in Gerrit

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[Android] Nest font metadata privately in manager [pdfium : main]

Tom Sepez (Gerrit)

New activity on the change

Related details

Lei Zhang (Gerrit)

Lei Zhang voted and added 2 comments

Votes added by Lei Zhang

2 comments

Related details

Aryan Krishnan (Gerrit)

Aryan Krishnan added 2 comments

Related details

Tom Sepez (Gerrit)

Tom Sepez added 4 comments

Related details

Tom Sepez (Gerrit)

Tom Sepez voted Commit-Queue+2

Related details

pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)

pdfium...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes

Unreviewed changes

Change information