Bug 41305 – [Chromium] Characters with bidi-mirror property are not correctly mirrored in Linux

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Xiaomei Ji]

Created attachment 59942 [details]
test case

In Linux, within RTL block, characters with unicode-bidi-mirror property are not mirrored.

Open the test case, the parenthesis in the first line shows as "(...)..." visually from left-to-right.
The parenthesis in the next 2 lines show as ")...(...", which are wrong. They should be "(...)..." as well.

In chromium bug:
http://crbug.com/18026

[Xiaomei Ji]

Created attachment 59945 [details]
patch w/ layout test

[Evan Martin]

+<div>&#x05c6(&#x05c6)</div>  <!-- Hebrew letter treated as complext script -->

Typo: "complex"
(also on next line)


I wonder if it'd be better to test m_item.item.bidiLevel outside of the loop and if it's not set just to a straight copy.  It's probably a premature optimization though.

[Xiaomei Ji]

Created attachment 59966 [details]
patch w/ layout test

updated per Evan's suggestion.

[Evan Martin]

Not a review, but looks good to me.
We'll need to grab new baselines from the bots when this lands.

[Eric Seidel (no email)]

Comment on attachment 59966 [details]
patch w/ layout test

Why wouldn't this be a separate function? 

m_item.string = mirrorCharacters(m_run);

SEems that would be cleaner.

[David Levin]

In addition to Eric's feedback, you have executable set on LayoutTests/fast/text/international/bidi-mirror-he-ar.html

Please consider addressing these two things.

[Eric Seidel (no email)]

The executable bit is a symptom of working on windows.  Not a big deal.  We go through and clean them up every so often.

[Xiaomei Ji]

Created attachment 60419 [details]
patch w/ layout test

updated per Eric and David's suggestion.

[David Levin]

Comment on attachment 60419 [details]
patch w/ layout test

Thanks.

[Xiaomei Ji]

Created attachment 60804 [details]
patch w/ layout test

Changed the prototype of mirrorCharacters() from
UChar* mirrorCharacters() to
void mirrorCharacters(UChar* dst, const UChar* src, int length) const

Thanks for review again.

[David Levin]

Comment on attachment 60419 [details]
patch w/ layout test

Cleared r+, cq+ so that this obsolete patch doesn't get picked up by any tools.

[David Levin]

Comment on attachment 60804 [details]
patch w/ layout test


> Index: WebCore/platform/graphics/chromium/FontLinux.cpp
> +        m_item.stringLength = length;
> +
> +        if (!m_item.item.bidiLevel)
> +            m_item.string = m_run.characters();
> +        else {
> +            // Assume mirrored character is in the same BMP as the original one.

Please expand "BMP" to "basic multilingual plane" (as you told me that it stood for).

> +            UChar* string = new UChar[length];
> +            mirrorCharacters(string, m_run.characters(), length);
> +            m_item.string = string;
> +        }
> +
>          reset();
>      }
>  
> @@ -192,6 +203,8 @@ public:
>          fastFree(m_item.font);
>          deleteGlyphArrays();
>          delete[] m_item.log_clusters;
> +        if (m_item.item.bidiLevel)
> +            delete[] m_item.string;
>      }
>  
>      void reset()
> @@ -455,6 +468,22 @@ private:
>          m_offsetX += m_pixelWidth;
>      }
>  
> +    void mirrorCharacters(UChar* dst, const UChar* src, int length) const
> +    {
> +        int position = 0;
> +        bool error;
> +        // Iterate characters in src and mirror character if needed.
> +        while (position < length) {
> +          UChar32 character;

Need a 4 space indent.

[David Levin]

Comment on attachment 60804 [details]
patch w/ layout test

One more thing.

> Index: WebCore/platform/graphics/chromium/FontLinux.cpp
> +    void mirrorCharacters(UChar* dst, const UChar* src, int length) const

Please use source and destination as WebKit style is to avoid abbreviations.

[Xiaomei Ji]

Created attachment 60810 [details]
patch w/ layout test

updated per David's feedback and uploaded again to commit queue.

[Xiaomei Ji]

Comment on attachment 60804 [details]
patch w/ layout test

clear the r+ flag.

[WebKit Commit Bot]

Comment on attachment 60810 [details]
patch w/ layout test

Clearing flags on attachment: 60810

Committed r62778: <http://trac.webkit.org/changeset/62778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62778 might have broken SnowLeopard Intel Release (Tests)

[Xiaomei Ji]

reopened since it was reverted in r62798 (please refer https://bugs.webkit.org/show_bug.cgi?id=41866 for detail).

The previous patch caused assertion failure due to uninitialized variable "error" in the following code:
+        bool error;
+        while (position < length) {
             .............
+            U16_APPEND(destination, position, length, character, error);
+            ASSERT(!error);
             .....
+        }

"error" should be initialized as "false" since U16_APPEND() wont reset the value at entry.

Will upload new patch with the initialization as the only code difference, plus some test results update getting from buildbot and trybot.

[Xiaomei Ji]

Created attachment 60983 [details]
patch w/ layout test

[David Levin]

Comment on attachment 60983 [details]
patch w/ layout test

I'll leave it to you to decide to set cq+ or not.

Given that you're a committer and the previous version of this patch had problems. I personally would want to land it myself to be sure that I'm around and check that it doesn't break any bots.

[Xiaomei Ji]

Committed r62965: <http://trac.webkit.org/changeset/62965>

[Xiaomei Ji]

Comment on attachment 60983 [details]
patch w/ layout test

clear the flags after the patch is committed.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62965 might have broken SnowLeopard Intel Release (Tests)