Summary: | [Chromium] Combining Diacritical Marks (U+0300..) are not handled correctly | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Yusuke Sato <yusukes> | ||||||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | agl, dglazkov, eric, jshin | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Linux | ||||||||||||||
Attachments: |
|
Description
Yusuke Sato
2009-08-26 09:00:28 PDT
Adding agl to the bug, since he added complex text support to Linux. Created attachment 38619 [details]
proposed patch v1
Created attachment 38620 [details]
screenshot after applying the v1 patch
Comment on attachment 38619 [details] proposed patch v1 (I am not a WebKit reviewer): LGTM > - const unsigned m_startingX; // Offset in pixels of the first script run. > + unsigned m_startingX; // Offset in pixels of the first script run. Why make this non-const? Maybe I missed something, but I don't see that you need to mutate it anywhere. Oops, that's simply a typo. I'll attach v2 patch shortly. Thanks! Created attachment 38623 [details]
patch v2
Comment on attachment 38623 [details]
patch v2
(I am not a WebKit reviewer): LGTM
Comment on attachment 38623 [details]
patch v2
Two nits:
We can get rid of the delete() code by using:
OwnPtr<TextRun> m_normalizedRun;
OwnArrayPtr<UChar> m_normalizedBuffer;
Then you just set m_run = m_normalizedRun.get();
In fact, then it still could be a const TextRun&, it doesn't need to change to be a pointer (although it can be made one if you feel that's cleaner).
This should use early return:
316 if (U_SUCCESS(error)) {
if (U_FAILURE(error))
return;
Created attachment 38713 [details]
patch v3
Thanks for the review! > We can get rid of the delete() code by using: > OwnPtr<TextRun> m_normalizedRun; > OwnArrayPtr<UChar> m_normalizedBuffer; Done. > In fact, then it still could be a const TextRun&, it doesn't need to change to > be a pointer (although it can be made one if you feel that's cleaner). Changed it back to const TextRun&. > if (U_FAILURE(error)) > return; Done. Comment on attachment 38713 [details]
patch v3
Looks good.
For the future, no { } around single line ifs:
if (block == UBLOCK_COMBINING_DIACRITICAL_MARKS) {
307 return getNormalizedTextRun(originalRun);
308 }
Comment on attachment 38713 [details]
patch v3
Oh, actually... about testing? I assume this is already covered by existing layout tests? If not, wee need a test here. removing from the commit queue for now.
> For the future, no { } around single line ifs: I see. Thanks for the review. > I assume this is already covered by existing layout tests? Yes, fast/text/stroking.html and fast/text/stroking-decorations.html are. Though these tests are currently disabled on Chromium Linux, I'm going to enable them as soon as this change is submitted and merged into Chromium tree. Here is the Chromium change to do that: http://codereview.chromium.org/173564 (already LGTM'ed). Comment on attachment 38713 [details]
patch v3
Best to mention what tests this affects, even if they are already existing. Anyway, good enough for now, will land. cq+
Comment on attachment 38713 [details] patch v3 Clearing flags on attachment: 38713 Committed r47922: <http://trac.webkit.org/changeset/47922> All reviewed patches have been landed. Closing bug. + // Note that we don't use the icu::Normalizer::isNormalized(UNORM_NFC) API here since + // the API returns FALSE (= not normalized) for complex runs that don't require NFC + // normalization (e.g., Arabic text). Yuseke, can you give me (off-line) strings you have this problem with? If input strings are indeed normalized and isNormalized returns false, that's a bug in ICU to fix. BTW, icu:: qualifier was missing for UnicodeString and I added it in r47998. Ah, the comment I wrote was not clear enough... I meant to say "Harfbuzz can handle unnormalized Arabic string." ICU is working well. Sorry for the confusion.
> BTW, icu:: qualifier was missing for UnicodeString and I added it in r47998.
Thanks!
|