Bug 48860 – U+FEFF is rendered as an empty box in a complex script page on Windows

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jungshik Shin]

Created attachment 72712 [details]
preliminary patch without layout test yet

This is a chromium bug (http://crbug.com/50337 ). I'm attaching a preliminary patch without a layout test (yet). 

Go to http://www.youtube.com/watch?v=Kjbmp2FAM1Q and comments have some empty boxes sprinkled.

[Jungshik Shin]

Created attachment 89799 [details]
updated patch (more generic)

This is more generic than the previous one. A layout test will be uploaded separately for now. After generating expected results, I upload everything together for review.

[Jungshik Shin]

Created attachment 89800 [details]
layout test (wip) 

I'm going to revise this test. This is a check-point upload.

[Jungshik Shin]

Created attachment 89945 [details]
patch for review 

This patch include a layout test. I also filed bug 58741 for Chromium Linux that fails the test (only in DumpRenerTree).

[Jungshik Shin]

BTW, I confirmed that both Safari Mac and Chrome on Mac with my patch (to treat U+FEFF as zero-width space glyph in simple/complex script paths) pass the test (complex script code path was not change for them and they work as expected). The latest nightly build of Webkit on Windows passes the complex script version and I'm sure it'll pass the simple script version with U+FEFF added if my patch is applied.

[WebKit Review Bot]

Attachment 89945 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/platform/graphics/chromium/UniscribeHelper.h:230:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:790:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:941:  More than one command on the same line in if  [whitespace/parens] [4]
Total errors found: 4 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Jungshik Shin]

Created attachment 90053 [details]
patch updated to fix style nits

[David Levin]

Comment on attachment 90053 [details]
patch updated to fix style nits

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

r- primarily due to the questions that I have which hopefully you'll help me understand.

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:790
> +                && !Font::treatAsZeroWidthSpaceInComplexScript(c))

fwiw, no line break is needed here (but if you wish to have one, go right ahead).

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:796
> +            if (Font::treatAsSpace(c)) {

Why not store the result of treatAsSpace (from above) and use the result here?

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:798
> +                // but space_width does. Here we find out how off we are from

space_width? (Perhaps you mean spaceWidthWithoutLetterSpacing).

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:800
> +                //  then just subtract that diff.

extra space after //

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:809
> +            // For characters to treat as zero-width space in complex

Consider
// For characters treated as zero-width spaces in complex

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:939
> +        // do not care if a character is zero-width invisible character.

Start with capital letter.

Also, it doesn't look like this comment adds any value as it just repeats what the code does. If you think a comment is valuable here, explain why the code is skipping zero-width spaces.

Perhaps like this
// Skip zero-width spaces because they are never considered to be missing in a font.

> Source/WebCore/platform/graphics/chromium/UniscribeHelper.cpp:945
> +            || (glyph == properties->wgInvalid && glyph != properties->wgBlank))

No line break needed (but if you wish that is fine).

> LayoutTests/fast/text/zero-width-characters-complex-script.html:18
> +    else if (abWidth > aWidth)

No need for else since the previous if has a return.

> LayoutTests/fast/text/zero-width-characters-complex-script.html:20
> +    else

Ditto.

> LayoutTests/fast/text/zero-width-characters-complex-script.html:29
> +    for (var i = 1; i < 32; ++i) // >

I don't understand what the comment ">" means.

> LayoutTests/fast/text/zero-width-characters-complex-script.html:30
> +      if (i != 9 && i != 10 && i != 13) // ;

Ditto for ";".

Also consider a 4 space indent (and the same for the next line).

Why are these values omitted (9, 10, 13) (since they are included in treatAsZeroWidthSpaceInComplexScript)?

> LayoutTests/fast/text/zero-width-characters-complex-script.html:38
> +    failedCount += testChar(0xFFFC);

It looks like this doesn't include everything in treatAsZeroWidthSpaceInComplexScript. Why not?

[David Levin]

Removing [chromium]. This bug is about move than chromium, since it changes treatAsZeroWidthSpace which is used by other ports by adding a check for zeroWidthNoBreakSpace = 0xFEFF; in there.

Adding Dan in case he has any concerns about that part of the change.

[Jungshik Shin]

Created attachment 90154 [details]
updated patch addressing Levin's comment

Thank you for the review. I addressed all the review comments. 

(9, 10, 13) are excluded because they're turned to an actual space before reaching the code in question per HTML(5). Well, you may ask why they're included in treatAsZeroWidthSpace(InComplexScript) in the first place. Perhaps, they shouldn't be included, but I haven't checked other uses of treatAsZeroWidthSpace and leaving it alone in this patch. 

As for not testing other characters in treatAsZeroWidthSpaceInComplexScript, it's an oversight because I made a new layout test by modifying zero-width-characters.html and didn't notice that the test does not test for U+007F - U+009F/U+00AD and U+202A - U+202E. 

I guess the latter block is not tested in zero-width-characters.html because they're BiDi controls and it can mess up the width measurement. I think I can rewrite zero-width-characters.html to test them as well. 

As for the former, I think it's just an oversight. 

I'll revise that test in a separate CL to address both issues.

[Jungshik Shin]

(In reply to comment #9)

> I'll revise that test in a separate CL to address both issues.

To clarify: 
zero-width-characters-complex-script.html does test two blocks mentioned. What I'm gonna revise in a separate patch is the existing test (zero-width-characters.html).

[Jungshik Shin]

I also attached a 'diagnosis-friendly' test to bug 58741 for Linux Chrome (it just fails one character, but other ports may fail more and want to know which ones fail).

[WebKit Review Bot]

http://trac.webkit.org/changeset/84264 might have broken Chromium Win Release

[WebKit Review Bot]

http://trac.webkit.org/changeset/84271 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
plugins/mouse-click-iframe-to-plugin.html

[Ryosuke Niwa]

The test added by this change is failing on Qt:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/31631

[Jungshik Shin]

bug 58741 was revised to track both Chrome Linux (DRT) and Qt Linux test failures.