[webkit-reviews] review denied: [Bug 47971] css combinator "+" in combination with NAV tag is buggy : [Attachment 96584] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 21 10:32:30 PDT 2011


Ojan Vafai <ojan at chromium.org> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 47971: css combinator "+" in combination with NAV tag is buggy
https://bugs.webkit.org/show_bug.cgi?id=47971

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=96584&action=review

The code change looks good to me. Please address Mike's and my comments on the
test then post a new patch for review.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:1
> +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Strict//EN">

These days we usually use the HTML5 doctype in tests.
<!DOCTYPE HTML>

> LayoutTests/fast/css/div_plus_nav_bug47971.html:4
> +<html>
> +<head>
> +<title>CSS Test: #div+nav</title>

No need to include these elements. The more sparse the test, the easier it is
to see what it's testing.

>> LayoutTests/fast/css/div_plus_nav_bug47971.html:17
>> +	<div id="one">DIV1</div>
> 
> Nit: "one" and "two" ids, and DIV1 and DIV2 text is not needed. It distracts
a bit when reading the test and looking at the results.

If you're doing this just so that the div will be visible, you could give the
div a width/height with CSS.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:44
> +function TestCase(element, resultElement)

This isn't really a class, but more of a method. Other tests will typically
call this "runTest".

>> LayoutTests/fast/css/div_plus_nav_bug47971.html:52
>> +	if (ArrayContains(greenValues, val, false))
> 
> Can you lowercase val on line 51, and say here "if (greenValues.indexOf(val)
!= -1)" ?

More importantly, why do you need to compare to a list of values? Won't the
results of getComputedStyle be the same string for all of these cases?


More information about the webkit-reviews mailing list