[PDF Ink Signatures] Add WebFormControlElement::GetTextInfo() [chromium/src : main]
AI Code Reviewer (Gerrit)
AI Code Reviewer added 5 comments![]( "Open in Gerrit")
std::optional<std::vector<TextInfo>> GetTextInfo() const;Blink Style Guide: Prefer blink:: types over STL and base types. In the public API, consider using `WebVector<TextInfo>` instead of `std::vector<TextInfo>`.
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)_
std::vector<uint16_t> glyphs;Blink Style Guide: Prefer blink:: types over STL and base types. In the public API, consider using `WebVector<uint16_t>` instead of `std::vector<uint16_t>`.
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)_
std::vector<WebFormControlElement::TextInfo> GetTextInfo() const;Blink Style Guide: Prefer blink:: types over STL and base types. Use `WTF::Vector` instead of `std::vector` in `core/` code.
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)_
auto& results =Blink Style Guide: Prefer blink:: types over STL and base types. Use `WTF::Vector` instead of `std::vector`.
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)_
std::vector<WebFormControlElement::TextInfo> HTMLTextAreaElement::GetTextInfo()Blink Style Guide: Prefer blink:: types over STL and base types. Use `WTF::Vector` instead of `std::vector` for the return type.
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
- Ian Kilpatrick
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")
SetBodyContent("<textarea id=test>ai\nis 爱\n</textarea>");Added a basic test here. The result is currently:
`["aiis ", "爱"]`
but maybe the result needs to be split into:
`[["ai"], ["is ", "爱"]]`.
Not 100% sure what we need yet, but this is roughly the direction this CL is going in.
Related details
- Ian Kilpatrick
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")
Also, would it be ok to exclude all this code from non-Desktop platforms and sidestep the red Fuchsia bot which probably does font matching differently.
Dominik Röttsches (Gerrit)
Dominik Röttsches added 1 comment![]( "Open in Gerrit")
shape_result_view->ForEachGlyph(/*initial_advance=*/0.0f,Approach seems correct to me, thanks for your patience and reworking this.
Related details
- Ian Kilpatrick
- 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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Does this patch replace the other patches that were out for review?
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")
Does this patch replace the other patches that were out for review?
I've abandoned all my other CLs, so this replaces my CLs where you were CC'd. I can't speak for https://crrev.com/c/7597894 with 100% certainty, but I imagine it will change if it gets built on top of this CL.
Related details
- Ian Kilpatrick
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Lei ZhangDoes this patch replace the other patches that were out for review?
I've abandoned all my other CLs, so this replaces my CLs where you were CC'd. I can't speak for https://crrev.com/c/7597894 with 100% certainty, but I imagine it will change if it gets built on top of this CL.
Can you merge https://crrev.com/c/7597894 and this into one patch?
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. |
April Kallmeyer (Gerrit)
April Kallmeyer added 1 comment![]( "Open in Gerrit")
Lei ZhangDoes this patch replace the other patches that were out for review?
Ian KilpatrickI've abandoned all my other CLs, so this replaces my CLs where you were CC'd. I can't speak for https://crrev.com/c/7597894 with 100% certainty, but I imagine it will change if it gets built on top of this CL.
Can you merge https://crrev.com/c/7597894 and this into one patch?
I'm working on this merge. I think it will ideally end up being two separate CLs.
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. |
April Kallmeyer (Gerrit)
April Kallmeyer added 1 comment![]( "Open in Gerrit")
Lei ZhangDoes this patch replace the other patches that were out for review?
Ian KilpatrickI've abandoned all my other CLs, so this replaces my CLs where you were CC'd. I can't speak for https://crrev.com/c/7597894 with 100% certainty, but I imagine it will change if it gets built on top of this CL.
April KallmeyerCan you merge https://crrev.com/c/7597894 and this into one patch?
I'm working on this merge. I think it will ideally end up being two separate CLs.
So far it looks like the API change will just be
```
struct TextInfo {
sk_sp<SkTypeface> typeface;
std::vector<uint16_t> glyphs;
+ std::vector<gfx::Vector2dF> glyph_offset;
+ gfx::RectF location;
};
```
with some more significant changes in the implementation.
April Kallmeyer (Gerrit)
April Kallmeyer added 1 comment![]( "Open in Gerrit")
Lei ZhangDoes this patch replace the other patches that were out for review?
Ian KilpatrickI've abandoned all my other CLs, so this replaces my CLs where you were CC'd. I can't speak for https://crrev.com/c/7597894 with 100% certainty, but I imagine it will change if it gets built on top of this CL.
April KallmeyerCan you merge https://crrev.com/c/7597894 and this into one patch?
April KallmeyerI'm working on this merge. I think it will ideally end up being two separate CLs.
So far it looks like the API change will just be
```
struct TextInfo {
sk_sp<SkTypeface> typeface;
std::vector<uint16_t> glyphs;
+ std::vector<gfx::Vector2dF> glyph_offset;
+ gfx::RectF location;
};
```with some more significant changes in the implementation.
Here is how it will look https://crrev.com/c/7632866 (further patchsets coming soon)
I think I might want to combine those 3 vectors into one vector of structs. Also I might clean up the temporary vector in the implementation.
I was able to test this end to end and get the text to line up exactly.
Related details
- Ian Kilpatrick
- 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")
shape_result_view->ForEachGlyph(/*initial_advance=*/0.0f,Approach seems correct to me, thanks for your patience and reworking this.
Acknowledged
Related details
- Ian Kilpatrick
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. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
ASSERT_EQ(1u, text_info[1].glyphs.size());If possible load in fonts with known glyphs. E.g. see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/canvas/text_metrics_test.cc;l=45-46;drc=3f7112dccdaf8ad032ee03247030eb43cfc8d706;bpv=1;bpt=1
Then if you list the font-family as Ahem, Roboto or similar it'll work the same on all platforms.
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")
If possible load in fonts with known glyphs. E.g. see: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/canvas/text_metrics_test.cc;l=45-46;drc=3f7112dccdaf8ad032ee03247030eb43cfc8d706;bpv=1;bpt=1
Then if you list the font-family as Ahem, Roboto or similar it'll work the same on all platforms.
- Ian Kilpatrick
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. |