Summary: | [Chromium] Characters with bidi-mirror property are not correctly mirrored in Linux | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xiaomei Ji <xji> | ||||||||||||||||
Component: | DOM | Assignee: | Xiaomei Ji <xji> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, agl, commit-queue, eric, evan, jshin, levin, mitz, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Other | ||||||||||||||||||
OS: | Linux | ||||||||||||||||||
Attachments: |
|
Description
Xiaomei Ji
2010-06-28 14:35:37 PDT
Created attachment 59945 [details]
patch w/ layout test
+<div>׆(׆)</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. Created attachment 59966 [details]
patch w/ layout test
updated per Evan's suggestion.
Not a review, but looks good to me. We'll need to grab new baselines from the bots when this lands. 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.
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. The executable bit is a symptom of working on windows. Not a big deal. We go through and clean them up every so often. Created attachment 60419 [details]
patch w/ layout test
updated per Eric and David's suggestion.
Comment on attachment 60419 [details]
patch w/ layout test
Thanks.
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 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 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 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. Created attachment 60810 [details]
patch w/ layout test
updated per David's feedback and uploaded again to commit queue.
Comment on attachment 60804 [details]
patch w/ layout test
clear the r+ flag.
Comment on attachment 60810 [details] patch w/ layout test Clearing flags on attachment: 60810 Committed r62778: <http://trac.webkit.org/changeset/62778> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/62778 might have broken SnowLeopard Intel Release (Tests) 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. Created attachment 60983 [details]
patch w/ layout test
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.
Committed r62965: <http://trac.webkit.org/changeset/62965> Comment on attachment 60983 [details]
patch w/ layout test
clear the flags after the patch is committed.
http://trac.webkit.org/changeset/62965 might have broken SnowLeopard Intel Release (Tests) |