Bug 100120 – -webkit-image-set should support resolution units other than 'x'

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rick Byers]

-webkit-image-set takes a scale factor with the unit 'x' which is equivalent to the standard 'dppx' unit.  This parsing of 'x' is hard-coded into CSSParser::parseImageSet.  Currently the draft spec (http://dev.w3.org/csswg/css4-images/#image-set-notation) says that this can be any valid resolution.  We should support parsing any resolution here, and perhaps just make 'x' a synonym for 'dppx' units.

This is probably pretty low priority in my opinion.

[Noam Rosenthal]

Created attachment 388531 [details]
Patch

[Noam Rosenthal]

Regarding computed style - the decision to use dppx instead of x as a computed value is a matter of choosing between backwards compatibility and spec correctness. I chose to go with correctness here because computed values for image-set are probably not a big thing backwards-compatibility-wise. But it's a possible alternative to keep "x" values for backwards compatibility. Thoughts about that welcome.

[Darin Adler]

Comment on attachment 388531 [details]
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.

[Noam Rosenthal]

Created attachment 388828 [details]
Patch

[Noam Rosenthal]

Created attachment 388829 [details]
Patch

[Noam Rosenthal]

Created attachment 388834 [details]
Patch

[EWS]

Comment on attachment 388834 [details]
Patch

Rejecting attachment 388834 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Darin Adler]

Comment on attachment 388834 [details]
Patch

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

> LayoutTests/fast/css/image-set-parsing-expected.txt:59
> -PASS subRule.cssText is '3'
> +FAIL subRule.cssText should be 3. Was 3dppx.

Let's not introduce this failure.

> LayoutTests/fast/css/image-set-parsing.html:102
> +                ["c", "3", "b", "2dppx", "a", "1dppx"]);

This should be "3dppx" not "3".

> Source/WebCore/css/CSSImageSetValue.cpp:61
> +        m_imagesInSet.append({imageValue, scaleFactor});

WebKit coding style puts spaces inside the braces. (Not sure why, but I’ve tried to stay consistent.)

[Noam Rosenthal]

Created attachment 388972 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 388972 [details]
Patch for landing

Clearing flags on attachment: 388972

Committed r255228: <https://trac.webkit.org/changeset/255228>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58947714>

[Sam Sneddon [:gsnedders]]

*** Bug 206142 has been marked as a duplicate of this bug. ***