Bug 75998 - line height test for CSS3 calc
Summary: line height test for CSS3 calc
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike Lawther
URL:
Keywords:
Depends on:
Blocks: 16662
  Show dependency treegraph
 
Reported: 2012-01-10 15:26 PST by Mike Lawther
Modified: 2012-01-11 15:27 PST (History)
2 users (show)

See Also:


Attachments
Patch (3.25 KB, patch)
2012-01-10 15:29 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (3.25 KB, patch)
2012-01-10 15:33 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch (3.36 KB, patch)
2012-01-10 21:56 PST, Mike Lawther
no flags Details | Formatted Diff | Diff
Patch for landing (3.25 KB, patch)
2012-01-11 15:06 PST, Mike Lawther
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2012-01-10 15:26:53 PST
line height test for CSS3 calc
Comment 1 Mike Lawther 2012-01-10 15:29:15 PST
Created attachment 121919 [details]
Patch
Comment 2 Mike Lawther 2012-01-10 15:33:39 PST
Created attachment 121920 [details]
Patch
Comment 3 Daniel Bates 2012-01-10 17:12:20 PST
Comment on attachment 121920 [details]
Patch

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

For completeness, you may want to consider looking into using LayoutTests/fast/js/resources/js-test-{pre, post}.js, which provide convenience functions for writing PASS/FAIL tests.

> LayoutTests/css3/calc/line-height-expected.txt:3
> +The line height of these lines should be identical
> +The line height of these lines should be identical
> +The line height of these lines should be identical

I suggest that we omit this text when the test is run using DRT so as to make the expected results straight forward to interpret for PASS/FAIL. An example of a test case that omits such support text when run using DRT is <http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/button-height.html>. Notice, when button-height.html is run in DRT (i.e. window.layoutTestController exists) it removes the element with id "test-container", which contains the test text/support structures.

> LayoutTests/css3/calc/line-height-expected.txt:5
> +FAIL
> +Computed line heights do not match: 32px, normal, normal

I suggest concatenating these two lines such that line 4 prefixes line 5 since line 5 describes the failure alluded to by line 4.

> LayoutTests/css3/calc/line-height.html:27
> +        if (heights.length != 0)

Nit: I would write this as:

if (heights.length)
    ...
Comment 4 Mike Lawther 2012-01-10 21:56:01 PST
Created attachment 121974 [details]
Patch
Comment 5 Mike Lawther 2012-01-10 21:58:20 PST
Hi Daniel - thanks for the review. I've rewritten the test to use js-test-{pre, post}.
Comment 6 Daniel Bates 2012-01-10 22:51:35 PST
Comment on attachment 121974 [details]
Patch

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

Thanks Mike for the updated patch! This looks good. I have some minor nits.

> LayoutTests/css3/calc/line-height.html:4
> +    span { font-size: 16px;}

Nit: The spacing in this line is inconsistent with the spacing used in the lines below. In particular, the curly braces on this line aren't aligned with the curly braces of subsequent lines. I prefer a single space between the identifier and the '{'. Regardless, I suggest that we choose a notation and stick with it so as to be consistent.

> LayoutTests/css3/calc/line-height.html:18
> +    if (window.layoutTestController)
> +        layoutTestController.dumpAsText();

Nit: This is unnecessary by line 2 of LayoutTests/fast/js/resources/js-test-pre.js, <http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre.js?rev=102918#L2>.
Comment 7 Mike Lawther 2012-01-11 15:06:10 PST
Created attachment 122102 [details]
Patch for landing
Comment 8 Mike Lawther 2012-01-11 15:09:15 PST
Comment on attachment 121974 [details]
Patch

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

>> LayoutTests/css3/calc/line-height.html:4
>> +    span { font-size: 16px;}
> 
> Nit: The spacing in this line is inconsistent with the spacing used in the lines below. In particular, the curly braces on this line aren't aligned with the curly braces of subsequent lines. I prefer a single space between the identifier and the '{'. Regardless, I suggest that we choose a notation and stick with it so as to be consistent.

Done.

>> LayoutTests/css3/calc/line-height.html:18
>> +        layoutTestController.dumpAsText();
> 
> Nit: This is unnecessary by line 2 of LayoutTests/fast/js/resources/js-test-pre.js, <http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre.js?rev=102918#L2>.

Done.
Comment 9 WebKit Review Bot 2012-01-11 15:27:44 PST
Comment on attachment 122102 [details]
Patch for landing

Clearing flags on attachment: 122102

Committed r104752: <http://trac.webkit.org/changeset/104752>
Comment 10 WebKit Review Bot 2012-01-11 15:27:48 PST
All reviewed patches have been landed.  Closing bug.