[webkit-reviews] review denied: [Bug 37674] Implement sizes attribute for link tag from HTML5 : [Attachment 101696] Patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Rachel Blum
<groby at chromium.org>'s request for review:
Bug 37674: Implement sizes attribute for link tag from HTML5
https://bugs.webkit.org/show_bug.cgi?id=37674

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=101696&action=review


the bots are all confused, probably need to re-run the patch. Great progress!

> LayoutTests/fast/dom/icon-size-property.html:42
> +

Let's also test setting the sizes attribute from script.

> Source/WebCore/html/DOMSettableTokenList.cpp:103
> +    return DOMTokenList::validateToken(token, ec);

Could we just override validateToken and not do the fooInternal dance?

> Source/WebCore/html/SizesList.cpp:56
> +		   localEc = SYNTAX_ERR;

Are we really supposed to throw SYNTAX_ERR in these cases?

> Source/WebCore/html/SizesList.cpp:85
> +    ExceptionCode ec;
> +    DOMSettableTokenList::setValue("");
> +    for (unsigned i = 0; i < tokens.size(); ++i)
> +	   add(tokens[i], ec);

This seems inefficient. It's almost like SpaceSplitString wants to have a
validating function parameter. But I guess it's ok as is.

> Source/WebCore/html/SizesList.h:37
> +class SizesList : public DOMSettableTokenList {

Should we be more specific in naming, like IconSizeList?