Bug 85332 – Add css3-images image-resolution (dppx only)

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

[David Barr]

The css3-images module is at candidate recommendation.
http://www.w3.org/TR/2012/CR-css3-images-20120417/#image-resolution

I propose to introduce image-resolution, initially behind a runtime feature flag.

Advertised on webkit-dev:
http://thread.gmane.org/gmane.os.opendarwin.webkit.devel/20505

As a first step, support just dppx values.

[David Barr]

Created attachment 139733 [details]
Patch

[Build Bot]

Comment on attachment 139733 [details]
Patch

Attachment 139733 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12591751

[David Barr]

Created attachment 139738 [details]
Patch

Added export definitions for Windows.

[WebKit Review Bot]

Comment on attachment 139738 [details]
Patch

Attachment 139738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12590760

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/css/getComputedStyle/computed-style.html
fast/css/getComputedStyle/computed-style-without-renderer.html

[WebKit Review Bot]

Created attachment 139746 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[David Barr]

Created attachment 139961 [details]
Patch

Rebased against https://bugs.webkit.org/show_bug.cgi?id=85457

[David Barr]

Created attachment 143472 [details]
Patch

Added ref-test with test-case generator.

[David Barr]

Created attachment 144439 [details]
Patch

Updated patch to use compile time flag rather than runtime flag.

[David Barr]

Created attachment 144457 [details]
Patch

Also hide DPPX parsing behind compile flag.

[WebKit Review Bot]

Attachment 144457 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/mac/test_expectations.txt:164:  fast/inline/continuation-outlines-with-layers.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:173:  tables/mozilla_expected_failures/collapsing_borders/bug41262-5.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/mac/test_expectations.txt:222:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:20:  inspector/styles/override-screen-size.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:62:  ietestcenter/css3/valuesandunits/units-000.htm is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:77:  fast/borders/border-antialiasing.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:81:  tables/mozilla/bugs/bug10296-1.html is also in a Skipped file.  [test/expectations] [5]
LayoutTests/platform/qt/test_expectations.txt:84:  editing/inserting/typing-space-to-trigger-smart-link.html is also in a Skipped file.  [test/expectations] [5]
WARNING: skia test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/skia/skia_test_expectations.txt' does not exist
WARNING: chromium test_expectations overrides file '/mnt/git/webkit-style-queue/Source/WebKit/chromium/webkit/tools/layout_tests/test_expectations.txt' does not exist
Total errors found: 8 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 144457 [details]
Patch

Attachment 144457 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12847615

[WebKit Review Bot]

Comment on attachment 144457 [details]
Patch

Attachment 144457 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12839787

New failing tests:
animations/duplicated-keyframes-name.html
css3/filters/custom/custom-filter-property-computed-style.html
css3/filters/custom/custom-filter-property-parsing.html
animations/animation-direction.html
animations/keyframe-timing-functions2.html
animations/fill-mode-removed.html
css3/filters/filter-property-parsing.html
animations/fill-mode-multiple-keyframes.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
animations/change-keyframes.html
css3/filters/filter-property-computed-style.html
animations/fill-mode.html
animations/fill-mode-reverse.html
animations/animation-direction-reverse-timing-functions.html
animations/change-keyframes-name.html
animations/fill-mode-iteration-count-non-integer.html
compositing/animation/animated-composited-inside-hidden.html
animations/keyframe-timing-functions.html
animations/animation-direction-reverse-non-hardware.html
animations/import.html
animations/fill-mode-missing-from-to-keyframes.html
animations/keyframes-comma-separated.html
animations/animation-direction-reverse-fill-mode.html
animations/animation-direction-reverse-hardware-opacity.html
animations/change-one-anim.html
animations/generic-from-to.html

[WebKit Review Bot]

Created attachment 144648 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[David Barr]

Created attachment 144732 [details]
Patch

Fixed a few issues with the application of the compile time flag.

[David Barr]

Comment on attachment 144732 [details]
Patch

Preliminary patches have landed, time to begin review.

[Mike Lawther]

Comment on attachment 144732 [details]
Patch

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

I'm not a reviewer, but here are some comments:

> Source/WebCore/css/CSSParser.cpp:1370
> +#endif

Please only #ifdef out the line that you've added. You'll need to shift the closing ; down a line, which is ugly, but this is only intended to temporary.

> Source/WebCore/css/CSSParser.cpp:2526
> +            while (isValid && value) {

Do you need to parse in a list of values? As per our offline conversation, for this patch, you only need to accept a single value, as you're only handling dppx at the moment.

> Source/WebCore/css/CSSParser.cpp:2527
> +                if (validUnit(value, FResolution | FNonNeg) && value->fValue > 0)

Do you need to also test fValue > 0 if FNonNeg is true in validUnit?

> Source/WebCore/css/CSSPropertyNames.in:107
> +#if defined(ENABLE_CSS_IMAGE_RESOLUTION) && ENABLE_CSS_IMAGE_RESOLUTION

This #if test is different to all the others? I don't understand why.

> Source/WebCore/rendering/RenderImage.cpp:142
> +        imageDimensionsChanged(true /* imageSizeChanged */);

I don't get what this does here?

> LayoutTests/fast/css/script-tests/image-resolution.js:40
> +  for (i=0; i<resolutions.length; i++) {

nit: spaces around operators

> LayoutTests/fast/css/script-tests/image-resolution.js:88
> +  for(i in tests) {

nit: space after for

> LayoutTests/fast/css/script-tests/image-resolution.js:96
> +    div.style['top'] = y;

is 'top' different here for a reason?

[David Barr]

Comment on attachment 144732 [details]
Patch

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

Thank you for the comments. I will address the nits without responding individually.

>> Source/WebCore/css/CSSParser.cpp:1370
>> +#endif
> 
> Please only #ifdef out the line that you've added. You'll need to shift the closing ; down a line, which is ugly, but this is only intended to temporary.

I tried this approach in an earlier patch and it broke the MSVC build.

>> Source/WebCore/css/CSSParser.cpp:2526
>> +            while (isValid && value) {
> 
> Do you need to parse in a list of values? As per our offline conversation, for this patch, you only need to accept a single value, as you're only handling dppx at the moment.

I think the surrounding block should be split into a function, parseImageResolution().
It should not accept multiple values of the same type.
Always creating a list makes parsing and applying the style rule a little less verbose when the remaining features are added.

>> Source/WebCore/css/CSSParser.cpp:2527
>> +                if (validUnit(value, FResolution | FNonNeg) && value->fValue > 0)
> 
> Do you need to also test fValue > 0 if FNonNeg is true in validUnit?

Zero is non-negative but invalid.

>> Source/WebCore/css/CSSPropertyNames.in:107
>> +#if defined(ENABLE_CSS_IMAGE_RESOLUTION) && ENABLE_CSS_IMAGE_RESOLUTION
> 
> This #if test is different to all the others? I don't understand why.

Following the existing style within .in files.

>> Source/WebCore/rendering/RenderImage.cpp:142
>> +        imageDimensionsChanged(true /* imageSizeChanged */);
> 
> I don't get what this does here?

RenderStyle::diff() interprets a change in image-resolution as StyleDifferenceLayout.
This ensures that the intrinsic size is recalculated on such a change.
The condition should be extended to require that image-resolution indeed changed.

>> LayoutTests/fast/css/script-tests/image-resolution.js:96
>> +    div.style['top'] = y;
> 
> is 'top' different here for a reason?

My editor seemed to think top was a reserved word, probably should have ignored it.

[David Barr]

Created attachment 144984 [details]
Patch

Addressed Mike's comments.

[David Barr]

Created attachment 144995 [details]
Patch

Updated ChangeLog.

[Ojan Vafai]

Comment on attachment 144995 [details]
Patch

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

Mostly just style nits.

> Source/WebCore/css/CSSParser.cpp:8324
> +        else if (length == 4 && isASCIIAlphaCaselessEqual(type[1], 'p')
> +                 && isASCIIAlphaCaselessEqual(type[2], 'p') && isASCIIAlphaCaselessEqual(type[3], 'x'))

I'm OK with committing this behind a command-line flag, but it sounds like there is contention on www-style about whether to use dppx or just x. Can you link to that thread here with a comment saying not to remove the define until that discussion is resolved?

> Source/WebCore/css/CSSParser.cpp:8325
> +                 m_token = DPPX;

Nit: indentation is off here.

> Source/WebCore/css/CSSPrimitiveValue.h:127
> +        // Used by image-resolution

I don't think this comment adds much value. It very well might be used for something else in the near future and it's highly unlikely that patch will think to update this comment.

> Source/WebCore/css/StyleBuilder.cpp:1785
> +        int len = valueList->length();

Is there a reason to pull this out into a local variable?

> Source/WebCore/rendering/RenderImage.cpp:200
> +#if ENABLE(CSS_IMAGE_RESOLUTION)
> +    bool intrinsicSizeChanged = updateIntrinsicSizeIfNeeded(m_imageResource->imageSize(style()->effectiveZoom() / style()->imageResolution()), imageSizeChanged);
> +#else

Presumably we need to do this for <input type=image> as well or does that use a RenderImage? If the former, that should be a followup patch, but a FIXME wouldn't hurt for this patch. If the latter, a simple testcase as part of this patch would be nice.

> LayoutTests/fast/css/image-resolution.html:1
> +<html>

Typically we prefer new tests to be in standards mode. So this and the reference should both have the html doctype "<!DOCTYPE html>".

> LayoutTests/fast/css/image-resolution.html:7
> +</head>
> +<body>

There's not wide agreement about this in the webkit community, so feel free to ignore this, but I prefer tests like this to leave out the html and head elements since they don't add any value. You need the body element because you references it in the JS code.

> LayoutTests/fast/css/image-resolution.html:8
> +<script src="script-tests/image-resolution.js"></script>

This doesn't really need to be a ref-test does it? It seems like the only thing you need to verify are that the image is the right dimensions.

Take a look at the css3/flexbox tests as a way to do this dumpAsText.

> LayoutTests/fast/css/script-tests/image-resolution.js:6
> +var dppxImplemented = true;
> +var dpiImplemented = false;
> +var dpcmImplemented = false;
> +var fromImageImplemented = false;
> +var fromImagePlumbingImplemented = false;
> +var snapImplemented = false;

This isn't great in a test. Typically, you would just have a different test for each of these cases that you would commit with (or before) the code that implement it. 

Also, what if one port decides to enable a different subset of these than another? Then one of them can't run this test.

It's better to have easily understandable tests than to avoid code duplication at all costs. As it is, I'm having trouble following exactly what this test is doing.

> LayoutTests/fast/css/script-tests/image-resolution.js:10
> +var imgUrl = '../images/resources/green.jpg';
> +var imgWidthPx = 16;
> +var imgHeightPx = 16;

These are only used in one place, may as well inline them.

> LayoutTests/fast/css/script-tests/image-resolution.js:11
> +var imgResolutionDppx = 72 / 96;

This is nice because it makes the code more readable, but you should define variables as close to where you use them as possible.

> LayoutTests/fast/css/script-tests/image-resolution.js:12
> +var epsilon = 0.001;

I don't understand the purpose of this. It probably deserves a comment explaining why.

> LayoutTests/fast/css/script-tests/image-resolution.js:18
> +  ['0dpi', '96dpi', '192dpi', '288dpi', '384dpi',
> +   '150dpi', '300dpi', '450dpi', '600dpi']);

Nit: webkit doesn't have a line-length limit. I think this would be more readable as a single line.

> LayoutTests/fast/css/script-tests/image-resolution.js:28
> +function permute3(rule) {
> +  var s = rule.split(' ');

Here and elsewhere. We don't have a formal rule on code style in tests, but we may as well be consistent with the larger code-style constraints of webkit so developers don't need to deal with different editor settings all the time. Specifically:
-4 space indent
-Opening bracket of a function should be on it's own line.

> LayoutTests/fast/css/script-tests/image-resolution.js:42
> +    for (fromImage = 0; fromImage < (snapImplemented ? 2 : 1); fromImage++) {

Nit: do you intend fromImage and snap to be global variables?

> LayoutTests/fast/css/script-tests/image-resolution.js:76
> +    dppx = imgResolutionDppx;

Why does this take imgResolutionDppx as an argument if we only ever use the constant value defined above? Could we just define the variable right above this line?

> LayoutTests/fast/css/script-tests/image-resolution.js:84
> +function test(tests) {

No need for this function IMO. Just generate the tests and dive into the for-loop in the global scope.

> LayoutTests/fast/css/script-tests/image-resolution.js:99
> +    div.appendChild(img);

I'd fine this test a lot more readable and concise if you wrote this using innerHTML/cssText.

var imgSize = Math.floor(imgWidthPx / dppx + epsilon) + 'px';
var position = '8px';
var div = document.createElement('div');
div.style.cssText = "position: absolute; top: ' + position + '; left: ' + ... + "; height: ' + imgSize;
div.innerHTML = '<img src="../images/resources/green.jpg" style="image-resolution: ' + tests[i] + '">';
document.body.appendChild(div);

[Tony Chang]

Comment on attachment 144995 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:1369
>      ASSERT((value->unit >= CSSPrimitiveValue::CSS_NUMBER && value->unit <= CSSPrimitiveValue::CSS_KHZ)
>             || (value->unit >= CSSPrimitiveValue::CSS_TURN && value->unit <= CSSPrimitiveValue::CSS_REMS)
>             || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMIN));

Nit: It would be nice if we just reordered the CSSPrimitiveValue::UnitTypes so CSS_TURN, CSS_REMS, CSS_VW to CSS_VMIN are next to CSS_KHZ.  Would be part of a cleanup patch.

> Source/WebCore/css/CSSParser.cpp:6831
> +    bool isValid = true;
> +    while (isValid && value) {

Nit: Can we get rid of the isValid variable and just return 0 when we see an invalid value?

> Source/WebCore/css/CSSParser.h:441
>          FFrequency = 0x0040,
>          FRelative  = 0x0100,

Nit: I wonder if we intentionally skipped 0x0080 here.  Bit shifting might be clearer.  Would be a good clean up patch for someone.

> Source/WebCore/css/CSSPrimitiveValue.cpp:66
>      case CSSPrimitiveValue:: CSS_DEG:
>      case CSSPrimitiveValue:: CSS_DIMENSION:
> +#if ENABLE(CSS_IMAGE_RESOLUTION)
> +    case CSSPrimitiveValue:: CSS_DPPX:
> +#endif
>      case CSSPrimitiveValue:: CSS_EMS:

Nit: Why are there spaces between :: and the unit name?  Would make a good cleanup patch.

> Source/WebCore/css/CSSPrimitiveValue.h:128
> +        CSS_DPPX = 115

It looks like this should be near CSS_KHZ.

> Source/WebCore/css/CSSPropertyNames.in:108
> +image-resolution

Should this be -webkit prefixed until the implementation is more complete?

> Source/WebCore/rendering/style/RenderStyle.h:1655
> +#if ENABLE(CSS_IMAGE_RESOLUTION)
> +    static float initialImageResolution() { return 1; }
> +#endif

Nit: I wouldn't bother putting this behind an ENABLE.  It'll get removed by the compiler if not used.

> LayoutTests/ChangeLog:12
> +        * fast/css/image-resolution-expected.html: Added.
> +        * fast/css/image-resolution.html: Added.

It might be useful to add a test that just does the css parsing.  For example, it would be nice to have test cases for things like invalid values for image-resolution or too many or not enough values.

> LayoutTests/platform/chromium/test_expectations.txt:157
> +// CSS image-resolution is not yet enabled.
> +BUGWK85262 SKIP : fast/css/image-resolution.html = PASS FAIL

I would put all image-resolution tests in a sub directory so you can skip all of them.  I suspect you'll have a few tests before you're ready to remove the flag.

[David Barr]

Comment on attachment 144995 [details]
Patch

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

Thanks for the comments.

>> Source/WebCore/css/CSSParser.cpp:1369
>>             || (value->unit >= CSSPrimitiveValue::CSS_VW && value->unit <= CSSPrimitiveValue::CSS_VMIN));
> 
> Nit: It would be nice if we just reordered the CSSPrimitiveValue::UnitTypes so CSS_TURN, CSS_REMS, CSS_VW to CSS_VMIN are next to CSS_KHZ.  Would be part of a cleanup patch.

I'll make a such cleanup easier by placing CSS_DPPX just after CSS_VMIN.

>> Source/WebCore/css/CSSParser.cpp:6831
>> +    while (isValid && value) {
> 
> Nit: Can we get rid of the isValid variable and just return 0 when we see an invalid value?

Yes.

>> Source/WebCore/css/CSSParser.cpp:8324
>> +                 && isASCIIAlphaCaselessEqual(type[2], 'p') && isASCIIAlphaCaselessEqual(type[3], 'x'))
> 
> I'm OK with committing this behind a command-line flag, but it sounds like there is contention on www-style about whether to use dppx or just x. Can you link to that thread here with a comment saying not to remove the define until that discussion is resolved?

I'll add a comment.

>> Source/WebCore/css/CSSParser.cpp:8325
>> +                 m_token = DPPX;
> 
> Nit: indentation is off here.

Oops.

>> Source/WebCore/css/CSSParser.h:441
>>          FRelative  = 0x0100,
> 
> Nit: I wonder if we intentionally skipped 0x0080 here.  Bit shifting might be clearer.  Would be a good clean up patch for someone.

Noted.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:66
>>      case CSSPrimitiveValue:: CSS_EMS:
> 
> Nit: Why are there spaces between :: and the unit name?  Would make a good cleanup patch.

Noted.

>> Source/WebCore/css/CSSPrimitiveValue.h:127
>> +        // Used by image-resolution
> 
> I don't think this comment adds much value. It very well might be used for something else in the near future and it's highly unlikely that patch will think to update this comment.

Comment to be removed.

>> Source/WebCore/css/CSSPrimitiveValue.h:128
>> +        CSS_DPPX = 115
> 
> It looks like this should be near CSS_KHZ.

Just after CSS_VMIN works well.

>> Source/WebCore/css/CSSPropertyNames.in:108
>> +image-resolution
> 
> Should this be -webkit prefixed until the implementation is more complete?

The flag should not be flipped until the implementation is complete.

>> Source/WebCore/css/StyleBuilder.cpp:1785
>> +        int len = valueList->length();
> 
> Is there a reason to pull this out into a local variable?

No.

>> Source/WebCore/rendering/RenderImage.cpp:200
>> +#else
> 
> Presumably we need to do this for <input type=image> as well or does that use a RenderImage? If the former, that should be a followup patch, but a FIXME wouldn't hurt for this patch. If the latter, a simple testcase as part of this patch would be nice.

The latter; <input type=image> becomes RenderImage.

>> Source/WebCore/rendering/style/RenderStyle.h:1655
>> +#endif
> 
> Nit: I wouldn't bother putting this behind an ENABLE.  It'll get removed by the compiler if not used.

Will clean up.

>> LayoutTests/ChangeLog:12
>> +        * fast/css/image-resolution.html: Added.
> 
> It might be useful to add a test that just does the css parsing.  For example, it would be nice to have test cases for things like invalid values for image-resolution or too many or not enough values.

I will simplify this test in the next patch. It already tests the invalid values of '' and '0dppx'.

>> LayoutTests/fast/css/image-resolution.html:1
>> +<html>
> 
> Typically we prefer new tests to be in standards mode. So this and the reference should both have the html doctype "<!DOCTYPE html>".

Ok.

>> LayoutTests/fast/css/image-resolution.html:7
>> +<body>
> 
> There's not wide agreement about this in the webkit community, so feel free to ignore this, but I prefer tests like this to leave out the html and head elements since they don't add any value. You need the body element because you references it in the JS code.

While I agree about the lack of value of these tags, they do make some reviewers feel warm and fuzzy so I'll keep them.

>> LayoutTests/fast/css/image-resolution.html:8
>> +<script src="script-tests/image-resolution.js"></script>
> 
> This doesn't really need to be a ref-test does it? It seems like the only thing you need to verify are that the image is the right dimensions.
> 
> Take a look at the css3/flexbox tests as a way to do this dumpAsText.

Being a ref-test helps keeping the test concise. The test and it's expected result differ only in a few CSS rules.

>> LayoutTests/fast/css/script-tests/image-resolution.js:6
>> +var snapImplemented = false;
> 
> This isn't great in a test. Typically, you would just have a different test for each of these cases that you would commit with (or before) the code that implement it. 
> 
> Also, what if one port decides to enable a different subset of these than another? Then one of them can't run this test.
> 
> It's better to have easily understandable tests than to avoid code duplication at all costs. As it is, I'm having trouble following exactly what this test is doing.

I'll remove all the placeholder code from the test to avoid confusion.

>> LayoutTests/fast/css/script-tests/image-resolution.js:10
>> +var imgHeightPx = 16;
> 
> These are only used in one place, may as well inline them.

I prefer to keep them together as they relate to the image resource used to build the test.

>> LayoutTests/fast/css/script-tests/image-resolution.js:11
>> +var imgResolutionDppx = 72 / 96;
> 
> This is nice because it makes the code more readable, but you should define variables as close to where you use them as possible.

Sure, within the constraint of grouping related data.

>> LayoutTests/fast/css/script-tests/image-resolution.js:12
>> +var epsilon = 0.001;
> 
> I don't understand the purpose of this. It probably deserves a comment explaining why.

I don't think it's necessary, will remove.

>> LayoutTests/fast/css/script-tests/image-resolution.js:18
>> +   '150dpi', '300dpi', '450dpi', '600dpi']);
> 
> Nit: webkit doesn't have a line-length limit. I think this would be more readable as a single line.

Will do.

>> LayoutTests/fast/css/script-tests/image-resolution.js:28
>> +  var s = rule.split(' ');
> 
> Here and elsewhere. We don't have a formal rule on code style in tests, but we may as well be consistent with the larger code-style constraints of webkit so developers don't need to deal with different editor settings all the time. Specifically:
> -4 space indent
> -Opening bracket of a function should be on it's own line.

Noted and will do.

>> LayoutTests/fast/css/script-tests/image-resolution.js:42
>> +    for (fromImage = 0; fromImage < (snapImplemented ? 2 : 1); fromImage++) {
> 
> Nit: do you intend fromImage and snap to be global variables?

I'll drop these from this patch.

>> LayoutTests/fast/css/script-tests/image-resolution.js:76
>> +    dppx = imgResolutionDppx;
> 
> Why does this take imgResolutionDppx as an argument if we only ever use the constant value defined above? Could we just define the variable right above this line?

As above, defined by the image resource. Passed as an argument to make it clear what the inputs are.

>> LayoutTests/fast/css/script-tests/image-resolution.js:84
>> +function test(tests) {
> 
> No need for this function IMO. Just generate the tests and dive into the for-loop in the global scope.

Will do.

>> LayoutTests/fast/css/script-tests/image-resolution.js:99
>> +    div.appendChild(img);
> 
> I'd fine this test a lot more readable and concise if you wrote this using innerHTML/cssText.
> 
> var imgSize = Math.floor(imgWidthPx / dppx + epsilon) + 'px';
> var position = '8px';
> var div = document.createElement('div');
> div.style.cssText = "position: absolute; top: ' + position + '; left: ' + ... + "; height: ' + imgSize;
> div.innerHTML = '<img src="../images/resources/green.jpg" style="image-resolution: ' + tests[i] + '">';
> document.body.appendChild(div);

I'll meet you halfway on this.

>> LayoutTests/platform/chromium/test_expectations.txt:157
>> +BUGWK85262 SKIP : fast/css/image-resolution.html = PASS FAIL
> 
> I would put all image-resolution tests in a sub directory so you can skip all of them.  I suspect you'll have a few tests before you're ready to remove the flag.

Will do.

[David Barr]

Created attachment 145930 [details]
Patch

[Tony Chang]

Comment on attachment 145930 [details]
Patch

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

r- because Ojan doesn't like the use of a ref-test.

On a side note, what are you plans for handling other images like images referenced by CSS (e.g., background-image)?

> LayoutTests/ChangeLog:12
> +        * fast/css/image-resolution/image-resolution-expected.html: Added.
> +        * fast/css/image-resolution/image-resolution.html: Added.

I'm not a huge fan of using a ref test when a dumpAsText test would suffice (it's slower to run and harder to make sense of the results).

> LayoutTests/fast/css/image-resolution/image-resolution.html:10
> +<body>
> +<script src="resources/image-resolution.js"></script>

Nit: Can you add a test description?  E.g., This test passes if no red is showing.

[David Barr]

Created attachment 146448 [details]
Patch

Refactor to js-test from ref-test.

[David Barr]

> r- because Ojan doesn't like the use of a ref-test.
> 
> On a side note, what are you plans for handling other images like images referenced by CSS (e.g., background-image)?

This patch does not add support for applying image-resolution to background-image.
When that is added, it will need a ref-test because AFAICT there's no way to extract the intrinsic size of a background image in javascript.

[Tony Chang]

Comment on attachment 146448 [details]
Patch

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

The code looks fine, just some questions/comments about the test.

> LayoutTests/fast/css/image-resolution/image-resolution-expected.txt:3
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".

Why don't I see TEST COMPLETE in the output?

> LayoutTests/fast/css/image-resolution/resources/image-resolution.js:1
> +if (window.layoutTestController) {

Is the plan is to use this js file in multiple tests?  If not, we should just inline it in the .html file.

> LayoutTests/fast/css/image-resolution/resources/image-resolution.js:2
> +    layoutTestController.dumpAsText();

Nit: This is called by js-test-pre.js.

> LayoutTests/fast/css/image-resolution/resources/image-resolution.js:26
> +        var cssPxPerIn = 96;
> +        if (unit === 'dpi')
> +            dppx = value / cssPxPerIn;
> +        var cmPerIn = 2.54;
> +        if (unit === 'dpcm')
> +            dppx = value / (cssPxPerIn / cmPerIn);

Nit: I would use 'else if' since these are multually exclusive.

> LayoutTests/fast/css/image-resolution/resources/image-resolution.js:59
> +    Array.prototype.forEach.call(resolutions, function(test) {

Nit: resolutions.forEach(function(test) {

[David Barr]

Created attachment 146667 [details]
Patch for landing

Addressed the comments of Tony's positive review. Prepared patch for landing.

[David Barr]

> The code looks fine, just some questions/comments about the test.
> 
> > LayoutTests/fast/css/image-resolution/image-resolution-expected.txt:3
> > +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> Why don't I see TEST COMPLETE in the output?

No idea how this fell out of the patch - fixed in the patch for landing.

> > LayoutTests/fast/css/image-resolution/resources/image-resolution.js:1
> > +if (window.layoutTestController) {
> 
> Is the plan is to use this js file in multiple tests?  If not, we should just inline it in the .html file.

Inline it for now. If additional tests have overlapping functions, extract at that time.

> > LayoutTests/fast/css/image-resolution/resources/image-resolution.js:2
> > +    layoutTestController.dumpAsText();
> 
> Nit: This is called by js-test-pre.js.

Removed.

> > LayoutTests/fast/css/image-resolution/resources/image-resolution.js:26
> > +        var cssPxPerIn = 96;
> > +        if (unit === 'dpi')
> > +            dppx = value / cssPxPerIn;
> > +        var cmPerIn = 2.54;
> > +        if (unit === 'dpcm')
> > +            dppx = value / (cssPxPerIn / cmPerIn);
> 
> Nit: I would use 'else if' since these are multually exclusive.

Moved constants before the if/else sequence and amended as suggested.

> > LayoutTests/fast/css/image-resolution/resources/image-resolution.js:59
> > +    Array.prototype.forEach.call(resolutions, function(test) {
> 
> Nit: resolutions.forEach(function(test) {

Amended as suggested.

[WebKit Review Bot]

Comment on attachment 146667 [details]
Patch for landing

Clearing flags on attachment: 146667

Committed r119984: <http://trac.webkit.org/changeset/119984>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.