Add image-rendering: pixelated.

Add image-rendering: pixelated.

This adds a new value "pixelated" to the "image-rendering"
property. This value sets the interpolation used when
scaling images to InterpolationNone.

Intent to implement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/Q8N6FoeoPXI

BUG=260739, 106662
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176839

[jackhou, 2014-06-13 03:12:58]

Hey Mike,

I'm not really sure what boxes a blink CL needs to tick before it's ready for
review, so sending this to you for some feedback.

[pdr., 2014-06-13 03:21:14]

Nice patch!

Simple nits below:

Could you add a little more to your change description such as a link to your
intent to implement?

https://codereview.chromium.org/336693005/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/336693005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:8158: // Otherwise same as below.
Nit: No need for this comment.

https://codereview.chromium.org/336693005/diff/1/Source/core/rendering/ImageQ...
File Source/core/rendering/ImageQualityController.cpp (right):

https://codereview.chromium.org/336693005/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:65: if
(object->style()->imageRendering() == ImageRenderingPixelated) {
Nit: no need for the curly braces.

[jackhou, 2014-06-13 03:33:19]

Wow that was a quick response, thanks.

https://codereview.chromium.org/336693005/diff/1/Source/core/css/parser/CSSPr...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/336693005/diff/1/Source/core/css/parser/CSSPr...
Source/core/css/parser/CSSPropertyParser.cpp:8158: // Otherwise same as below.
On 2014/06/13 03:21:14, pdr wrote:
> Nit: No need for this comment.

Done.

https://codereview.chromium.org/336693005/diff/1/Source/core/rendering/ImageQ...
File Source/core/rendering/ImageQualityController.cpp (right):

https://codereview.chromium.org/336693005/diff/1/Source/core/rendering/ImageQ...
Source/core/rendering/ImageQualityController.cpp:65: if
(object->style()->imageRendering() == ImageRenderingPixelated) {
On 2014/06/13 03:21:14, pdr wrote:
> Nit: no need for the curly braces.

Done.

[pdr., 2014-06-13 03:59:23]

Sorry, I missed your test files before. Another round of nits below.

With these fixed, this patch looks good to me. Lets wait for an LG from Mike, as
he may have some additional comments.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
File LayoutTests/css3/images/pixelated-canvas.html (right):

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:5: position: fixed;
There are a lot of unnecessary values here. I think you just want:
width: 100px;
height: 100px;
image-rendering: pixelated;

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:23: if
(window.layoutTestController)
I haven't seen this since pre-fork Webkit :) Can you switch this to testRunner
instead of layoutTestController?

I'm not sure why you want dumpAsText though. Don't you just want the image
result?

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:26: var canvas;
Nit: indentation.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:56:
canvas.putImageData(imageHandle, 0, 0);
nit: indentation.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
File LayoutTests/css3/images/pixelated-image.html (right):

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:19: <!-- The resulting image should
be 100x100, consisting of 4 50x50 blocks of solid olor, with no blurring of
edges -->
Nit: solid color

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:20: <image style=""
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAYAAABytg0kAAAAFElEQVQIHWP4z8DwHwyBNJDN8B8AQNEG+t5Ik2kAAAAASUVORK5CYII="></img>
Nit: image -> img, no </img> tag needed, and remove the empty style.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:24:
layoutTestController.dumpAsText(true);
Ditto about layoutTestController and dumpAsText.

[jackhou, 2014-06-13 04:27:32]

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
File LayoutTests/css3/images/pixelated-canvas.html (right):

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:5: position: fixed;
On 2014/06/13 03:59:23, pdr wrote:
> There are a lot of unnecessary values here. I think you just want:
> width: 100px;
> height: 100px;
> image-rendering: pixelated;

Done.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:23: if
(window.layoutTestController)
On 2014/06/13 03:59:23, pdr wrote:
> I haven't seen this since pre-fork Webkit :) Can you switch this to testRunner
> instead of layoutTestController?
> 
> I'm not sure why you want dumpAsText though. Don't you just want the image
> result?

Yeah, I patched these from a 2 year old CL without really understanding them. I
think the dumpAsText can be removed, the test works without them.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:26: var canvas;
On 2014/06/13 03:59:23, pdr wrote:
> Nit: indentation.

Done.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-canvas.html:56:
canvas.putImageData(imageHandle, 0, 0);
On 2014/06/13 03:59:23, pdr wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
File LayoutTests/css3/images/pixelated-image.html (right):

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:19: <!-- The resulting image should
be 100x100, consisting of 4 50x50 blocks of solid olor, with no blurring of
edges -->
On 2014/06/13 03:59:23, pdr wrote:
> Nit: solid color

Done.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:20: <image style=""
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAYAAABytg0kAAAAFElEQVQIHWP4z8DwHwyBNJDN8B8AQNEG+t5Ik2kAAAAASUVORK5CYII="></img>
On 2014/06/13 03:59:23, pdr wrote:
> Nit: image -> img, no </img> tag needed, and remove the empty style.

Done.

https://codereview.chromium.org/336693005/diff/10001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-image.html:24:
layoutTestController.dumpAsText(true);
On 2014/06/13 03:59:23, pdr wrote:
> Ditto about layoutTestController and dumpAsText.

Done.

[Erik Dahlström (inactive), 2014-06-13 08:13:26]

Could you please add a test like LayoutTests/css3/images/pixelated-image.html
that uses svg:image:

<svg><image image-rendering="pixelated" xlink:href="..."/></svg>

[jackhou, 2014-06-13 08:44:16]

On 2014/06/13 08:13:26, Erik Dahlström wrote:
> Could you please add a test like LayoutTests/css3/images/pixelated-image.html
> that uses svg:image:
> 
> <svg><image image-rendering="pixelated" xlink:href="..."/></svg>

Done.

[Erik Dahlström (inactive), 2014-06-13 10:29:16]

https://codereview.chromium.org/336693005/diff/70001/LayoutTests/css3/images/...
File LayoutTests/css3/images/pixelated-svg-image.html (right):

https://codereview.chromium.org/336693005/diff/70001/LayoutTests/css3/images/...
LayoutTests/css3/images/pixelated-svg-image.html:3: svg {
I really meant:

image {  image-rendering: pixelated; }.

It might be interesting to have a separate test for pixelate on the svg element
itself too, especially if it had some other graphical elements inside too (like
<path> or such), just to show that even though you specify 'pixelated' it
shouldn't mean that the svg shouldn't render the shapes nicely.

[Mike Lawther (Google), 2014-06-14 02:27:50]

On 2014/06/13 03:59:23, pdr wrote:
> Sorry, I missed your test files before. Another round of nits below.
> 
> With these fixed, this patch looks good to me. Lets wait for an LG from Mike,
as
> he may have some additional comments.

Thanks for the reviews Philip! I'm on planes all day today, so I'll take a
proper look when I get back to the office.

[Mike Lawther (Google), 2014-06-20 04:26:50]

lgtm with nits.

Sorry for the delay. Yay for this patch being sooooo much simpler now that we
don't have to support multiple graphics backends!

This patch implements the correct behaviour for scaling up, but not yet for
scaling down. Can you please file a bug about this and add FIXMEs into the
relevant locations?

We should also seriously consider deprecating and removing
webkit-optimise-contrast. It only ever worked on Core Graphics backends, and is
no longer in any spec. Or at the very least, simply making it a synonym for
pixelated.

https://codereview.chromium.org/336693005/diff/90001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/336693005/diff/90001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:405: case
CSSPropertyImageRendering: // auto | optimizeContrast
Can you please update this comment for consistency?

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderHTMLCanvas.cpp (right):

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderHTMLCanvas.cpp:73: interpolationQuality =
InterpolationLow;
These should really be the same, ie InterpolationNone. From memory, it was only
set to InterpolationLow because that was the value that CoreGraphics used to
mean 'nearest neighbour'

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderHTMLCanvas.cpp:76: interpolationQuality =
InterpolationNone;
Strictly according to spec, we only use nearest neighbour if we are scaling up.
If we are scaling down, then we should be using the default scaling (quoth the
spec: "When scaling the image up, the "nearest neighbor" or similar algorithm
must be used, so that the image appears to be simply composed of very large
pixels. When scaling down, this is the same as auto.").

I'm fine with filing a bug about this with a FIXME.

[jackhou, 2014-06-20 07:14:44]

> This patch implements the correct behaviour for scaling up, but not yet for
> scaling down. Can you please file a bug about this and add FIXMEs into the
> relevant locations?

Done. http://crbug.com/386944

> We should also seriously consider deprecating and removing
> webkit-optimise-contrast. It only ever worked on Core Graphics backends, and
is
> no longer in any spec. Or at the very least, simply making it a synonym for
> pixelated.

Sounds good, I'll put together another CL for this.

https://codereview.chromium.org/336693005/diff/90001/Source/core/css/parser/B...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/336693005/diff/90001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:405: case
CSSPropertyImageRendering: // auto | optimizeContrast
On 2014/06/20 04:26:50, Mike Lawther (Google) wrote:
> Can you please update this comment for consistency?

Done.

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderHTMLCanvas.cpp (right):

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderHTMLCanvas.cpp:73: interpolationQuality =
InterpolationLow;
On 2014/06/20 04:26:50, Mike Lawther (Google) wrote:
> These should really be the same, ie InterpolationNone. From memory, it was
only
> set to InterpolationLow because that was the value that CoreGraphics used to
> mean 'nearest neighbour'

Done.

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderHTMLCanvas.cpp:76: interpolationQuality =
InterpolationNone;
On 2014/06/20 04:26:50, Mike Lawther (Google) wrote:
> Strictly according to spec, we only use nearest neighbour if we are scaling
up.
> If we are scaling down, then we should be using the default scaling (quoth the
> spec: "When scaling the image up, the "nearest neighbor" or similar algorithm
> must be used, so that the image appears to be simply composed of very large
> pixels. When scaling down, this is the same as auto.").
> 
> I'm fine with filing a bug about this with a FIXME.

Done.

[jackhou, 2014-06-20 07:16:20]

pdr, PTAL for OWNERS

loislo, please review for OWNERS:

Source/devtools/front_end/*

[jackhou, 2014-06-20 08:35:43]

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
File Source/core/rendering/RenderHTMLCanvas.cpp (right):

https://codereview.chromium.org/336693005/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderHTMLCanvas.cpp:73: interpolationQuality =
InterpolationLow;
On 2014/06/20 07:14:44, jackhou wrote:
> On 2014/06/20 04:26:50, Mike Lawther (Google) wrote:
> > These should really be the same, ie InterpolationNone. From memory, it was
> only
> > set to InterpolationLow because that was the value that CoreGraphics used to
> > mean 'nearest neighbour'
> 
> Done.

I realized this would require updating all the tests, which differ by platform.
Also, it would be shipped immediately, whereas pixelated is still experimental.
So probably best to leave it unchanged for now and address it in the follow-up
CL.

[pdr., 2014-06-20 16:54:01]

Woohoo, nice change!

LGTM

[vsevik, 2014-06-24 06:44:05]

I don't think we should add support for this property to DevTools before it is
shipped.

[jackhou, 2014-06-24 06:53:17]

On 2014/06/24 06:44:05, vsevik wrote:
> I don't think we should add support for this property to DevTools before it is
> shipped.

Ah, sounds good. I'll send you another CL when it ships. Thanks for reviewing.

[jackhou1, 2014-06-24 06:57:46]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-24 06:58:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/336693005/220001

[commit-bot: I haz the power, 2014-06-24 08:00:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 08:31:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 08:31:29]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[jackhou1, 2014-06-24 14:03:01]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-06-24 14:03:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/336693005/220001

[commit-bot: I haz the power, 2014-06-24 14:05:26]

Change committed as 176839