Bug 124666 – Simple line layout should support floats

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

it should

[Sam Weinig]

(In reply to comment #0)
> it should

Should it?

[Antti Koivisto]

Created attachment 217459 [details]
patch

[Antti Koivisto]

> Should it?

Totally.

[Build Bot]

Comment on attachment 217459 [details]
patch

Attachment 217459 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/30088021

New failing tests:
fast/block/margin-collapse/empty-clear-blocks.html

[Build Bot]

Created attachment 217467 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Dave Hyatt]

Comment on attachment 217459 [details]
patch

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

r=me. I'm a bit concerned about all the physical naming though. This layout should obviously work with vertical text too, so the naming needs to be a bit less writing-mode-biased.

> Source/WebCore/rendering/SimpleLineLayoutResolver.h:111
> +    const LayoutUnit m_topBorderAndPadding;

Rename this to m_beforeBorderAndPadding.

> Source/WebCore/rendering/SimpleLineLayoutResolver.h:179
>      float baselineY = resolver.m_lineHeight * m_iterator.lineIndex() + resolver.m_baseline;

Would be nice if this "baselineY" didn't contain "Y" since it's horizontal for vertical text... Maybe just baselinePosition or baselineOffset?

> Source/WebCore/rendering/SimpleLineLayoutResolver.h:256
> +    , m_topBorderAndPadding(flow.borderTop() + flow.paddingTop())

Shouldn't this be 'before" rather than "top"?

[Antti Koivisto]

https://trac.webkit.org/r159579 (with more direction independent naming)

[Dean Jackson]

This broke a test, but seemed like it was ok to me.

I would appreciate either of you checking my fix:
https://trac.webkit.org/r159589

[Antti Koivisto]

The bad result was from http://trac.webkit.org/changeset/159575 which landed around the same time.