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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jan 10 22:51:34 PST 2012


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>.


More information about the webkit-reviews mailing list