Bug 28742 – [Chromium] Combining Diacritical Marks (U+0300..) are not handled correctly

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Yusuke Sato]

Created attachment 38615 [details]
screenshot

Text sequences that contain combining diacritical marks (e.g., "è") are not rendered correctly on Chromium Linux, while pre-combined forms (e.g., "&#00e8;") are OK. For example, the position of the accent grave of "è" is too high (see the screen shot). We can't enable the LayoutTests/fast/text/stroking*.html tests on Linux unless this issue is fixed.

Chromium bug: http://code.google.com/p/chromium/issues/detail?id=20130

[Dimitri Glazkov (Google)]

Adding agl to the bug, since he added complex text support to Linux.

[Yusuke Sato]

Created attachment 38619 [details]
proposed patch v1

[Yusuke Sato]

Created attachment 38620 [details]
screenshot after applying the v1 patch

[Adam Langley]

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.

[Yusuke Sato]

Oops, that's simply a typo. I'll attach v2 patch shortly. Thanks!

[Yusuke Sato]

Created attachment 38623 [details]
patch v2

[Adam Langley]

Comment on attachment 38623 [details]
patch v2

(I am not a WebKit reviewer): LGTM

[Eric Seidel (no email)]

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;

[Yusuke Sato]

Created attachment 38713 [details]
patch v3

[Yusuke Sato]

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.

[Eric Seidel (no email)]

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             }

[Eric Seidel (no email)]

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.

[Yusuke Sato]

> 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).

[Eric Seidel (no email)]

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+

[Eric Seidel (no email)]

Comment on attachment 38713 [details]
patch v3

Clearing flags on attachment: 38713

Committed r47922: <http://trac.webkit.org/changeset/47922>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[Jungshik Shin]

+        // 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.

[Yusuke Sato]

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!