Bug 79961 - Font fallback in cr-win is wrong for string contains zero-width-space
Summary: Font fallback in cr-win is wrong for string contains zero-width-space
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-29 17:20 PST by Xiaomei Ji
Modified: 2012-03-21 12:04 PDT (History)
4 users (show)

See Also:


Attachments
patch w/ layout test (116.00 KB, patch)
2012-03-01 11:54 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (5.94 KB, patch)
2012-03-06 17:32 PST, Xiaomei Ji
no flags Details | Formatted Diff | Diff
patch w/ layout test (7.82 KB, patch)
2012-03-08 12:42 PST, 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 2012-02-29 17:20:59 PST
related chromium bug: crbug.com/111002
Comment 1 Xiaomei Ji 2012-03-01 11:54:29 PST
Created attachment 129728 [details]
patch w/ layout test
Comment 2 Jungshik Shin 2012-03-06 13:21:58 PST
Comment on attachment 129728 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=129728&action=review

If possible, I'd rather add a layout test that does not rely on the actual font used. Revising zero-width-characters-complex-script.html would be better, wouldn't it?

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:889
> +            bool treatAsZeroWidthSpace = c == zeroWidthSpace || Font::treatAsZeroWidthSpaceInComplexScript(c);

Can you add ZWSP to the set of characters returned by treatAsZeroWidth....?
Comment 3 Xiaomei Ji 2012-03-06 17:32:22 PST
Created attachment 130486 [details]
patch w/ layout test

Thanks for the review!

(In reply to comment #2)
> (From update of attachment 129728 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129728&action=review
> 
> If possible, I'd rather add a layout test that does not rely on the actual font used. Revising zero-width-characters-complex-script.html would be better, wouldn't it?

Yes. Done.

> 
> > Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:889
> > +            bool treatAsZeroWidthSpace = c == zeroWidthSpace || Font::treatAsZeroWidthSpaceInComplexScript(c);
> 
> Can you add ZWSP to the set of characters returned by treatAsZeroWidth....?

Looks like it is save to add ZWSP as one of the characters to be treatedAsZeroWidthSpaceInComplexSccipt. Done.
Comment 4 Jungshik Shin 2012-03-07 16:00:24 PST
Comment on attachment 130486 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=130486&action=review

> LayoutTests/fast/text/zero-width-characters-complex-script.html:33
> +    return failedCount;

Perhaps, it's better to count Devanagari and Arabic failed counts separately and report them separately as well by changing the caller as well as the function signature (to accept 'a' and 'b' as parameters).
Comment 5 Xiaomei Ji 2012-03-08 12:42:00 PST
Created attachment 130881 [details]
patch w/ layout test

address comment by changing layout test file.
Comment 6 Jungshik Shin 2012-03-21 10:39:40 PDT
Comment on attachment 130881 [details]
patch w/ layout test

Looks good. Thank you
Comment 7 Xiaomei Ji 2012-03-21 10:40:30 PDT
tkent, abath,

could you please review the patch or suggest a reviewer?
Comment 8 Adam Barth 2012-03-21 10:53:45 PDT
This patch changes non-Chromium specific code and therefore shouldn't have the [Chromium] tag.
Comment 9 Adam Barth 2012-03-21 10:56:09 PDT
Comment on attachment 130881 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=130881&action=review

> Source/WebCore/platform/graphics/Font.h:232
> -    static bool treatAsZeroWidthSpaceInComplexScript(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter; }
> +    static bool treatAsZeroWidthSpaceInComplexScript(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || c == zeroWidthSpace || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter; }

Are there any other callers of this function?
Comment 10 Adam Barth 2012-03-21 11:02:11 PDT
> Are there any other callers of this function?

grep says:

/Users/abarth/svn/webkit/Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:
  889:             if (!treatAsSpace && !Font::treatAsZeroWidthSpaceInComplexScript(c))
 1039:         if (Font::treatAsZeroWidthSpaceInComplexScript(c))

/Users/abarth/svn/webkit/Source/WebCore/platform/graphics/harfbuzz/HarfBuzzShaperBase.cpp:
   66:         else if (Font::treatAsZeroWidthSpace(character))

/Users/abarth/svn/webkit/Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:
  522:             } else if (ch == zeroWidthSpace || (Font::treatAsZeroWidthSpace(ch) && !treatAsSpace)) {

/Users/abarth/svn/webkit/Source/WebCore/platform/graphics/win/UniscribeController.cpp:
  275:         bool treatAsZeroWidthSpace = ch == zeroWidthSpace || Font::treatAsZeroWidthSpace(ch);
  276:         if (treatAsSpace || treatAsZeroWidthSpace) {
Comment 11 Adam Barth 2012-03-21 11:04:07 PDT
Comment on attachment 130881 [details]
patch w/ layout test

I'm not a font expert, but it seems reasonable to treat zeroWidthSpace as a ZeroWidthSpaceInComplexScript.  :)
Comment 12 Xiaomei Ji 2012-03-21 11:19:43 PDT
Comment on attachment 130881 [details]
patch w/ layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=130881&action=review

>> Source/WebCore/platform/graphics/Font.h:232
>> +    static bool treatAsZeroWidthSpaceInComplexScript(UChar c) { return c < 0x20 || (c >= 0x7F && c < 0xA0) || c == softHyphen || c == zeroWidthSpace || (c >= 0x200e && c <= 0x200f) || (c >= 0x202a && c <= 0x202e) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter; }
> 
> Are there any other callers of this function?

treatedAsZeroWidthSpaceInComplexScript is only used in graphics/chromium/UniscribeHelper.cpp and Font::treatedAsZeroWidthSpace().

The bug is directly caused by that zeroWidthSpace (U+200B) itself is not handled the same way as those characters that could be treated as zero-width-space-in-complex-script in cr-win code. So, adding zeroWidthSpace here fixes the cr-win bug.

The above change also makes Font::treatAsZeroWidthSpace() returns true for  zeroWidthSpace (U+200B).
Font::treatAsZeroWidthSpace()  is used in Font()::normalizeSpaces(), normalizeSpacesAndMirrorChars() in HarfBuzzShaperBase.cpp,  in which, the character is normalized to zeroWidthSpace if it could be treatAsZeroWidthSapce(), so the above change will keep the functionality.

Font::treatAsZeroWidthSpace()  is also used in graphics/mac/ComplexTextController.cpp and graphics/win/UniscribeController.cpp, in which, zeroWidthSpace is handled separately but in the same way as other characters that can be treatedAsZeroWidthSpace(). so, above change (plus the updated code in this patch for those 2 files) will keep the functionality too.

There are no other usages.
Comment 13 Adam Barth 2012-03-21 11:24:43 PDT
Comment on attachment 130881 [details]
patch w/ layout test

Thanks.  That's consistent with what my grep turned up.
Comment 14 WebKit Review Bot 2012-03-21 12:04:47 PDT
Comment on attachment 130881 [details]
patch w/ layout test

Clearing flags on attachment: 130881

Committed r111589: <http://trac.webkit.org/changeset/111589>
Comment 15 WebKit Review Bot 2012-03-21 12:04:52 PDT
All reviewed patches have been landed.  Closing bug.