[webkit-reviews] review denied: [Bug 48860] [Chromium] U+FEFF is rendered as an empty box in a complex script page on Windows : [Attachment 90053] patch updated to fix style nits

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Jungshik Shin
<jshin at chromium.org>'s request for review:
Bug 48860: [Chromium] U+FEFF is rendered as an empty box in a complex script
page on Windows
https://bugs.webkit.org/show_bug.cgi?id=48860

Attachment 90053: patch updated to fix style nits
https://bugs.webkit.org/attachment.cgi?id=90053&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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?