[webkit-reviews] review denied: [Bug 85332] Add css3-images image-resolution (dppx only) : [Attachment 144995] Patch

Mon Jun 4 13:53:48 PDT 2012

Ojan Vafai <ojan at chromium.org> has denied David Barr <davidbarr at chromium.org>'s
request for review:
Bug 85332: Add css3-images image-resolution (dppx only)
https://bugs.webkit.org/show_bug.cgi?id=85332

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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);