[webkit-reviews] review granted: [Bug 100120] -webkit-image-set should support resolution units other than 'x' : [Attachment 388531] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Noam Rosenthal <noam at webkit.org>'s
request for review:
Bug 100120: -webkit-image-set should support resolution units other than 'x'
https://bugs.webkit.org/show_bug.cgi?id=100120

Attachment 388531: Patch

https://bugs.webkit.org/attachment.cgi?id=388531&action=review




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 388531
  --> https://bugs.webkit.org/attachment.cgi?id=388531
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=388531&action=review

> Source/WebCore/css/CSSImageSetValue.cpp:53
> +    for (size_t i = 0; i < length - 1; i += 2) {

This does the wrong thing for length of 0. I suggest i + 1 < length instead,
which handles the case correctly.

> Source/WebCore/css/CSSImageSetValue.cpp:133
> +    for (size_t i = 0; i < length - 1; i += 2) {

This does the wrong thing for length of 0. I suggest i + 1 < length instead,
which handles the case correctly.

> Source/WebCore/css/CSSImageSetValue.cpp:139
> +	   result.append(item(i + 1)->cssText());

Write this as a single line for better efficiency:

    result.append(item(i)->cssText(), ' ', item(i + 1)->cssText());

> Source/WebCore/css/parser/CSSPropertyParserHelpers.h:63
> +enum class AllowXResolutionUnit {
> +    Allow,
> +    Forbid
> +};

My style opinion: This enum and the one above it should just be defined on a
single line.