Font fallback in cr-win is wrong for string contains zero-width-space

• Component: Platform
• Status: RESOLVED FIXED

[Xiaomei Ji]

related chromium bug: crbug.com/111002

[Xiaomei Ji]

Created attachment 129728 [details]
patch w/ layout test

[Jungshik Shin]

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

[Xiaomei Ji]

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.

[Jungshik Shin]

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

[Xiaomei Ji]

Created attachment 130881 [details]
patch w/ layout test

address comment by changing layout test file.

[Jungshik Shin]

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

Looks good. Thank you

[Xiaomei Ji]

tkent, abath,

could you please review the patch or suggest a reviewer?

[Adam Barth]

This patch changes non-Chromium specific code and therefore shouldn't have the [Chromium] tag.

[Adam Barth]

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?

[Adam Barth]

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

[Adam Barth]

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

[Xiaomei Ji]

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.

[Adam Barth]

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

Thanks.  That's consistent with what my grep turned up.

[WebKit Review Bot]

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

Clearing flags on attachment: 130881

Committed r111589: <http://trac.webkit.org/changeset/111589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.