[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