Bug 180818 - Line clamp implementation
Summary: Line clamp implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dave Hyatt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-12-14 10:24 PST by Dave Hyatt
Modified: 2018-01-24 14:53 PST (History)
3 users (show)

See Also:


Attachments
Patch (12.97 KB, patch)
2017-12-14 10:26 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (12.77 KB, patch)
2017-12-14 10:33 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (14.25 KB, patch)
2017-12-14 10:51 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (71.48 KB, patch)
2018-01-16 12:41 PST, Dave Hyatt
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.35 MB, application/zip)
2018-01-16 14:19 PST, EWS Watchlist
no flags Details
Patch (71.47 KB, patch)
2018-01-17 09:08 PST, Dave Hyatt
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.35 MB, application/zip)
2018-01-17 10:49 PST, EWS Watchlist
no flags Details
Patch (79.71 KB, patch)
2018-01-18 08:41 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (82.28 KB, patch)
2018-01-18 08:52 PST, Dave Hyatt
no flags Details | Formatted Diff | Diff
Patch (91.46 KB, patch)
2018-01-24 14:25 PST, Dave Hyatt
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dave Hyatt 2017-12-14 10:24:12 PST
Line clamp parsing
Comment 1 Dave Hyatt 2017-12-14 10:26:55 PST
Created attachment 329361 [details]
Patch
Comment 2 EWS Watchlist 2017-12-14 10:29:04 PST
Attachment 329361 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/LineClampValue.h:66:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Dave Hyatt 2017-12-14 10:33:22 PST
Created attachment 329362 [details]
Patch
Comment 4 Dave Hyatt 2017-12-14 10:51:51 PST
Created attachment 329364 [details]
Patch
Comment 5 Alex Christensen 2017-12-14 12:10:08 PST
Comment on attachment 329364 [details]
Patch

It looks like you forgot to add the actual test.  This patch only contains expectations.
Comment 6 Dave Hyatt 2018-01-16 12:41:22 PST
Created attachment 331414 [details]
Patch
Comment 7 EWS Watchlist 2018-01-16 12:43:41 PST
Attachment 331414 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLinesClampFlow.cpp:27:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:60:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:106:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:110:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:129:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:139:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
ERROR: Source/WebCore/rendering/RenderLinesClampSet.cpp:150:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 40 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 EWS Watchlist 2018-01-16 14:19:55 PST
Comment on attachment 331414 [details]
Patch

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

New failing tests:
fast/overflow/line-clamp.html
Comment 9 EWS Watchlist 2018-01-16 14:19:57 PST
Created attachment 331429 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 10 Dave Hyatt 2018-01-17 09:08:58 PST
Created attachment 331500 [details]
Patch
Comment 11 EWS Watchlist 2018-01-17 10:49:10 PST
Comment on attachment 331500 [details]
Patch

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

New failing tests:
fast/overflow/line-clamp.html
Comment 12 EWS Watchlist 2018-01-17 10:49:12 PST
Created attachment 331518 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 13 Dave Hyatt 2018-01-18 08:41:23 PST
Created attachment 331624 [details]
Patch
Comment 14 Dave Hyatt 2018-01-18 08:52:23 PST
Created attachment 331628 [details]
Patch
Comment 15 Dave Hyatt 2018-01-24 14:25:02 PST
Created attachment 332200 [details]
Patch
Comment 16 Dean Jackson 2018-01-24 14:36:53 PST
Comment on attachment 332200 [details]
Patch

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

How will Mail toggle the setting? Is it already exposed as a WebPreference?

> Source/WebCore/rendering/RenderBlockFlow.cpp:3238
> +    // FIXME: Orthogonal writing modes don't work here, but it's not even clear how they should be behave anyway.

Nit: should be behave

> Source/WebCore/rendering/RenderLinesClampFlow.h:39
> +    void layout() override;

Did you need to do this, since you just call RenderMultiColumnFlow::layout()?

> Source/WebCore/rendering/RenderLinesClampSet.cpp:188
> +

Nit: remove blank line
Comment 17 Dave Hyatt 2018-01-24 14:53:15 PST
Landed in r227577.