Bug 124666 - Simple line layout should support floats
Summary: Simple line layout should support floats
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2013-11-20 09:05 PST by Antti Koivisto
Modified: 2013-11-20 17:16 PST (History)
8 users (show)

See Also:

patch (20.72 KB, patch)
2013-11-20 11:14 PST, Antti Koivisto
hyatt: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (727.59 KB, application/zip)
2013-11-20 12:43 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2013-11-20 09:05:42 PST
it should
Comment 1 Sam Weinig 2013-11-20 10:51:22 PST
(In reply to comment #0)
> it should

Should it?
Comment 2 Antti Koivisto 2013-11-20 11:14:10 PST
Created attachment 217459 [details]
Comment 3 Antti Koivisto 2013-11-20 11:14:38 PST
> Should it?

Comment 4 Build Bot 2013-11-20 12:43:16 PST
Comment on attachment 217459 [details]

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

New failing tests:
Comment 5 Build Bot 2013-11-20 12:43:17 PST
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
Comment 6 Dave Hyatt 2013-11-20 12:43:21 PST
Comment on attachment 217459 [details]

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"?
Comment 7 Antti Koivisto 2013-11-20 13:22:50 PST
https://trac.webkit.org/r159579 (with more direction independent naming)
Comment 8 Dean Jackson 2013-11-20 15:53:30 PST
This broke a test, but seemed like it was ok to me.

I would appreciate either of you checking my fix:
Comment 9 Antti Koivisto 2013-11-20 17:16:26 PST
The bad result was from http://trac.webkit.org/changeset/159575 which landed around the same time.