Remove known emoji-sequence participating codepoints from CJK/Symbol set [chromium/src : main]
Dominik Röttsches (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Koji Ishii
- jj
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 1 comment![]( "Open in Gerrit")
These were removed by having gemini continuously run the newly added test and look at the failing overlapping ranges and removing them from here. Plus manual verification and adding descriptions for some of the individual codepoints that remained.
Related details
- Koji Ishii
- jj
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii voted and added 2 comments![]( "Open in Gerrit")
Votes added by Koji Ishii
| Code-Review | +1 |
2 comments
lgtm
Not sure adding tests helps us. If newer ICU has existing code points, we'll need to maintain the list, or add `#IF ICU_VERSION`. Do we want that?
There's no behavior changes, correct? Can you add to the CL message if so?
Related details
- Dominik Röttsches
- jj
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. |
jj (Gerrit)
jj voted and added 1 comment![]( "Open in Gerrit")
Votes added by jj
| Code-Review | +1 |
1 comment
lgtm, thanks for the cleanup!
Related details
- 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. |
Dominik Röttsches (Gerrit)
Dominik Röttsches voted and added 2 comments![]( "Open in Gerrit")
Votes added by Dominik Röttsches
| Commit-Queue | +2 |
lgtm
Not sure adding tests helps us. If newer ICU has existing code points, we'll need to maintain the list, or add `#IF ICU_VERSION`. Do we want that?
Thanks for the review.
Are you suggesting to add more tests? Or do you wonder whether the tests added in this change are helping in the face of ICU upgrades?
For the latter, I believe, it's an okay part of ICU roll work to remove ranges or codepoints that the constant arrays cover if ICU will contain those as emoji in the future. The process is relatively simple, as the tests will report which codepoints or ranges are already covered, and then they can be edited out of the manual list definitions.
(If needed, I'll follow-up with a follow-up change, and land this in the meantime, hope that's what you intended with your LGTM.)
There's no behavior changes, correct? Can you add to the CL message if so?
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 voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
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 1 comment![]( "Open in Gerrit")
Dominik Röttscheslgtm
Not sure adding tests helps us. If newer ICU has existing code points, we'll need to maintain the list, or add `#IF ICU_VERSION`. Do we want that?
Thanks for the review.
Are you suggesting to add more tests? Or do you wonder whether the tests added in this change are helping in the face of ICU upgrades?
For the latter, I believe, it's an okay part of ICU roll work to remove ranges or codepoints that the constant arrays cover if ICU will contain those as emoji in the future. The process is relatively simple, as the tests will report which codepoints or ranges are already covered, and then they can be edited out of the manual list definitions.
(If needed, I'll follow-up with a follow-up change, and land this in the meantime, hope that's what you intended with your LGTM.)
PS: One such test was added in the parent change by JJ,
https://crrev.com/c/7747465/9/third_party/blink/renderer/platform/fonts/plain_text_node_test.cc - perhaps that's what you meant?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Koji Ishii (Gerrit)
Koji Ishii voted and added 1 comment![]( "Open in Gerrit")
Votes added by Koji Ishii
| Code-Review | +1 |
Dominik Röttscheslgtm
Not sure adding tests helps us. If newer ICU has existing code points, we'll need to maintain the list, or add `#IF ICU_VERSION`. Do we want that?
Dominik RöttschesThanks for the review.
Are you suggesting to add more tests? Or do you wonder whether the tests added in this change are helping in the face of ICU upgrades?
For the latter, I believe, it's an okay part of ICU roll work to remove ranges or codepoints that the constant arrays cover if ICU will contain those as emoji in the future. The process is relatively simple, as the tests will report which codepoints or ranges are already covered, and then they can be edited out of the manual list definitions.
(If needed, I'll follow-up with a follow-up change, and land this in the meantime, hope that's what you intended with your LGTM.)
PS: One such test was added in the parent change by JJ,
https://crrev.com/c/7747465/9/third_party/blink/renderer/platform/fonts/plain_text_node_test.cc - perhaps that's what you meant?
Sorry my words were not clear. I meant not to add duplications tests, as it makes the array dependent on specific ICU versions, but ok if you prefer that.
Related details
- Dominik Röttsches
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |