Bug 169516 – Simple line layout: Paginated content is not painted properly when font overflows line height.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

font size -> big number.
line height -> small number 
-> lines overflow each other.

[alan]

Created attachment 304183 [details]
Patch

[Build Bot]

Comment on attachment 304183 [details]
Patch

Attachment 304183 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3296283

New failing tests:
fast/multicol/simple-line-layout-line-index-after-strut.html

[Build Bot]

Created attachment 304185 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[alan]

Created attachment 304197 [details]
Patch

[Antti Koivisto]

Comment on attachment 304197 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=304197&action=review

> Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:145
> +    for (int lineIndex = strut.lineBreak; lineIndex < (int)m_layout.lineCount(); ++lineIndex) {

Why int? Can strut.lineBreak and so lineIndex be negative? If it can should that case be handled separately as the rest of the function doesn't seem safe with negative values.

static_cast<int> would be more WebKit style than c-style cast (if we really need it).

> Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:147
> +        if (strutIndex < struts.size() && struts.at(strutIndex).lineBreak == (unsigned)lineIndex)

Same here.

> Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:152
> +            lastIndexCandidate = lineIndex;

If lineIndex was negative wouldn't coercing it to unsigned cause problems?

[alan]

(In reply to comment #5)
> Comment on attachment 304197 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=304197&action=review
> 
> > Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:145
> > +    for (int lineIndex = strut.lineBreak; lineIndex < (int)m_layout.lineCount(); ++lineIndex) {
> 
> Why int? Can strut.lineBreak and so lineIndex be negative? If it can should
> that case be handled separately as the rest of the function doesn't seem
> safe with negative values.
> 
> static_cast<int> would be more WebKit style than c-style cast (if we really
> need it).
> 
> > Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:147
> > +        if (strutIndex < struts.size() && struts.at(strutIndex).lineBreak == (unsigned)lineIndex)
> 
> Same here.
> 
> > Source/WebCore/rendering/SimpleLineLayoutResolver.cpp:152
> > +            lastIndexCandidate = lineIndex;
> 
> If lineIndex was negative wouldn't coercing it to unsigned cause problems?
Yea, you are right. I am not sure why I have it like that. Must be some leftover.

[alan]

Created attachment 304215 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304215 [details]
Patch

Clearing flags on attachment: 304215

Committed r213779: <http://trac.webkit.org/changeset/213779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.