| Summary: | [HarfBuzz] Decomposed Vietnamese characters are rendered incorrectly | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||
| Component: | Platform | Assignee: | Fujii Hironori <Hironori.Fujii> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | annulen, bashi, commit-queue, fred.wang, mcatanzaro, mmaxfield, webkit-bug-importer, yoshiaki.jitsukawa | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 175580 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Fujii Hironori
2017-07-12 02:40:56 PDT
Created attachment 315221 [details]
test case
HarfBuzzShaper should normalize the input text before shaping. Actually, HarfBuzzShaper::setNormalizedBuffer does the task. https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaper.cpp?rev=219385#L233 But, this function hasn't called from anywhere since Bug 108077. Created attachment 315321 [details]
Patch
Created attachment 315322 [details]
vietnamese-nfd-actual.png
Created attachment 315323 [details]
vietnamese-nfd-expected.png
(In reply to Fujii Hironori from comment #2) > But, this function hasn't called from anywhere since Bug 108077. Not Bug 108077, but Bug 90951. It'd be great help if you can take a glance at my patch and give me your thoughts. The patch looks good to me. Please ask WebKit reviewers for actual review :) It's a great helpful. Thank you so much to have a time to review my patch. Oh wow, apparently it's font bug week or something! I have not the faintest clue if your patch is right or not. I'm encouraged that it fixes the problem and includes a layout test. I'm also encouraged to see that Kenichi likes your patch. I see that he wrote all this code, so that's good I suppose! Comment on attachment 315321 [details]
Patch
I'm giving r+ even though I don't understand the code, because I'm pretty sure we don't have any Igalia reviewers who might understand it. Myles, if you see any reason not to commit this, please say so.
I have a suspicion that this commit might affect a lot of test results, so we will need to watch the bots carefully after landing it. So we will have to try landing this another day.
(In reply to Michael Catanzaro from comment #10) > I have not the faintest clue if your patch is right or not. I'm encouraged > that it fixes the problem and includes a layout test. I'm also encouraged to > see that Kenichi likes your patch. I see that he wrote all this code, so > that's good I suppose! ๐ Comment on attachment 315321 [details]
Patch
OK, Myles posted a happy emojii, and the tree is back open, so we should be good to cq+ this tomorrow when we're available for watching the bots.
Comment on attachment 315321 [details] Patch Clearing flags on attachment: 315321 Committed r219504: <http://trac.webkit.org/changeset/219504> All reviewed patches have been landed. Closing bug. Looks like there's no difference in the test results. OK then. Thank you, Michael. I'll keep an eys on it during this weekend. I'll investigate new failures next Tuesday if any. (next Monday is Japan's national holiday) There were not any new failures! Oh actually there was a regression, I just missed it. :( It's imported/blink/fast/text/international/text-shaping-arabic-diffs.html: Expected image: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r220734%20(2623)/imported/blink/fast/text/international/text-shaping-arabic-diffs.html New image after this patch was committed: https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r220734%20(2623)/imported/blink/fast/text/international/text-shaping-arabic-actual.png Fujii, any ideas? Re-opened since this is blocked by bug 175580 Thank you for rolling back my change, Michael. I'm sorry for not noticing the regression. normalizeCharacters was using FontCascade::treatAsZeroWidthSpaceInComplexScript while normalizeSpacesAndMirrorChars is using FontCascade::treatAsZeroWidthSpace. Created attachment 318221 [details]
Patch
Careful, or you'll become a text layout expert! Is this patch ready for review? Comment on attachment 318221 [details]
Patch
EWS got green. finished testing in my env. It's ready for review now.
Comment on attachment 318221 [details] Patch Clearing flags on attachment: 318221 Committed r220797: <http://trac.webkit.org/changeset/220797> All reviewed patches have been landed. Closing bug. |
/r220734%20(2623)/imported/blink/fast/text/international/text-shaping-arabic-actual.png){kind=link}