Simplify CFX_StandardFont standard font lookup [pdfium : main]
Tom Sepez (Gerrit)
Tom Sepez added 4 comments![]( "Open in Gerrit")
return stock_fonts_[static_cast<size_t>(index)];The commit message calls `CFX_StandardFont::ID` a "scoped enum", but it is actually unscoped (`enum ID : uint8_t`). Because of this, the `static_cast<size_t>` here isn't strictly necessary for array indexing (it would implicitly convert), though it is harmless and required for the `CHECK_LT` above.
CFX_StandardFont::GetStandardFontName(&family);This completely ignores the returned `std::optional<ID>` and only relies on the side effect of mutating `family` to the canonical name. This isn't a new bug (the old code did this too), but it highlights why the `GetStandardFontName` name is confusing.
static std::optional<ID> GetStandardFontIndex(const ByteString& name);Since this returns `std::optional<ID>`, `GetStandardFontId` or `FindStandardFontId` might be a more accurate name than `...Index`.
static std::optional<ID> GetStandardFontName(ByteString* name);Consider renaming this to something like `ResolveStandardFontAlias` or `CanonicalizeStandardFontName`. The current name implies a simple getter, but it actually takes an alias, mutates the string to the canonical name if found, and returns the ID.
Related details
- 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)
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 1 comment![]( "Open in Gerrit")
enum Index : uint8_t {Can a separate CL first do this mass rename?
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 1 comment![]( "Open in Gerrit")
Can a separate CL first do this mass rename?
- 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 2 comments![]( "Open in Gerrit")
static pdfium::span<const uint8_t> GetGenericSansFontData();Wishing there was less churn here, but ok.
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. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
* Move kFoxitFonts to cfx_standardfont.cpp.No longer part of this CL.
return font == CFX_StandardFont::kSymbol ||Leave these as they were?
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. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
return !subst_font->IsActualFontLoaded(base_font_name);Not GetCanonicalFontName() call before using `base_font_name` here? Or inline GetCanonicalFontName() call here.
fontname = CFX_StandardFont::GetCanonicalFontName(font_id.value());Maybe merge into line 297?
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 6 comments![]( "Open in Gerrit")
No longer part of this CL.
Done
return !subst_font->IsActualFontLoaded(base_font_name);Not GetCanonicalFontName() call before using `base_font_name` here? Or inline GetCanonicalFontName() call here.
Here is a place the split makes sense. If it is a standard font, the name is updated, but a value is returned, and we return out of here not using the name again at 247. Otherwise, it was not a standard font, and the name is not altered as a side-effect. In which case we use the non-adjusted name
fontname = CFX_StandardFont::GetCanonicalFontName(font_id.value());Maybe merge into line 297?
for the sake of update, and since I seemed to have lost one already, keep here.
Tom SepezLeave this alone.
jetski and blank lines.
static pdfium::span<const uint8_t> GetGenericSansFontData();Wishing there was less churn here, but ok.
I put these in order of .cpp after trying to minimize .cpp diff.
return font == CFX_StandardFont::kSymbol ||Leave these as they were?
- 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
return !subst_font->IsActualFontLoaded(base_font_name);Tom SepezNot GetCanonicalFontName() call before using `base_font_name` here? Or inline GetCanonicalFontName() call here.
Here is a place the split makes sense. If it is a standard font, the name is updated, but a value is returned, and we return out of here not using the name again at 247. Otherwise, it was not a standard font, and the name is not altered as a side-effect. In which case we use the non-adjusted name
Oh, I read line 246 wrong. I was expected the opposite conditional there.
if (base14_font_.has_value()) {It turns out IsBase14Font() internally does this too. So this code expands out to:
```
if (base14_font_.has_value()) {
... GetCanonicalFontName(base14_font_.value());
}
if (!base14_font_.has_value()) {
return LoadCommon();
}
```
Maybe just switch out the IsBase14Font() call and keep the early return?
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 1 comment![]( "Open in Gerrit")
Votes added by Tom Sepez
| Commit-Queue | +2 |
1 comment
It turns out IsBase14Font() internally does this too. So this code expands out to:
```
if (base14_font_.has_value()) {
... GetCanonicalFontName(base14_font_.value());
}
if (!base14_font_.has_value()) {
return LoadCommon();
}
```Maybe just switch out the IsBase14Font() call and keep the early return?
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
26 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: core/fpdfapi/font/cpdf_type1font.cpp
Insertions: 3, Deletions: 5.
@@ -85,13 +85,11 @@
bool CPDF_Type1Font::Load() {
base14_font_ = CFX_StandardFont::GetStandardFontIndex(base_font_name_);
- if (base14_font_.has_value()) {
- base_font_name_ =
- CFX_StandardFont::GetCanonicalFontName(base14_font_.value());
- }
- if (!IsBase14Font()) {
+ if (!base14_font_.has_value()) {
return LoadCommon();
}
+ base_font_name_ =
+ CFX_StandardFont::GetCanonicalFontName(base14_font_.value());
RetainPtr<const CPDF_Dictionary> font_desc =
font_dict_->GetDictFor("FontDescriptor");
```
Change information
Simplify CFX_StandardFont standard font lookup
* Replace GetStandardFontName(ByteString*), which mutated its input
argument, with a non-mutating GetStandardFontIndex() method.
* Callers that need the canonical standard font name must now explicitly
call GetCanonicalFontName() rather than relying on a side-effect.
* Update affected call sites across fpdfapi/ and fxge/.
TAG=agy
CONV=63532648-374f-4a87-acdf-e5aebbaa70a3
- M core/fpdfapi/font/cpdf_font.cpp
- M core/fpdfapi/font/cpdf_stockfontarray.cpp
- M core/fpdfapi/font/cpdf_type1font.cpp
- M core/fpdfapi/page/cpdf_docpagedata.cpp
- M core/fxge/cfx_fontmapper.cpp
- M core/fxge/cfx_standardfont.cpp
- M core/fxge/cfx_standardfont.h
- M core/fxge/cfx_standardfont_unittest.cpp
Code-Review: +1 by Lei Zhang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |