[webkit-reviews] review granted: [Bug 49786] Implement (non-EventListener) marquee IDL attributes from HTML5. : [Attachment 74359] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andy Estes <aestes at apple.com>'s
request for review:
Bug 49786: Implement (non-EventListener) marquee IDL attributes from HTML5.
https://bugs.webkit.org/show_bug.cgi?id=49786

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=74359&action=review

This seems OK, but could easily be event better.

> WebCore/html/HTMLMarqueeElement.cpp:135
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +	   if (Attribute* scrollAmount =
namedNodeMap->getAttributeItem(scrollamountAttr))
> +	       return scrollAmount->value().string().toUInt();
> +    }

This should be written like this:

    const AtomicString& scrollamount = fastGetAttribute(scrollAmountAttr);
    if (!scrollAmount.isNull())
	return scrollAmount.string().toUInt();

Also, I don’t see the test cases about edge cases of parsing that would verify
whether toUInt is appropriate, such as garbage after a numeric string or the
empty string, a string with a non-digit in it a string with a negative number
in it a string with a number but some leading whitespace, a string with the
largest integer in it, or a string with the largest integer plus one. Depending
on how those edge cases should work, this code might or might not be correct.

> WebCore/html/HTMLMarqueeElement.cpp:149
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +	   if (Attribute* scrollDelay =
namedNodeMap->getAttributeItem(scrolldelayAttr))
> +	       return scrollDelay->value().string().toUInt();
> +    }

Same thing here. It’s not right to use attributes(). You should use
fastGetAttribute instead.

I’d like to see more test cases about parsing edge cases as I mentioned above.

> WebCore/html/HTMLMarqueeElement.cpp:166
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +	   if (Attribute* loop = namedNodeMap->getAttributeItem(loopAttr)) {
> +	       int loopVal = loop->value().string().toInt();
> +	       return (loopVal > 0 ? loopVal : -1);
> +	   }
> +    }
> +    return -1;

This can be written like this:

    int loopValue = fastGetAttribute(loopAttr).string().toInt();
    return loopValue > 0 ? loopValue : -1;

I’d like to see more test cases about parsing edge cases as I mentioned above.

> WebCore/html/HTMLMarqueeElement.cpp:172
> +    if (loop > 0 || loop == -1)
> +	   setIntegralAttribute(loopAttr, loop);

The loop == -1 case is not covered by the test case.

> WebCore/html/HTMLMarqueeElement.idl:27
> +	   attribute [Reflect=bgcolor] DOMString bgColor;

No need for "=bgcolor" here. [Reflect] handles the case difference
automatically. You can see examples of this in other files such as
HTMLBodyElement.idl.

> WebCore/html/HTMLMarqueeElement.idl:34
> +	   attribute [Reflect=truespeed] boolean trueSpeed;

No need for "=truespeed" here. [Reflect] handles the case difference
automatically.

> LayoutTests/fast/html/script-tests/marquee-element.js:1
> +description('Various tests for the marquee element.');

Does this test pass on IE?