[webkit-reviews] review denied: [Bug 21076] incorrect tabindex parsing : [Attachment 67523] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 21076: incorrect tabindex parsing
https://bugs.webkit.org/show_bug.cgi?id=21076

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67523&action=prettypatch

> WebCore/html/parser/HTMLParserIdioms.cpp:106
> +    } sign = Positive;
Why not use an int and set it to 1 or -1?

> WebCore/html/parser/HTMLParserIdioms.cpp:139
> +    // Step 6
> +    if (*position == '-') {
>  
> +	   // Substep 1
> +	   sign = Negative;
> +
> +	   // Substep 2
> +	   ++position;
> +
> +	   // Substep 3
> +	   if (position == end)
> +	       return false;
> +    } else if (*position == '+') {
> +	   // Substep 1
> +	   ++position;
> +
> +	   // Substep 2
> +	   if (position == end)
> +	       return false;
> +    }
> +    ASSERT(position < end);
This is all super-redundant.  1.  you can just ignore +.  2.  You could share
the position advancement and check code.

I think it's good that we're following the spec closely, but I don't think we
need to write it out in such verbosity.  Just document the individual steps
with comments.

> WebCore/html/parser/HTMLParserIdioms.cpp:152
> +    int number = WTF::charactersToIntStrict(digits.data(), digits.size());
Slightly sad this doesn't just take a vector or a WTF::String, but I guess
taking pointer/len pair makes the implementation simpler?

> WebCore/html/parser/HTMLParserIdioms.cpp:162
> +    switch (sign) {
> +    case Positive:
> +	   value = number;
> +	   break;
> +    case Negative:
> +	   value = -number;
> +	   break;
> +    }
Then this becomes number *= sign;  Actually just becomes int number =
WTF:::parseFunction() * sign. :)

Bleh. I much prefer results to include their pass fail information.  I agree
with your theory that it makes them easier to change.  But I dislike your
argument that since at least one result in teh repository requires you to
compare against another port's expected, we should make them all require you to
compare against expected.  But it's not a deal breaker.

r- for the unnecessary code duplication above.