[webkit-reviews] review denied: [Bug 56017] REGRESSION:Soft hyphen is not always rendered : [Attachment 85185] Updated patch to fix tabs/spaces style issue (whoops!)

Wed Mar 9 10:34:25 PST 2011

mitz at webkit.org has denied David Sosby <dsosby at rim.com>'s request for review:
Bug 56017: REGRESSION:Soft hyphen is not always rendered
https://bugs.webkit.org/show_bug.cgi?id=56017

Attachment 85185: Updated patch to fix tabs/spaces style issue (whoops!)
https://bugs.webkit.org/attachment.cgi?id=85185&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=85185&action=review

Code change is good, but I have comments about the change log and the test.

> Source/WebCore/ChangeLog:5
> +	   Resolve issue with soft hyphen not rendering

It’s customary to include the bug title in the change log.

> Source/WebCore/ChangeLog:11
> +	   (WebCore::RenderBlock::findNextLineBreak):

Please describe the change to this function.

> LayoutTests/ChangeLog:8
> +	   * fast/text/soft-hyphen-4.html: Added.

Where are the expected results?

> LayoutTests/fast/text/soft-hyphen-4.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Why the antiquated DOCTYPE?

> LayoutTests/fast/text/soft-hyphen-4.html:12
> +	       font-family: sans-serif;

Please consider rewriting the test to use the Ahem fonts so that the results
are consistent across platforms (and can be landed alongside the test, rather
than in platform-specific locations.