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

See Also:

Attachments
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
View All Add an attachment (proposed patch, testcase, etc.)

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]
patch
Comment 3 Antti Koivisto 2013-11-20 11:14:38 PST 
> Should it?

Totally.
Comment 4 Build Bot 2013-11-20 12:43:16 PST 
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
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]
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"?
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:
https://trac.webkit.org/r159589
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.