[Webkit-unassigned] [Bug 20677] WebKit wraps between hyphen-minus and numeric characters

[bugzilla-daemon at webkit.org]

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





--- Comment #39 from Darin Adler <darin at apple.com>  2011-09-02 10:41:32 PST ---
(From update of attachment 79411)
View in context: https://bugs.webkit.org/attachment.cgi?id=79411&action=review

> Source/WebCore/rendering/break_lines.cpp:3
> + * Copyright (C) 2010, 2011 Google Inc. All rights reserved.

I’m a little confused about the 2010 here. Is there substantial code here that Google published in 2010?

> Source/WebCore/rendering/break_lines.cpp:89
> -    { B(1, 1, 1, 1, 1, 1, 1, 1), B(1, 1, 1, 1, 1, 1, 1, 1), F, B(1, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1) }, // -
> +    { B(1, 1, 1, 1, 1, 1, 1, 1), B(1, 1, 1, 1, 1, 0, 1, 0), 0, B(0, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1), F, F, F, B(1, 1, 1, 1, 1, 1, 1, 1) }, // -

It’s hard for me to review the change in this line because the change log does not state what these 4 changes are or what they are supposed to accomplish.

> Source/WebCore/rendering/break_lines.cpp:135
> +            return isASCIIAlphanumeric(lastCh);

This doesn’t seem right. Why does this handle ASCII alphabetical characters differently from non-ASCII alphabetical characters? Does this code only run if all characters are ASCII? Maybe the ICU code already does this right for non-ASCII, but I can’t tell that from the patch.

I’d like to see test cases covering the non-ASCII characters to show we have the same kinds of behavior in those cases.

> Source/WebCore/rendering/break_lines.cpp:173
> +    UChar lastLastCh = pos > 1 ? str[pos - 2] : 0;

This now requires more context to correctly break lines. Do the callers always provide enough context in cases where text within the line is split across multiple elements?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.