[webkit-reviews] review requested: [Bug 20677] WebKit wraps between hyphen-minus and numeric characters : [Attachment 106301] new patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Sep 4 21:15:44 PDT 2011


Xianzhu Wang <wangxianzhu at chromium.org> has asked  for review:
Bug 20677: WebKit wraps between hyphen-minus and numeric characters
https://bugs.webkit.org/show_bug.cgi?id=20677

Attachment 106301: new patch
https://bugs.webkit.org/attachment.cgi?id=106301&action=review

------- Additional Comments from Xianzhu Wang <wangxianzhu at chromium.org>
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?

Yes. I forgot to update the copyright in
https://bugs.webkit.org/show_bug.cgi?id=37698. I ever used phnixwxz at gmail.com
when I had already been a Google employee.

>> Source/WebCore/rendering/break_lines.cpp:89
>> +	{ 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.

It's part of a line-breaking matrix. The above change disables line breaking
after '-' and before '.' and '0'..'9'. In fact, the above change doesn't affect
the result after '-' and before digits because this is hard-coded in
shouldBreakAfter().
Added some comment here and ChangeLog.

>> 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.

Yes. ICU can handle the case correctly for both ASCII and non-ASCII characters
but with lower performance.
This code will be executed if both nextCh and ch are ASCII characters, and is
covering the cases of this bug.
I think non-ASCII characters are out of the scope of this bug.

>> 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?

I did some test with ICU by reducing the context only from the lastChar. The
ICU line-break iterator works differently with the reduced contexts. Many ICU
line-breaking rules require even larger contexts.

However, for now WebKit won't pass contexts across element boundaries to
line-break iterators. This causes different line-breaking on in-line element
boundaries. For example, it may break after the '-' in '1111111-abcdefg', but
not in '1111111-<i>abcdefg</i>'. This patch won't change this behavior. Should
this be a bug? I searched and found a related bug:
https://bugs.webkit.org/show_bug.cgi?id=56269.

>>>>
LayoutTests/fast/text/line-breaks-after-hyphen-before-number-expected.txt:24
>>>> +1111111-2222222
>>> 
>>> This output is wrong, no?
>> 
>> The line breaks based on width don’t show up in the DumpRenderTree output,
even though the break does happen on screen. The test checks that the result is
correct by comparing the heights of the elements.
>> 
>> That’s consistent with the expected behavior. When you convert to text, line
breaks based on the width of the layout aren’t considered.
> 
> I think it's quite misleading to dump the text result like this then. If I
saw this result, I'd assume that the test is failing.  We should probably hide
these text when ran in DRT/TestRunner.

Done.

>> LayoutTests/fast/text/line-breaks-after-hyphen-before-number-expected.txt:30

>> +PASS
> 
> We should probably print PASS/FAIL for each test case so that we can easily
diagnose the results when one of these cases fail.

Done.


More information about the webkit-reviews mailing list