Replace CHARSET_FLAG macros with CharsetFlags enum and Mask<> [pdfium : main]

[Aryan Krishnan (Gerrit)]

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan added 3 comments

File core/fxge/cfx_folderfontinfo.h

Line 24, Patchset 8: bool GB = false;

Lei Zhang . resolved

Make this consistently lowercase.

Aryan Krishnan

Thought GB (I assumed Great Britain) was an acronym and hence should be all-caps. Edited now.

Line 19, Patchset 8:struct CharsetFlags {

Lei Zhang . resolved

Can this be part of FontFaceInfo instead of being in the global namespace?

Aryan Krishnan

GetCharset() is in an anonymous namespace (not in the class), yet needs to use these variables. Moving this to the class just makes it fail. To address this am moving GetCharset() into the class as a private: method

Line 19, Patchset 8:struct CharsetFlags {

Lei Zhang . resolved

Have you considered using fxcrt::Mask?

Aryan Krishnan

Right, that would have worked better here. Done.

Open in Gerrit

Related details

Attention is currently required from:

• Andy Phan
• Lei Zhang
• Tom Sepez

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: pdfium

Gerrit-Branch: main

Gerrit-Change-Id: Iad4046d423162bdbf60f42f8c1528e53bee76238

Gerrit-Change-Number: 145670

Gerrit-PatchSet: 12

Gerrit-Owner: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Andy Phan <andy...@chromium.org>

Gerrit-Reviewer: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Lei Zhang <the...@chromium.org>

Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>

Gerrit-CC: Pdfium LUCI CQ <pdfium...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Lei Zhang <the...@chromium.org>

Gerrit-Attention: Tom Sepez <tse...@chromium.org>

Gerrit-Attention: Andy Phan <andy...@chromium.org>

Gerrit-Comment-Date: Wed, 08 Apr 2026 13:53:14 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Lei Zhang <the...@chromium.org>

[Aryan Krishnan (Gerrit)]

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan added 1 comment

File core/fxge/cfx_folderfontinfo.h

Line 19, Patchset 8:struct CharsetFlags {

Lei Zhang . resolved

Can this be part of FontFaceInfo instead of being in the global namespace?

Aryan Krishnan

GetCharset() is in an anonymous namespace (not in the class), yet needs to use these variables. Moving this to the class just makes it fail. To address this am moving GetCharset() into the class as a private: method

Aryan Krishnan

Edit, it can't be public in FontFaceInfo, had to make it private due to accessors outside of FontFaceInfo but in the file.

Open in Gerrit

Related details

Attention is currently required from:

• Andy Phan
• Lei Zhang
• Tom Sepez

Submit Requirements:

• Code-Owners
• Code-Review
• Review-Enforcement

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: pdfium

Gerrit-Branch: main

Gerrit-Change-Id: Iad4046d423162bdbf60f42f8c1528e53bee76238

Gerrit-Change-Number: 145670

Gerrit-PatchSet: 15

Gerrit-Owner: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Andy Phan <andy...@chromium.org>

Gerrit-Reviewer: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Lei Zhang <the...@chromium.org>

Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>

Gerrit-CC: Pdfium LUCI CQ <pdfium...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Lei Zhang <the...@chromium.org>

Gerrit-Attention: Tom Sepez <tse...@chromium.org>

Gerrit-Attention: Andy Phan <andy...@chromium.org>

Gerrit-Comment-Date: Wed, 08 Apr 2026 14:15:54 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Lei Zhang <the...@chromium.org>

Comment-In-Reply-To: Aryan Krishnan <aryankr...@gmail.com>

[Aryan Krishnan (Gerrit)]

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan added 1 comment

File core/fxge/cfx_folderfontinfo.h

Line 19, Patchset 8:struct CharsetFlags {

Lei Zhang . resolved

Can this be part of FontFaceInfo instead of being in the global namespace?

Aryan Krishnan

GetCharset() is in an anonymous namespace (not in the class), yet needs to use these variables. Moving this to the class just makes it fail. To address this am moving GetCharset() into the class as a private: method

Aryan Krishnan

Edit, it can't be public in FontFaceInfo, had to make it private due to accessors outside of FontFaceInfo but in the file.

Aryan Krishnan

Edit: replace public with private and private with public

Gerrit-Comment-Date: Wed, 08 Apr 2026 15:39:43 +0000

[Lei Zhang (Gerrit)]

Attention needed from Andy Phan, Aryan Krishnan and Tom Sepez

Lei Zhang added 3 comments

File core/fxge/cfx_folderfontinfo.h

Line 57, Patchset 15 (Latest): FX_Charset charset);

Lei Zhang . unresolved

Auto-format?

Line 24, Patchset 8: bool GB = false;

Lei Zhang . resolved

Make this consistently lowercase.

Aryan Krishnan

Thought GB (I assumed Great Britain) was an acronym and hence should be all-caps. Edited now.

Lei Zhang

"Shift JIS" was lowercased as well.

File core/fxge/cfx_folderfontinfo.cpp

Line 315, Patchset 15 (Latest):fxcrt::Mask<CFX_FolderFontInfo::FontFaceInfo::CharsetFlags>

Lei Zhang . unresolved

Should this return a singular value and not a Mask? Same for IsEligibleForFindFont().

Open in Gerrit

Related details

Attention is currently required from:

• Andy Phan
• Aryan Krishnan
• 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

Gerrit-MessageType: comment

Gerrit-Project: pdfium

Gerrit-Branch: main

Gerrit-Change-Id: Iad4046d423162bdbf60f42f8c1528e53bee76238

Gerrit-Change-Number: 145670

Gerrit-PatchSet: 15

Gerrit-Owner: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Andy Phan <andy...@chromium.org>

Gerrit-Reviewer: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Lei Zhang <the...@chromium.org>

Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>

Gerrit-CC: Pdfium LUCI CQ <pdfium...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Tom Sepez <tse...@chromium.org>

Gerrit-Attention: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Attention: Andy Phan <andy...@chromium.org>

Gerrit-Comment-Date: Fri, 10 Apr 2026 00:24:02 +0000

[Aryan Krishnan (Gerrit)]

Attention needed from Andy Phan, Lei Zhang and Tom Sepez

Aryan Krishnan added 2 comments

File core/fxge/cfx_folderfontinfo.h

Line 57, Patchset 15: FX_Charset charset);

Lei Zhang . resolved

Auto-format?

Aryan Krishnan

Done

File core/fxge/cfx_folderfontinfo.cpp

Line 315, Patchset 15:fxcrt::Mask<CFX_FolderFontInfo::FontFaceInfo::CharsetFlags>

Lei Zhang . resolved

Should this return a singular value and not a Mask? Same for IsEligibleForFindFont().

Aryan Krishnan

Done. Same for the test helper as well AddDummyFont().

Open in Gerrit

Related details

Attention is currently required from:

• Andy Phan
• Lei Zhang
• Tom Sepez

Submit Requirements:

• Code-Owners
• Code-Review

• Review-Enforcement

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

Gerrit

Gerrit-MessageType: comment

Gerrit-Project: pdfium

Gerrit-Branch: main

Gerrit-Change-Id: Iad4046d423162bdbf60f42f8c1528e53bee76238

Gerrit-Change-Number: 145670

Gerrit-PatchSet: 16

Gerrit-Owner: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Andy Phan <andy...@chromium.org>

Gerrit-Reviewer: Aryan Krishnan <aryankr...@gmail.com>

Gerrit-Reviewer: Lei Zhang <the...@chromium.org>

Gerrit-Reviewer: Tom Sepez <tse...@chromium.org>

Gerrit-CC: Pdfium LUCI CQ <pdfium...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Lei Zhang <the...@chromium.org>

Gerrit-Attention: Tom Sepez <tse...@chromium.org>

Gerrit-Attention: Andy Phan <andy...@chromium.org>

Gerrit-Comment-Date: Fri, 10 Apr 2026 04:54:55 +0000