Bug 41305 - [Chromium] Characters with bidi-mirror property are not correctly mirrored in Linux
Summary: [Chromium] Characters with bidi-mirror property are not correctly mirrored in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Xiaomei Ji
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-28 14:35 PDT by Xiaomei Ji
Modified: 2010-07-09 11:03 PDT (History)
9 users (show)

See Also:


Attachments
test case (557 bytes, text/html)
2010-06-28 14:35 PDT, Xiaomei Ji
no flags Details
patch w/ layout test (36.96 KB, patch)
2010-06-28 14:49 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (37.13 KB, patch)
2010-06-28 18:02 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (37.28 KB, patch)
2010-07-02 16:43 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (37.18 KB, patch)
2010-07-07 16:46 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (37.24 KB, patch)
2010-07-07 17:18 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (66.86 KB, patch)
2010-07-08 16:43 PDT, Xiaomei Ji
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xiaomei Ji 2010-06-28 14:35:37 PDT
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
Comment 1 Xiaomei Ji 2010-06-28 14:49:41 PDT
Created attachment 59945 [details]
patch w/ layout test
Comment 2 Evan Martin 2010-06-28 15:49:03 PDT
+<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.
Comment 3 Xiaomei Ji 2010-06-28 18:02:36 PDT
Created attachment 59966 [details]
patch w/ layout test

updated per Evan's suggestion.
Comment 4 Evan Martin 2010-06-29 10:59:19 PDT
Not a review, but looks good to me.
We'll need to grab new baselines from the bots when this lands.
Comment 5 Eric Seidel (no email) 2010-07-02 14:29:44 PDT
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.
Comment 6 David Levin 2010-07-02 14:35:44 PDT
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.
Comment 7 Eric Seidel (no email) 2010-07-02 14:42:20 PDT
The executable bit is a symptom of working on windows.  Not a big deal.  We go through and clean them up every so often.
Comment 8 Xiaomei Ji 2010-07-02 16:43:25 PDT
Created attachment 60419 [details]
patch w/ layout test

updated per Eric and David's suggestion.
Comment 9 David Levin 2010-07-07 13:55:17 PDT
Comment on attachment 60419 [details]
patch w/ layout test

Thanks.
Comment 10 Xiaomei Ji 2010-07-07 16:46:06 PDT
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.
Comment 11 David Levin 2010-07-07 16:50:32 PDT
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.
Comment 12 David Levin 2010-07-07 16:56:59 PDT
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.
Comment 13 David Levin 2010-07-07 17:04:27 PDT
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.
Comment 14 Xiaomei Ji 2010-07-07 17:18:51 PDT
Created attachment 60810 [details]
patch w/ layout test

updated per David's feedback and uploaded again to commit queue.
Comment 15 Xiaomei Ji 2010-07-07 17:19:16 PDT
Comment on attachment 60804 [details]
patch w/ layout test

clear the r+ flag.
Comment 16 WebKit Commit Bot 2010-07-08 04:48:32 PDT
Comment on attachment 60810 [details]
patch w/ layout test

Clearing flags on attachment: 60810

Committed r62778: <http://trac.webkit.org/changeset/62778>
Comment 17 WebKit Commit Bot 2010-07-08 04:48:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2010-07-08 05:20:50 PDT
http://trac.webkit.org/changeset/62778 might have broken SnowLeopard Intel Release (Tests)
Comment 19 Xiaomei Ji 2010-07-08 16:42:18 PDT
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.
Comment 20 Xiaomei Ji 2010-07-08 16:43:38 PDT
Created attachment 60983 [details]
patch w/ layout test
Comment 21 David Levin 2010-07-08 17:13:12 PDT
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.
Comment 22 Xiaomei Ji 2010-07-09 10:29:39 PDT
Committed r62965: <http://trac.webkit.org/changeset/62965>
Comment 23 Xiaomei Ji 2010-07-09 10:31:43 PDT
Comment on attachment 60983 [details]
patch w/ layout test

clear the flags after the patch is committed.
Comment 24 WebKit Review Bot 2010-07-09 11:03:14 PDT
http://trac.webkit.org/changeset/62965 might have broken SnowLeopard Intel Release (Tests)