Bug 100120 - -webkit-image-set should support resolution units other than 'x'
Summary: -webkit-image-set should support resolution units other than 'x'
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Noam Rosenthal
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-10-23 07:06 PDT by Rick Byers
Modified: 2020-01-27 23:55 PST (History)
12 users (show)

See Also:


Attachments
Patch (34.02 KB, patch)
2020-01-23 03:10 PST, Noam Rosenthal
darin: review+
Details | Formatted Diff | Diff
Patch (34.13 KB, patch)
2020-01-26 22:54 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (34.13 KB, patch)
2020-01-26 22:57 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (34.13 KB, patch)
2020-01-26 23:51 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch for landing (35.45 KB, patch)
2020-01-27 23:12 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rick Byers 2012-10-23 07:06:19 PDT
-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.
Comment 1 Noam Rosenthal 2020-01-23 03:10:12 PST
Created attachment 388531 [details]
Patch
Comment 2 Noam Rosenthal 2020-01-23 03:24:17 PST
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.
Comment 3 Darin Adler 2020-01-26 21:49:26 PST
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.
Comment 4 Noam Rosenthal 2020-01-26 22:54:39 PST
Created attachment 388828 [details]
Patch
Comment 5 Noam Rosenthal 2020-01-26 22:57:25 PST
Created attachment 388829 [details]
Patch
Comment 6 Noam Rosenthal 2020-01-26 23:51:55 PST
Created attachment 388834 [details]
Patch
Comment 7 EWS 2020-01-27 14:55:46 PST
Comment on attachment 388834 [details]
Patch

Rejecting attachment 388834 [details] from commit-queue.

noam@webkit.org 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.
Comment 8 Darin Adler 2020-01-27 16:54:36 PST
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.)
Comment 9 Noam Rosenthal 2020-01-27 23:12:01 PST
Created attachment 388972 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-27 23:54:53 PST
Comment on attachment 388972 [details]
Patch for landing

Clearing flags on attachment: 388972

Committed r255228: <https://trac.webkit.org/changeset/255228>
Comment 11 WebKit Commit Bot 2020-01-27 23:54:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2020-01-27 23:55:27 PST
<rdar://problem/58947714>