[webkit-reviews] review denied: [Bug 16131] ZWNJ - Display non-printing, invisible character : [Attachment 51638] Removed ZWJ test case for complex font path

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Apr 19 09:33:50 PDT 2010 

Joseph Pecoraro <joepeck at webkit.org> has denied David Yonge-Mallo
<davinci at chromium.org>'s request for review:
Bug 16131: ZWNJ - Display non-printing, invisible character
https://bugs.webkit.org/show_bug.cgi?id=16131

Attachment 51638: Removed ZWJ test case for complex font path
https://bugs.webkit.org/attachment.cgi?id=51638&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> +2010-03-25  David Yonge-Mallo  <davinci at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Added tests for the handling of ZWJ and ZWNJ characters in simple
font
> +	   path.  These characters have zero width but may change the widths of
text 
> +	   around them.  Furthermore, their glyphs should not be displayed in
> +	   simple static text.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=16131
> +
> +	   * fast/text/format-control.html: Added.
> +	   * fast/text/zero-width-characters.html: modified to test ZWSP, ZWJ,
and ZWNJ.
> +	   * platform/mac/fast/text/format-control-expected.checksum: Added.
> +	   * platform/mac/fast/text/format-control-expected.png: Added.
> +	   * platform/mac/fast/text/format-control-expected.txt: Added.

NIT: Since I'm going to recommend some minor changes, could you clean up the
ChangeLog a little. The bug title "ZWNJ - Display non-printing, invisible
character" is missing, and the bug link should go above the comments.
Otherwise, this is a great comment and a good ChangeLog!

The normal order (produced by prepare-ChangeLog) is:

  Reviewed By ...

  Bug Title
  https://bugs.webkit.org/show_bug.cgi?id=...

  Comments...

  * files and comments...


> +<html><head>
> +<meta charset="utf-8"> 
> +</head>
> +<body>
> +This tests the ZWJ and ZWNJ format control characters on basic Latin text.
> +
> +<p>fi fl ff ffi ffl</p> 
> +<p>f‌i f‌l f‌f f‌f‌i f‌f‌l</p> 
> +<p>f‍i f‍l f‍f f‍f‍i f‍f‍l</p> 
> +<textarea rows="5" cols="80">fi fl ff ffi ffl
> +f‌i f‌l f‌f f‌f‌i f‌f‌l
> +f‍i f‍l f‍f f‍f‍i f‍f‍l</textarea>
> +</body>
> +</html>

This test is currently passing on Mac platforms. It would be needlessly
difficult to make it fail by default on all possible platforms, but I think
making it fail by default on mac platforms as well would help it to be caught
on build bots. On Mac you can use a font like "Times New Roman". I would
recommend duplicating the <p> and <textarea> down below but forcing that font:

  <div style="font-family: Times New Roman">
  <p>fi fl ff ffi ffl</p> 
  <p>f‌i f‌l f‌f f‌f‌i f‌f‌l</p> 
  <p>f‍i f‍l f‍f f‍f‍i f‍f‍l</p> 
  <textarea rows="5" cols="80">fi fl ff ffi ffl
  f‌i f‌l f‌f f‌f‌i f‌f‌l
  f‍i f‍l f‍f f‍f‍i f‍f‍l</textarea>
  </div>

This would require updated test results.




> +2010-03-25  David Yonge-Mallo  <davinci at chromium.org>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Fix the (non)display of glyphs for ZWJ and ZWNJ in simple font code
path.
> +
> +	   https://bugs.webkit.org/show_bug.cgi?id=16131
> +
> +	   Tests: fast/text/format-control.html
> +		  fast/text/zero-width-characters.html
> +
> +	   * platform/graphics/Font.h:
> +	   (WebCore::Font::operator!=):
> +	   (WebCore::Font::treatAsZeroWidthSpace): treat ZWNJ and ZWJ as ZWSP.
> +	   * platform/graphics/GlyphPageTreeNode.cpp:
> +	   (WebCore::GlyphPageTreeNode::initializePage): added ZWNJ and ZWJ.
> +	   * platform/text/CharacterNames.h: added ZWNJ and ZWJ.
> +

NIT: The same ChangeLog comments that I had above.


The patch itself looks good to me! Just update the test so a regression could
surely be caught!

More information about the webkit-reviews mailing list