[PDF Ink Signatures] Add WebFormControlElement::GetFontsForText() [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 1 comment![]( "Open in Gerrit")
FontFallbackIterator::HintCharList hint_list;Blink Style Guide: Prefer blink:: types. Iterating directly over 'String' yields 16-bit 'UChar's, not 'UChar32' code points. This will split surrogate pairs and likely break font lookup for non-BMP characters. Since you included 'code_point_iterator.h', consider using 'WTF::CodePointIterator(value_)' to correctly iterate over code points.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
April Kallmeyer (Gerrit)
April Kallmeyer added 1 comment![]( "Open in Gerrit")
results.push_back(actual_font->PlatformData().FontFamilyName());Is this enough information to uniquely identify the font? It's possible to get the SkTypeface here and that has a serialize method.
I don't know enough about fonts to know if family is enough.
Related details
- Lei Zhang
Code-Coverage
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 1 comment![]( "Open in Gerrit")
results.push_back(actual_font->PlatformData().FontFamilyName());Is this enough information to uniquely identify the font? It's possible to get the SkTypeface here and that has a serialize method.
I don't know enough about fonts to know if family is enough.
In my local build, I can print out the font names and they look sufficient. I haven't tried sending the font names to the backend and then actually retrieving the font data.
Related details
- April Kallmeyer
Code-Coverage
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 1 comment![]( "Open in Gerrit")
results.push_back(actual_font->PlatformData().FontFamilyName());Lei ZhangIs this enough information to uniquely identify the font? It's possible to get the SkTypeface here and that has a serialize method.
I don't know enough about fonts to know if family is enough.
In my local build, I can print out the font names and they look sufficient. I haven't tried sending the font names to the backend and then actually retrieving the font data.
If I toggle bold and/or italic, the font family name stays the same, e.g. "Arial", while the PostScript name changes. e.g. ArialMT, Arial-BoldMT, ArialBoldItalicMT and Arial-ItalicMT.
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
results.push_back(actual_font->PlatformData().FontFamilyName());Lei ZhangIs this enough information to uniquely identify the font? It's possible to get the SkTypeface here and that has a serialize method.
I don't know enough about fonts to know if family is enough.
Lei ZhangIn my local build, I can print out the font names and they look sufficient. I haven't tried sending the font names to the backend and then actually retrieving the font data.
If I toggle bold and/or italic, the font family name stays the same, e.g. "Arial", while the PostScript name changes. e.g. ArialMT, Arial-BoldMT, ArialBoldItalicMT and Arial-ItalicMT.
I'm very tempted to just send the font data over. Then the PDF backend doesn't have to separately retrieve it and potentially get it wrong. But sending over MBs of font data for each <textarea> can be expensive in terms of memory usage.
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
Blink Style Guide: Prefer blink:: types. Iterating directly over 'String' yields 16-bit 'UChar's, not 'UChar32' code points. This will split surrogate pairs and likely break font lookup for non-BMP characters. Since you included 'code_point_iterator.h', consider using 'WTF::CodePointIterator(value_)' to correctly iterate over code points.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Invalid: blink::String::begin() returns CodePointIterator.
results.push_back(actual_font->PlatformData().FontFamilyName());Lei ZhangIs this enough information to uniquely identify the font? It's possible to get the SkTypeface here and that has a serialize method.
I don't know enough about fonts to know if family is enough.
Lei ZhangIn my local build, I can print out the font names and they look sufficient. I haven't tried sending the font names to the backend and then actually retrieving the font data.
Lei ZhangIf I toggle bold and/or italic, the font family name stays the same, e.g. "Arial", while the PostScript name changes. e.g. ArialMT, Arial-BoldMT, ArialBoldItalicMT and Arial-ItalicMT.
I'm very tempted to just send the font data over. Then the PDF backend doesn't have to separately retrieve it and potentially get it wrong. But sending over MBs of font data for each <textarea> can be expensive in terms of memory usage.
Should be ok, since later CLs will only ever send each font once.
Related details
- April Kallmeyer
Code-Coverage
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")
masonf@: Can I get an early review to make sure the blink/public/ changes are ok? I will add tests, assuming this is ok.
Related details
- April Kallmeyer
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
Thanks - this approach seems ok to me. But I'd be more comfortable if drott@ took a look also.
Related details
- April Kallmeyer
- Dominik Röttsches
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominik Röttsches (Gerrit)
Dominik Röttsches added 2 comments![]( "Open in Gerrit")
Vector<sk_sp<SkTypeface>> HTMLTextAreaElement::GetFontsForText() const {Do you need the fonts and text or do you only need the sequence of glyph ids plus font blobs? - If you only need glyph ids and font blobs, it might make more sense to extract this from the TextPainter and drawing commands.
const Font& font_ref = *ComputedStyleRef().GetFont();This re-runs the fallback logic even though this is already done for the text that has appeared on the screen. I think a better way to do this here would be to try and get access to the ShapeResult(View) of the text area and extract it from there, instead of reimplementing the fallback logic. But see comment above - if only the glyph stream is needed, interfacing with paint would be better.
Related details
- April Kallmeyer
- Lei Zhang
Code-Coverage
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 1 comment![]( "Open in Gerrit")
Vector<sk_sp<SkTypeface>> HTMLTextAreaElement::GetFontsForText() const {Do you need the fonts and text or do you only need the sequence of glyph ids plus font blobs? - If you only need glyph ids and font blobs, it might make more sense to extract this from the TextPainter and drawing commands.
I think glyph IDs plus font blobs can work. Do I have to instantiate new TextPainter objects here, or is there some other way to hook into the paint code?
Related details
- April Kallmeyer
- Dominik Röttsches
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |