[Webkit-unassigned] [Bug 47971] css combinator "+" in combination with NAV tag is buggy

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=47971





--- Comment #12 from Mike Lawther <mikelawther at chromium.org>  2011-06-20 23:54:03 PST ---
(From update of attachment 96584)
View in context: https://bugs.webkit.org/attachment.cgi?id=96584&action=review

Hi Alex - thanks for the patch. I'm not a reviewer, but I've done a shadow review of the test case (mostly just nits). The actual fix in the flex looks OK to me.

> LayoutTests/fast/css/div_plus_nav_bug47971.html:5
> +<style type="text/css">

nit: no need for the type, it's assumed

> LayoutTests/fast/css/div_plus_nav_bug47971.html:6
> +#one + nav { color: green; }

Can you also put in a test for a case not involving the ID selector? eg "label+nav".

> LayoutTests/fast/css/div_plus_nav_bug47971.html:7
> + #two+nav { color: green; }

nit: extra space on the beginning of this line

> LayoutTests/fast/css/div_plus_nav_bug47971.html:9
> +<script>

nit: put this down in the <script> section below - no need for a separate one

> LayoutTests/fast/css/div_plus_nav_bug47971.html:16
> +<div>

Do you need this outer div?

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

> LayoutTests/fast/css/div_plus_nav_bug47971.html:21
> +<div>

ditto: is this div needed?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:26
> +<script type="text/javascript">

nit: no need for the type, it's assumed

> LayoutTests/fast/css/div_plus_nav_bug47971.html:27
> +function ArrayContains(array, value, ci)

You may not need this function - see below

> LayoutTests/fast/css/div_plus_nav_bug47971.html:29
> +  ci = ci == true ? true : false;

Should this line be here?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:33
> +    {

No braces for single lines.

> 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)" ?

> LayoutTests/fast/css/div_plus_nav_bug47971.html:57
> +  }catch(e){}

I don't think you want to swallow exceptions in test code like this.

> Source/WebCore/ChangeLog:10
> +        like "#div+nav" as: "#div" "+n" "av".

Is the id selector necessary? The bug only mentions "div+nav", but the id selector doesn't seem to be involved.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.