[webkit-reviews] review granted: [Bug 75998] line height test for CSS3 calc : [Attachment 121974] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 75998: line height test for CSS3 calc
https://bugs.webkit.org/show_bug.cgi?id=75998

Attachment 121974: Patch
https://bugs.webkit.org/attachment.cgi?id=121974&action=review

------- Additional Comments from Daniel Bates <dbates at webkit.org>
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>.