Use srcset's resource pixel density to determine intrinsic size

Use srcset's resource pixel density to determine intrinsic size

According to the spec "When an img element has a current pixel density that is not 1.0,
the element's image data must be treated as if its resolution, in device pixels per CSS pixels,
was the current pixel density."

I've added that support using the following changes:
* HTMLSrcsetParser now returns the image candidate to HTMLImageElement.
* HTMLImageElement passes the devicePixelRatio data to RenderImage, which stores it.
* Bitmap images are scaled using the devicePixelRatio at RenderImageResource's intrinsicSize() and imageSize().
* SVG images are scaled using the devicePixelRatio at RenderReplaced::computeAspectRatioInformationForRenderBox.
* Canvas support added at CanvasRenderingContext2D::sizeFor.


Other changes:
* Removed useless dumpAsText from tests
* Modified one of the tests to use testRunner.dumpResourceRequestCallbacks() instead of
  internals.isPreloaded(), since isPreloaded wasn't reporting what the test was intended to test.
* Added some tests to TestExpectations because of bugs discovered during this CL work.

BUG=299556
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159544

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159622

[Yoav Weiss, 2013-09-30 16:21:40]

Following pdr's advice, I added some SVG tests, to make sure intrinsic size
works well on them. It isn't. 

I'll debug and upload a patch that covers SVG images as well.

[pdr., 2013-10-02 02:53:13]

Not LGTM, but I have ideas on how to fix this :)

The core issue with this patch is storing the multiplier on the ImageResource
which is shared by multiple renderers. An example of where this could go wrong
is having the same image used twice on a site, once at 2x and once at 1x. With
this patch, they would both render with the same multiplier.

I can think of two potential solutions to this problem:
1) We could move all images to the SVG model. I put up an updated doc describing
how it works and how we might adapt it for srcset:
https://docs.google.com/a/chromium.org/document/d/1J3MPhE2nSQz_HQ0Ext6ZIlnMzV...
2) Another (possibly simpler) model would be to keep the multiplier on the
HTMLImageElement itself. This would mean keeping all srcset logic out of
ImageLoader and ImageResouce, and relying on HTMLImageElement to handle all
srcset work.

[Yoav Weiss, 2013-10-06 22:22:57]

On 2013/10/02 02:53:13, pdr wrote:
> Not LGTM, but I have ideas on how to fix this :)
> 
> The core issue with this patch is storing the multiplier on the ImageResource
> which is shared by multiple renderers. An example of where this could go wrong
> is having the same image used twice on a site, once at 2x and once at 1x. With
> this patch, they would both render with the same multiplier.

Ok. With ImageResource shared between renderers, I see why my approach won't
work.
> 
> I can think of two potential solutions to this problem:
> 1) We could move all images to the SVG model. I put up an updated doc
describing
> how it works and how we might adapt it for srcset:
>
https://docs.google.com/a/chromium.org/document/d/1J3MPhE2nSQz_HQ0Ext6ZIlnMzV...

I've started implementing that with https://codereview.chromium.org/25627006/.
I also got this approach to work locally for bitmap images.
OTOH, I'm having some trouble with SVG images. I wasn't yet able to find the
"right" place in which I can control the container size of SVG images.
Following your advice on IRC, I modified SVGImage::dataChanged (and
SVGImage::containerSize) to try to influence that.
What I see is that scaling the containerSize there results in smaller displayed
SVG, but within the original container dimensions. That is not really what I
need it to do. 

What I need it to do is when given <img srcset="40x60.svg 2x">, the displayed
image would be of 20x30 pixels (both the image and its container).
Right now what I see is a 20x30 pixels image, inside a 40x60 pixels container.

Do you have any ideas where the actual rendered container size is taken from?

> 2) Another (possibly simpler) model would be to keep the multiplier on the
> HTMLImageElement itself. This would mean keeping all srcset logic out of
> ImageLoader and ImageResouce, and relying on HTMLImageElement to handle all
> srcset work.

I've tried modifying the returned sizes in HTMLImageElement::width/height, but
AFAICT the image dimensions must be modified at
ImageResource::imageSizeForRenderer in order to take effect, at least for bitmap
images. 
I failed to find a way to get from ImageResource back to HTMLImageElement. It's
highly probable that I'm missing something, but since the relationship is "many
to one" I believe I'll need to create a mechanism very similar to
ImageForContainer in order to get it to work. Do you agree? If not, what am I
missing?

[pdr., 2013-10-06 22:41:59]

On 2013/10/06 22:22:57, Yoav Weiss wrote:
> I've started implementing that with https://codereview.chromium.org/25627006/.
> I also got this approach to work locally for bitmap images.
> OTOH, I'm having some trouble with SVG images. I wasn't yet able to find the
> "right" place in which I can control the container size of SVG images.
> Following your advice on IRC, I modified SVGImage::dataChanged (and
> SVGImage::containerSize) to try to influence that.
> What I see is that scaling the containerSize there results in smaller
displayed
> SVG, but within the original container dimensions. That is not really what I
> need it to do. 
> 
> What I need it to do is when given <img srcset="40x60.svg 2x">, the displayed
> image would be of 20x30 pixels (both the image and its container).
> Right now what I see is a 20x30 pixels image, inside a 40x60 pixels container.
> 
> Do you have any ideas where the actual rendered container size is taken from?

Lets use your example to make sure I understand what you did: <img width="100"
height="100" srcset="40x60.svg 2x">

I think you took ImageForContainer and added an additional size or scale to it.
For 40x60.svg we would have the following:
SVGImage -> m_instrinsicSize = 40x60
ImageForContainer -> m_containerSize = 100x100
ImageForContainer -> m_srcSetScale = 2

If the above is correct, all your container sizes should be correct. At draw
time, if you want to modify the SVG's container size you would do something
like:
ImageForContainer::draw(...) {
  m_image->drawForContainer(context, m_containerSize * m_srcSetScale, m_zoom,
dstRect, srcRect, compositeOp, blendMode);
}

And to modify the dimensions that the SVG image is rendered at, you would do
something like:
ImageForContainer::draw(...) {
  GraphicsContextStateSaver stateSaver(*context);
  context->scale(m_srcSetScale);
  m_image->drawForContainer(context, m_containerSize, m_zoom, dstRect, srcRect,
compositeOp, blendMode);
}
(alternatively, you may be able to modify m_zoom)

> 
> > 2) Another (possibly simpler) model would be to keep the multiplier on the
> > HTMLImageElement itself. This would mean keeping all srcset logic out of
> > ImageLoader and ImageResouce, and relying on HTMLImageElement to handle all
> > srcset work.
> 
> I've tried modifying the returned sizes in HTMLImageElement::width/height, but
> AFAICT the image dimensions must be modified at
> ImageResource::imageSizeForRenderer in order to take effect, at least for
bitmap
> images. 
> I failed to find a way to get from ImageResource back to HTMLImageElement.
It's
> highly probable that I'm missing something, but since the relationship is
"many
> to one" I believe I'll need to create a mechanism very similar to
> ImageForContainer in order to get it to work. Do you agree? If not, what am I
> missing?

I'd like to double-check that my understanding is correct here:
It's my understanding that srcset will still have one image per renderer; is
this correct? For example, if you have:
<img id="foo" src="bar1.png" srcset="bar2.png 2x">
we would only have one image for the renderer at any single time (either
bar1.png or bar2.png)?

To your question:
I certainly didn't think this all the way through and there could be issues with
this method. My thinking was that you could hide almost all the implementation
details of srcset in HTMLImageElement. For example, if you have:
<img id="foo" src="bar1.png" srcset="bar2.png 2x">
the image element would have this map:
"bar1.png" -> 1
"bar2.png" -> 2
In this model, imageSizeForRenderer would return the "wrong" value and you would
have to modify callers to call something on HTMLImageElement to adjust for the
srcset scale. Because srcset is only applicable on the html image element, all
users (e.g., RenderImage) should be able to access the original HTMLImageElement
using node(). HTMLImageElement::naturalWidth() would do the dirty work of
returning 2 * width for the 2x image, for example. The drawing methods on
RenderImage would likely require some modifications.

[Yoav Weiss, 2013-10-07 21:20:21]

On 2013/10/06 22:41:59, pdr wrote:
> On 2013/10/06 22:22:57, Yoav Weiss wrote:
> > I've started implementing that with
https://codereview.chromium.org/25627006/.
> > I also got this approach to work locally for bitmap images.
> > OTOH, I'm having some trouble with SVG images. I wasn't yet able to find the
> > "right" place in which I can control the container size of SVG images.
> > Following your advice on IRC, I modified SVGImage::dataChanged (and
> > SVGImage::containerSize) to try to influence that.
> > What I see is that scaling the containerSize there results in smaller
> displayed
> > SVG, but within the original container dimensions. That is not really what I
> > need it to do. 
> > 
> > What I need it to do is when given <img srcset="40x60.svg 2x">, the
displayed
> > image would be of 20x30 pixels (both the image and its container).
> > Right now what I see is a 20x30 pixels image, inside a 40x60 pixels
container.
> > 
> > Do you have any ideas where the actual rendered container size is taken
from?
> 
> Lets use your example to make sure I understand what you did: <img width="100"
> height="100" srcset="40x60.svg 2x">

That's actually not the example I'm using. I'm using: <img srcset="40x60.svg
2x"> (without any explicit HTML dimensions)
So 
SVGImage -> m_instrinsicSize = 40x60
ImageForContainer -> m_containerSize = 40x60
Modifying ImageForContainer->m_containerSize doesn't seem to have much of an
impact.
Modifying m_intrinsicSize results in smaller displayed image (e.g. scale(0.5)
results in 20x30 painted image), but it takes up the same space (so 40x60 in our
example)
Here's a link to the displayed image, when added with a black border:
http://ubuntuone.com/2tKFNbZgEhExKYAhsNT0zr

> 
> I think you took ImageForContainer and added an additional size or scale to
it.
> For 40x60.svg we would have the following:
> SVGImage -> m_instrinsicSize = 40x60
> ImageForContainer -> m_containerSize = 100x100
> ImageForContainer -> m_srcSetScale = 2
> 
> If the above is correct, all your container sizes should be correct. At draw
> time, if you want to modify the SVG's container size you would do something
> like:
> ImageForContainer::draw(...) {
>   m_image->drawForContainer(context, m_containerSize * m_srcSetScale, m_zoom,
> dstRect, srcRect, compositeOp, blendMode);
> }
> 
> And to modify the dimensions that the SVG image is rendered at, you would do
> something like:
> ImageForContainer::draw(...) {
>   GraphicsContextStateSaver stateSaver(*context);
>   context->scale(m_srcSetScale);
>   m_image->drawForContainer(context, m_containerSize, m_zoom, dstRect,
srcRect,
> compositeOp, blendMode);
> }
> (alternatively, you may be able to modify m_zoom)

I tried that, without results for now. (Scaling the GraphicsContext causes the
SVG images to disappear :/) I'll keep digging.

> 
> > 
> > > 2) Another (possibly simpler) model would be to keep the multiplier on the
> > > HTMLImageElement itself. This would mean keeping all srcset logic out of
> > > ImageLoader and ImageResouce, and relying on HTMLImageElement to handle
all
> > > srcset work.
> > 
> > I've tried modifying the returned sizes in HTMLImageElement::width/height,
but
> > AFAICT the image dimensions must be modified at
> > ImageResource::imageSizeForRenderer in order to take effect, at least for
> bitmap
> > images. 
> > I failed to find a way to get from ImageResource back to HTMLImageElement.
> It's
> > highly probable that I'm missing something, but since the relationship is
> "many
> > to one" I believe I'll need to create a mechanism very similar to
> > ImageForContainer in order to get it to work. Do you agree? If not, what am
I
> > missing?
> 
> I'd like to double-check that my understanding is correct here:
> It's my understanding that srcset will still have one image per renderer; is
> this correct? For example, if you have:
> <img id="foo" src="bar1.png" srcset="bar2.png 2x">
> we would only have one image for the renderer at any single time (either
> bar1.png or bar2.png)?

Yes, srcset will have a single image per renderer.
> 
> To your question:
> I certainly didn't think this all the way through and there could be issues
with
> this method. My thinking was that you could hide almost all the implementation
> details of srcset in HTMLImageElement. For example, if you have:
> <img id="foo" src="bar1.png" srcset="bar2.png 2x">
> the image element would have this map:
> "bar1.png" -> 1
> "bar2.png" -> 2
> In this model, imageSizeForRenderer would return the "wrong" value and you
would
> have to modify callers to call something on HTMLImageElement to adjust for the
> srcset scale. Because srcset is only applicable on the html image element, all
> users (e.g., RenderImage) should be able to access the original
HTMLImageElement
> using node(). HTMLImageElement::naturalWidth() would do the dirty work of
> returning 2 * width for the 2x image, for example. The drawing methods on
> RenderImage would likely require some modifications.

Oh, now I see what you mean. This may very well be a simpler method for bitmap
images. I'll look into that.

[pdr., 2013-10-08 06:36:04]

On 2013/10/07 21:20:21, Yoav Weiss wrote:
> That's actually not the example I'm using. I'm using: <img srcset="40x60.svg
> 2x"> (without any explicit HTML dimensions)
> So 
> SVGImage -> m_instrinsicSize = 40x60
> ImageForContainer -> m_containerSize = 40x60
> Modifying ImageForContainer->m_containerSize doesn't seem to have much of an
> impact.
> Modifying m_intrinsicSize results in smaller displayed image (e.g. scale(0.5)
> results in 20x30 painted image), but it takes up the same space (so 40x60 in
our
> example)
> Here's a link to the displayed image, when added with a black border:
> http://ubuntuone.com/2tKFNbZgEhExKYAhsNT0zr
> 

I apologize, I was trying to cover two things at once. I created a few testcases
at http://pr.gg/srcset.html. These reflect my understanding of srcset (the
values in italic are the current values returned by js). Do you mind taking a
look and making sure I'm not off in the weeds?

I talked with Tab on IRC and confirmed canvas.drawImage(<img srcset="100x100.png
2x">) should draw at 50x50. Because the canvas spec is explicit about using the
intrinsic size for drawImage, we have an unfortunate mixing of terms that makes
SVGImage's m_intrinsicSize very confusing. SVGImage's m_intrinsicSize is the
explicit size of the svg image before any container or context and because it's
shared we shouldn't modify it. Canvas uses ImageResource::imageSizeForRenderer
so we should make sure that returns the correct value for SVG.

If you can post an updated patch with the testcase that's failing, I can dive in
a bit deeper.

> > 
> > I think you took ImageForContainer and added an additional size or scale to
> it.
> > For 40x60.svg we would have the following:
> > SVGImage -> m_instrinsicSize = 40x60
> > ImageForContainer -> m_containerSize = 100x100
> > ImageForContainer -> m_srcSetScale = 2
> > 
> > If the above is correct, all your container sizes should be correct. At draw
> > time, if you want to modify the SVG's container size you would do something
> > like:
> > ImageForContainer::draw(...) {
> >   m_image->drawForContainer(context, m_containerSize * m_srcSetScale,
m_zoom,
> > dstRect, srcRect, compositeOp, blendMode);
> > }
> > 
> > And to modify the dimensions that the SVG image is rendered at, you would do
> > something like:
> > ImageForContainer::draw(...) {
> >   GraphicsContextStateSaver stateSaver(*context);
> >   context->scale(m_srcSetScale);
> >   m_image->drawForContainer(context, m_containerSize, m_zoom, dstRect,
> srcRect,
> > compositeOp, blendMode);
> > }
> > (alternatively, you may be able to modify m_zoom)
> 
> I tried that, without results for now. (Scaling the GraphicsContext causes the
> SVG images to disappear :/) I'll keep digging.

Scaling the context probably requires accounting for the current offset. For
example, you may have to inversely scale the dstRect and zoom. Another option
may be to just change dstRect.

> 
> > 
> > > 
> > > > 2) Another (possibly simpler) model would be to keep the multiplier on
the
> > > > HTMLImageElement itself. This would mean keeping all srcset logic out of
> > > > ImageLoader and ImageResouce, and relying on HTMLImageElement to handle
> all
> > > > srcset work.
> > > 
> > > I've tried modifying the returned sizes in HTMLImageElement::width/height,
> but
> > > AFAICT the image dimensions must be modified at
> > > ImageResource::imageSizeForRenderer in order to take effect, at least for
> > bitmap
> > > images. 
> > > I failed to find a way to get from ImageResource back to HTMLImageElement.
> > It's
> > > highly probable that I'm missing something, but since the relationship is
> > "many
> > > to one" I believe I'll need to create a mechanism very similar to
> > > ImageForContainer in order to get it to work. Do you agree? If not, what
am
> I
> > > missing?
> > 
> > 
> > To your question:
> > I certainly didn't think this all the way through and there could be issues
> with
> > this method. My thinking was that you could hide almost all the
implementation
> > details of srcset in HTMLImageElement. For example, if you have:
> > <img id="foo" src="bar1.png" srcset="bar2.png 2x">
> > the image element would have this map:
> > "bar1.png" -> 1
> > "bar2.png" -> 2
> > In this model, imageSizeForRenderer would return the "wrong" value and you
> would
> > have to modify callers to call something on HTMLImageElement to adjust for
the
> > srcset scale. Because srcset is only applicable on the html image element,
all
> > users (e.g., RenderImage) should be able to access the original
> HTMLImageElement
> > using node(). HTMLImageElement::naturalWidth() would do the dirty work of
> > returning 2 * width for the 2x image, for example. The drawing methods on
> > RenderImage would likely require some modifications.
> 
> Oh, now I see what you mean. This may very well be a simpler method for bitmap
> images. I'll look into that.

This has the advantage that svg images should come for free without special
handling. I saw the CSS image-set() property mentioned on the whatwg mailing
list. If this is similar to srcset, we may have to go with option #1
(ImageForContainer, etc) because stuffing srcset in HTMLImageElement won't be
generalizable to CSS :/

[Yoav Weiss, 2013-10-08 07:42:53]

> I created a few testcases
> at http://pr.gg/srcset.html. These reflect my understanding of srcset (the
> values in italic are the current values returned by js). Do you mind taking a
> look and making sure I'm not off in the weeds?

Looks good! (Besides the fact that all canvas tests break horribly in my current
build)

> 
> I talked with Tab on IRC and confirmed canvas.drawImage(<img
srcset="100x100.png
> 2x">) should draw at 50x50. Because the canvas spec is explicit about using
the
> intrinsic size for drawImage, we have an unfortunate mixing of terms that
makes
> SVGImage's m_intrinsicSize very confusing. SVGImage's m_intrinsicSize is the
> explicit size of the svg image before any container or context and because
it's
> shared we shouldn't modify it.

Maybe we can just rename it to m_naturalSize and leave it be?

> Canvas uses ImageResource::imageSizeForRenderer
> so we should make sure that returns the correct value for SVG.

So supporting canvas favors option #1?

> 
> If you can post an updated patch with the testcase that's failing, I can dive
in
> a bit deeper.

Sure. I'll get my latest option #1 branch into shape and upload. 

> 
> > > 
> > > I think you took ImageForContainer and added an additional size or scale
to
> > it.
> > > For 40x60.svg we would have the following:
> > > SVGImage -> m_instrinsicSize = 40x60
> > > ImageForContainer -> m_containerSize = 100x100
> > > ImageForContainer -> m_srcSetScale = 2
> > > 
> > > If the above is correct, all your container sizes should be correct. At
draw
> > > time, if you want to modify the SVG's container size you would do
something
> > > like:
> > > ImageForContainer::draw(...) {
> > >   m_image->drawForContainer(context, m_containerSize * m_srcSetScale,
> m_zoom,
> > > dstRect, srcRect, compositeOp, blendMode);
> > > }
> > > 
> > > And to modify the dimensions that the SVG image is rendered at, you would
do
> > > something like:
> > > ImageForContainer::draw(...) {
> > >   GraphicsContextStateSaver stateSaver(*context);
> > >   context->scale(m_srcSetScale);
> > >   m_image->drawForContainer(context, m_containerSize, m_zoom, dstRect,
> > srcRect,
> > > compositeOp, blendMode);
> > > }
> > > (alternatively, you may be able to modify m_zoom)
> > 
> > I tried that, without results for now. (Scaling the GraphicsContext causes
the
> > SVG images to disappear :/) I'll keep digging.
> 
> Scaling the context probably requires accounting for the current offset. For
> example, you may have to inversely scale the dstRect and zoom. Another option
> may be to just change dstRect.

OK. Will try.

> 
> This has the advantage that svg images should come for free without special
> handling. I saw the CSS image-set() property mentioned on the whatwg mailing
> list. If this is similar to srcset, we may have to go with option #1
> (ImageForContainer, etc) because stuffing srcset in HTMLImageElement won't be
> generalizable to CSS :/

We need this mechanism to work for srcset, src-N, Client-Hints and image-set()
(according to http://lists.w3.org/Archives/Public/www-style/2012Feb/1103.html).
The first 3 can hang off HTMLImageElement, but as you say, image-set cannot. 
So, option #1 is the winner? Or should we consider an unrelated implementation
for image-set?

[Yoav Weiss, 2013-10-08 16:42:20]

> > 
> > If you can post an updated patch with the testcase that's failing, I can
dive
> in
> > a bit deeper.
> 
> Sure. I'll get my latest option #1 branch into shape and upload. 
> 

I've uploaded my latest patch, which is *not yet ready*:
* Bitmap images are working as they should.
* SVG images are not (the failing test case is at
fast/hidpi/image-srcset-intrinsic-size.html).
* Canvas also doesn't work (Didn't add a failing test case yet)
* object-fit for delayed images broke
(http/tests/css/object-fit-delayed-img-svg.html)
* Several SVG tests show flakiness that wasn't there before

[Yoav Weiss, 2013-10-09 04:52:56]

Moved to store the intrinsicSizeFactor in RenderImage, following eseidel's
suggestion.
I've also added pdr's SVG test page.

I've uploaded my latest patch, which is (again) *not yet ready*:
* Bitmap images are working as they should.
* Canvas (with bitmap images) working as it should.
* SVG images are not (the failing test case is at
fast/hidpi/image-srcset-intrinsic-size.html).
* object-fit for delayed images broke (again)
(http/tests/css/object-fit-delayed-img-svg.html)
* Several SVG tests show flakiness that wasn't there before (again)

[Yoav Weiss, 2013-10-09 21:03:15]

With latest patch, PNG and SVG images are working fine.
Canvas works as well, with both PNG and SVG images.

Open issues:
* Canvas + SVG +2X shows some flakiness, and once in a while shows an image
picked with the wrong dimensions.
* Some SVG tests show flakiness

I've also added failing tests to TestExpectations for:
* An unrelated issue with the srcset parser
* Image-set not responding to deviceScaleFactor

[Yoav Weiss, 2013-10-10 21:08:43]

Got everything working.

> 
> Open issues:
> * Canvas + SVG +2X shows some flakiness, and once in a while shows an image
> picked with the wrong dimensions.

This flakiness is not related to my changes, and appears also in 1x canvas+SVG.
See http://code.google.com/p/chromium/issues/detail?id=306222

> * Some SVG tests show flakiness

Since flakiness is mostly in image tests, and not visible, may I rebaseline
these tests?

[pdr., 2013-10-11 05:17:33]

This patch is fantastic Yoav, thank you for seeing it through.

I think this is almost ready to go, lets just do one more round of reviews.

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:653: #crbug.com/245611 [ Win ]
fast/hidpi/image-set-without-specified-width.html [ ImageOnlyFailure Pass ]
Can you add a comment above the Skipped versions of these that they need to be
uncommented when the tests are unskipped?

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/fast/hidpi/im...
File
LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt
(right):

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/fast/hidpi/im...
LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt:5:
main frame - img requested 'blue-100-px-square.png'
Why is blue-100-px-square.png loaded here?

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.cpp:160:
image->setIntrinsicSizeFactor(m_intrinsicSizeFactor);
I'm not sure this will work for dynamic changes. For example:
<img srcset="mustard.png 2x">
<script>
document.body.onload = function() {
  var img = document.getElementsByTagName('img')[0];
  img.srcset = "ketchup.png 3x";
}
</script>

In this example, we wont create a new RenderImage but m_intrinsicSizeFactor will
change. One solution may be to set the value in parseAttribute if there's
already a renderer, similar to how the alt text is updated.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1215: static LayoutSize
sizeFor(HTMLImageElement* image, ImageSizeType sizeType)
Can you help me understand why this is needed?

I thought canvas could only access the image size without the scale factor.
For example:
<img srcset="100x100.png 2x">

ctx.drawImage(img, 0, 0) should render at 50x50
and
ctx.drawImage(img, 0, 0, 100, 100) should render the original image (without
upscaling) at 100x100
and
ctx.drawImage(img, 0, 0, 1000, 1000) should render the original image upscaled
to 1000x1000

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:170: intrinsicSizeFactor = 1 /
candidate.scaleFactor();
Do we need to check that scaleFactor is non-zero here?

The spec requires this but it doesn't seem to be enforced in HTMLSrcsetParser.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:73: String
bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
srcAttribute, const String& srcsetAttribute, float& scaleFactor);
This is a little confusing because the function returns values in two ways. What
do you think about returning an ImageCandidate instead?

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:615: {
Can you add an assert here:
ASSERT(m_imageResource);

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:618: if (cachedImage &&
cachedImage->image() && cachedImage->image()->isSVGImage())
I don't understand the cachedImage->image()->isSVGImage() check here. Shouldn't
this apply to all images?

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.h:66: LayoutSize unscaledIntrinsicSize()
const;
Can we change this to something more descriptive? Possibly
"intrinsicSizeWithoutSrcsetDevicePixelRatio()"?
Similarly for the intrinsicSizeFactor. Possibly "m_srcsetDevicePixelRatio" and
"setSrcsetDevicePixelRatio(...)"?
Similarly for HTMLImageElement, etc.

This sounds pedantic but there are already several scale factors (ctm scale,
screen's device pixel ratio, page zoom scale) and it sounds like we're even
going to add another device pixel ratio soon (for client hints).

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImageResource.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImageResource.cpp:108: LayoutSize size =
m_cachedImage ? m_cachedImage->imageSizeForRenderer(m_renderer, multiplier,
type) : LayoutSize();
May want to slightly refactor this to not scale size if there's no
m_cachedImage.

For example:
if (!m_cachedImage)
  return LayoutSize();

LayoutSize size = m_cachedImage->imageSizeForRenderer(...);
if (m_renderer ... etc

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:272: virtual float intrinsicSizeFactor()
const { return 1.0f; }
I think we can get away without adding this. Instead, just add a check & cast
for RenderImage.

For example, getImageSize might look like:
if (m_renderer && m_renderer->isRenderImage())
  size.scale(toRenderImage(m_renderer)->intrinsicSizeFactor());

[Yoav Weiss, 2013-10-11 07:22:31]

Thanks for the review! My comments below.

One more question, is it OK if I rebaseline the few SVG tests that now fail for
minor (practically invisible) pixel differences?

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:653: #crbug.com/245611 [ Win ]
fast/hidpi/image-set-without-specified-width.html [ ImageOnlyFailure Pass ]
On 2013/10/11 05:17:33, pdr wrote:
> Can you add a comment above the Skipped versions of these that they need to be
> uncommented when the tests are unskipped?

Sure.

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/fast/hidpi/im...
File
LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt
(right):

https://codereview.chromium.org/25105004/diff/91001/LayoutTests/fast/hidpi/im...
LayoutTests/fast/hidpi/image-srcset-change-dynamically-from-js-2x-expected.txt:5:
main frame - img requested 'blue-100-px-square.png'
On 2013/10/11 05:17:33, pdr wrote:
> Why is blue-100-px-square.png loaded here?

The page is loaded twice, where the first load set the deviceScaleFactor to 2
and then reloads. (That the standard hack to change the deviceScaleFactor.
Therefore, the first load has a deviceScaleFactor of 1, and loads the 1x
resources.

*But*, I guess the interesting question is why is green-200-px-square.png
requested on the first page load.

That looks like a bug in the srcset implementation! (Not really related to this
CL) I'll file one.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/HTMLImag...
File Source/core/html/HTMLImageElement.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/HTMLImag...
Source/core/html/HTMLImageElement.cpp:160:
image->setIntrinsicSizeFactor(m_intrinsicSizeFactor);
On 2013/10/11 05:17:33, pdr wrote:
> I'm not sure this will work for dynamic changes. For example:
> <img srcset="mustard.png 2x">
> <script>
> document.body.onload = function() {
>   var img = document.getElementsByTagName('img')[0];
>   img.srcset = "ketchup.png 3x";
> }
> </script>
> 
> In this example, we wont create a new RenderImage but m_intrinsicSizeFactor
will
> change. One solution may be to set the value in parseAttribute if there's
> already a renderer, similar to how the alt text is updated.

Good catch! Will fix.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/C...
File Source/core/html/canvas/CanvasRenderingContext2D.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/C...
Source/core/html/canvas/CanvasRenderingContext2D.cpp:1215: static LayoutSize
sizeFor(HTMLImageElement* image, ImageSizeType sizeType)
On 2013/10/11 05:17:33, pdr wrote:
> Can you help me understand why this is needed?
> 
> I thought canvas could only access the image size without the scale factor.
> For example:
> <img srcset="100x100.png 2x">
> 
> ctx.drawImage(img, 0, 0) should render at 50x50
> and
> ctx.drawImage(img, 0, 0, 100, 100) should render the original image (without
> upscaling) at 100x100
> and
> ctx.drawImage(img, 0, 0, 1000, 1000) should render the original image upscaled
> to 1000x1000

For what I've seen, drawImage calls sizeFor 3 times:
* In CanvasRenderingContext2D:1376 it is called to determine the destination
canvas size. That size should be scaled (so 50x50 as you said for
ctx.drawImage(img, 0, 0))
* In CanvasRenderingContext2D:1387 it is called to determine the source image
size. Giving it scaled dimensions here resorts in only a quarter of the image
drawn.
* In CanvasRenderingContext2D:1418 it is called to determine the image
dimensions for clipping purposes. Giving scaled image dimensions here results in
image distortion.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.cpp:170: intrinsicSizeFactor = 1 /
candidate.scaleFactor();
On 2013/10/11 05:17:33, pdr wrote:
> Do we need to check that scaleFactor is non-zero here?
> 
> The spec requires this but it doesn't seem to be enforced in HTMLSrcsetParser.

Yes, we should. Will fix.

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
File Source/core/html/parser/HTMLSrcsetParser.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/html/parser/H...
Source/core/html/parser/HTMLSrcsetParser.h:73: String
bestFitSourceForImageAttributes(float deviceScaleFactor, const String&
srcAttribute, const String& srcsetAttribute, float& scaleFactor);
On 2013/10/11 05:17:33, pdr wrote:
> This is a little confusing because the function returns values in two ways.
What
> do you think about returning an ImageCandidate instead?

OK.

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:615: {
On 2013/10/11 05:17:33, pdr wrote:
> Can you add an assert here:
> ASSERT(m_imageResource);

Sure

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.cpp:618: if (cachedImage &&
cachedImage->image() && cachedImage->image()->isSVGImage())
On 2013/10/11 05:17:33, pdr wrote:
> I don't understand the cachedImage->image()->isSVGImage() check here.
Shouldn't
> this apply to all images?

It should. Applying it to bitmaps breaks them. Currently, I'm not sure why.
I'll see if I can come up with a good explanation as to why this works the way
it does.

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImage.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImage.h:66: LayoutSize unscaledIntrinsicSize()
const;
I think (hope) this mechanism is not srcset specific, and CH, image-set and srcN
could use it just as well.

Maybe intrinsicSizeWithoutDevicePixelRatio() ?

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderImageResource.cpp (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderImageResource.cpp:108: LayoutSize size =
m_cachedImage ? m_cachedImage->imageSizeForRenderer(m_renderer, multiplier,
type) : LayoutSize();
On 2013/10/11 05:17:33, pdr wrote:
> May want to slightly refactor this to not scale size if there's no
> m_cachedImage.
> 
> For example:
> if (!m_cachedImage)
>   return LayoutSize();
> 
> LayoutSize size = m_cachedImage->imageSizeForRenderer(...);
> if (m_renderer ... etc
OK

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
File Source/core/rendering/RenderObject.h (right):

https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
Source/core/rendering/RenderObject.h:272: virtual float intrinsicSizeFactor()
const { return 1.0f; }
On 2013/10/11 05:17:33, pdr wrote:
> I think we can get away without adding this. Instead, just add a check & cast
> for RenderImage.
> 
> For example, getImageSize might look like:
> if (m_renderer && m_renderer->isRenderImage())
>   size.scale(toRenderImage(m_renderer)->intrinsicSizeFactor());

OK

[Yoav Weiss, 2013-10-11 08:45:04]

> One more question, is it OK if I rebaseline the few SVG tests that now fail
for
> minor (practically invisible) pixel differences?

Nevermind that. I see the bot failures are now for other tests (not the SVG ones
I've seen earlier).

[Yoav Weiss, 2013-10-11 17:05:46]

Addressed the review comments and uploaded the modified patch.

>
https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
> Source/core/rendering/RenderImage.cpp:618: if (cachedImage &&
> cachedImage->image() && cachedImage->image()->isSVGImage())
> On 2013/10/11 05:17:33, pdr wrote:
> > I don't understand the cachedImage->image()->isSVGImage() check here.
> Shouldn't
> > this apply to all images?
> 
> It should. Applying it to bitmaps breaks them. Currently, I'm not sure why.
> I'll see if I can come up with a good explanation as to why this works the way
> it does.
> 

I now have a better understanding as to why applying this to PNGs breaks them. 
Apparently for PNGs, the m_intrinsicSize value of RenderReplaced is set using
the value returned from this function.
I'm not sure where that happens (I wish I did), but scaling PNGs here results in
a loop where they are scaled down to infinitesimal values.
SVGs don't suffer from the same behavior, so scaling them here is not a problem.
Ignoring PNGs here is also not a problem, since they are scaled down in
RenderImageResource::getImageSize.

[pdr., 2013-10-11 18:26:38]

On 2013/10/11 07:22:31, Yoav Weiss wrote:
>
https://codereview.chromium.org/25105004/diff/91001/Source/core/html/canvas/C...
> Source/core/html/canvas/CanvasRenderingContext2D.cpp:1215: static LayoutSize
> sizeFor(HTMLImageElement* image, ImageSizeType sizeType)
> On 2013/10/11 05:17:33, pdr wrote:
> > Can you help me understand why this is needed?
> > 
> > I thought canvas could only access the image size without the scale factor.
> > For example:
> > <img srcset="100x100.png 2x">
> > 
> > ctx.drawImage(img, 0, 0) should render at 50x50
> > and
> > ctx.drawImage(img, 0, 0, 100, 100) should render the original image (without
> > upscaling) at 100x100
> > and
> > ctx.drawImage(img, 0, 0, 1000, 1000) should render the original image
upscaled
> > to 1000x1000
> 
> For what I've seen, drawImage calls sizeFor 3 times:
> * In CanvasRenderingContext2D:1376 it is called to determine the destination
> canvas size. That size should be scaled (so 50x50 as you said for
> ctx.drawImage(img, 0, 0))
> * In CanvasRenderingContext2D:1387 it is called to determine the source image
> size. Giving it scaled dimensions here resorts in only a quarter of the image
> drawn.
> * In CanvasRenderingContext2D:1418 it is called to determine the image
> dimensions for clipping purposes. Giving scaled image dimensions here results
in
> image distortion.

Ahh, I see now! You're using the unscaled value for the source rect.

I have two requests here then:
1) Can you change the variable name "size" to "sourceRectSize" where appropriate
in the drawImage functions?
2) Can you add a comment to the enum to make it clear what these two sizes are.
Possibly even rename ScaledSize to ImageSizeAfterSrcsetDevicePixelRatio.

I really don't like when reviewers bikeshed on names but a year from now we will
have forgotten how srcset device pixel ratio works and I think this will help us
remember.

>
https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
> Source/core/rendering/RenderImage.h:66: LayoutSize unscaledIntrinsicSize()
> const;
> I think (hope) this mechanism is not srcset specific, and CH, image-set and
srcN
> could use it just as well.

I think CH will have to work slightly differently from srcset/srcN. CH will
actually store its device pixel ratio on Image.h. Some of the same changes to
context2d will need to apply though, so I think it's likely we'll have three
device pixel ratios for images(the physical screen's dpr, srcset's dpr, and ch's
dpr).

[pdr., 2013-10-11 18:29:38]

On 2013/10/11 17:05:46, Yoav Weiss wrote:
> Addressed the review comments and uploaded the modified patch.
> 
> >
>
https://codereview.chromium.org/25105004/diff/91001/Source/core/rendering/Ren...
> > Source/core/rendering/RenderImage.cpp:618: if (cachedImage &&
> > cachedImage->image() && cachedImage->image()->isSVGImage())
> > On 2013/10/11 05:17:33, pdr wrote:
> > > I don't understand the cachedImage->image()->isSVGImage() check here.
> > Shouldn't
> > > this apply to all images?
> > 
> > It should. Applying it to bitmaps breaks them. Currently, I'm not sure why.
> > I'll see if I can come up with a good explanation as to why this works the
way
> > it does.
> > 
> 
> I now have a better understanding as to why applying this to PNGs breaks them.

> Apparently for PNGs, the m_intrinsicSize value of RenderReplaced is set using
> the value returned from this function.
> I'm not sure where that happens (I wish I did), but scaling PNGs here results
in
> a loop where they are scaled down to infinitesimal values.
> SVGs don't suffer from the same behavior, so scaling them here is not a
problem.
> Ignoring PNGs here is also not a problem, since they are scaled down in
> RenderImageResource::getImageSize.

I think we'll have to figure this one out before landing. Do you mind digging a
bit further?

[Yoav Weiss, 2013-10-11 22:18:07]

Renamed intrinsicSizeFactor to imageDevicePixelRatio & ScaledSize to
imageSizeAfterDevicePixelRatio

> > I now have a better understanding as to why applying this to PNGs breaks
them.
> 
> > Apparently for PNGs, the m_intrinsicSize value of RenderReplaced is set
using
> > the value returned from this function.
> > I'm not sure where that happens (I wish I did), but scaling PNGs here
results
> in
> > a loop where they are scaled down to infinitesimal values.
> > SVGs don't suffer from the same behavior, so scaling them here is not a
> problem.
> > Ignoring PNGs here is also not a problem, since they are scaled down in
> > RenderImageResource::getImageSize.
> 
> I think we'll have to figure this one out before landing. Do you mind digging
a
> bit further?

Removed SVG scaling from intrinsicSize() into
RenderReplaced::computeAspectRatioInformationForRenderBox, right next to the
effective zoom scaling.
I feel more comfortable with scaling here (instead of intrinsicSize()), but
bitmap scaling still happens elsewhere.

Scaling bitmaps here results in a similar loop to the one I've seen earlier, do
I've left bitmap scaling in RenderImageResource::getImageSize, where it was.

[pdr., 2013-10-13 00:12:13]

On 2013/10/11 22:18:07, Yoav Weiss wrote:
> Removed SVG scaling from intrinsicSize() into
> RenderReplaced::computeAspectRatioInformationForRenderBox, right next to the
> effective zoom scaling.
> I feel more comfortable with scaling here (instead of intrinsicSize()), but
> bitmap scaling still happens elsewhere.
> 
> Scaling bitmaps here results in a similar loop to the one I've seen earlier,
do
> I've left bitmap scaling in RenderImageResource::getImageSize, where it was.

I think this is very reasonable, and it aligns with the zoom calculation as
well.

[pdr., 2013-10-13 00:12:44]

LGTM!

[pdr., 2013-10-13 00:14:06]

Please update the change description to reflect the new approach before landing.

[commit-bot: I haz the power, 2013-10-13 08:16:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/134001

[commit-bot: I haz the power, 2013-10-13 09:57:17]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-13 14:51:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/147001

[commit-bot: I haz the power, 2013-10-13 14:52:09]

Failed to apply patch for Source/core/html/canvas/CanvasRenderingContext2D.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/html/canvas/CanvasRenderingContext2D.cpp
  Hunk #1 FAILED at 60.
  Hunk #2 succeeded at 1212 (offset 5 lines).
  Hunk #3 succeeded at 1378 (offset 5 lines).
  Hunk #4 succeeded at 1389 (offset 5 lines).
  Hunk #5 succeeded at 1420 (offset 5 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
Source/core/html/canvas/CanvasRenderingContext2D.cpp.rej

Patch:       Source/core/html/canvas/CanvasRenderingContext2D.cpp
Index: Source/core/html/canvas/CanvasRenderingContext2D.cpp
diff --git a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
index
4bc56b201c406dea598180b30114f4f276b1512c..416e3c79c358cd0086e609df847776f7fe4f2965
100644
--- a/Source/core/html/canvas/CanvasRenderingContext2D.cpp
+++ b/Source/core/html/canvas/CanvasRenderingContext2D.cpp
@@ -60,6 +60,7 @@
 #include "core/platform/graphics/FontCache.h"
 #include "core/platform/graphics/GraphicsContextStateSaver.h"
 #include "core/platform/graphics/TextRun.h"
+#include "core/rendering/RenderImage.h"
 #include "core/rendering/RenderLayer.h"
 #include "core/rendering/RenderTheme.h"
 #include "platform/geometry/FloatQuad.h"
@@ -1207,11 +1208,22 @@ bool CanvasRenderingContext2D::shouldDrawShadows() const
     return alphaChannel(state().m_shadowColor) && (state().m_shadowBlur ||
!state().m_shadowOffset.isZero());
 }
 
-static LayoutSize sizeFor(HTMLImageElement* image)
+enum ImageSizeType {
+    ImageSizeAfterDevicePixelRatio,
+    ImageSizeBeforeDevicePixelRatio
+};
+
+static LayoutSize sizeFor(HTMLImageElement* image, ImageSizeType sizeType)
 {
-    if (ImageResource* cachedImage = image->cachedImage())
-        return cachedImage->imageSizeForRenderer(image->renderer(), 1.0f); //
FIXME: Not sure about this.
-    return IntSize();
+    LayoutSize size;
+    ImageResource* cachedImage = image->cachedImage();
+    if (cachedImage) {
+        size = cachedImage->imageSizeForRenderer(image->renderer(), 1.0f); //
FIXME: Not sure about this.
+
+        if (sizeType == ImageSizeAfterDevicePixelRatio && image->renderer() &&
image->renderer()->isRenderImage() && cachedImage->image() &&
!cachedImage->image()->hasRelativeWidth())
+           
size.scale(toRenderImage(image->renderer())->imageDevicePixelRatio());
+    }
+    return size;
 }
 
 static IntSize sizeFor(HTMLVideoElement* video)
@@ -1362,8 +1374,8 @@ void CanvasRenderingContext2D::drawImage(HTMLImageElement*
image, float x, float
         es.throwUninformativeAndGenericDOMException(TypeMismatchError);
         return;
     }
-    LayoutSize size = sizeFor(image);
-    drawImage(image, x, y, size.width(), size.height(), es);
+    LayoutSize destRectSize = sizeFor(image, ImageSizeAfterDevicePixelRatio);
+    drawImage(image, x, y, destRectSize.width(), destRectSize.height(), es);
 }
 
 void CanvasRenderingContext2D::drawImage(HTMLImageElement* image,
@@ -1373,8 +1385,8 @@ void CanvasRenderingContext2D::drawImage(HTMLImageElement*
image,
         es.throwUninformativeAndGenericDOMException(TypeMismatchError);
         return;
     }
-    LayoutSize size = sizeFor(image);
-    drawImage(image, FloatRect(0, 0, size.width(), size.height()), FloatRect(x,
y, width, height), es);
+    LayoutSize sourceRectSize = sizeFor(image,
ImageSizeBeforeDevicePixelRatio);
+    drawImage(image, FloatRect(0, 0, sourceRectSize.width(),
sourceRectSize.height()), FloatRect(x, y, width, height), es);
 }
 
 void CanvasRenderingContext2D::drawImage(HTMLImageElement* image,
@@ -1404,7 +1416,7 @@ void CanvasRenderingContext2D::drawImage(HTMLImageElement*
image, const FloatRec
     if (!cachedImage || !image->complete())
         return;
 
-    LayoutSize size = sizeFor(image);
+    LayoutSize size = sizeFor(image, ImageSizeBeforeDevicePixelRatio);
     if (!size.width() || !size.height()) {
         es.throwUninformativeAndGenericDOMException(InvalidStateError);
         return;

[commit-bot: I haz the power, 2013-10-13 16:09:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/160001

[commit-bot: I haz the power, 2013-10-13 16:21:55]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-10-13 16:37:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/196001

[pdr., 2013-10-13 17:38:59]

On 2013/10/13 16:37:03, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/yoav%40yoav.ws/25105004/196001

It looks like the presubmit script is failing lint on TestExpectations.

[Yoav Weiss, 2013-10-13 17:41:41]

Yeah. Fixed the presubmit failures. Now hoping the rebaslines worked. :)


On Sun, Oct 13, 2013 at 7:39 PM, <pdr@chromium.org> wrote:

> On 2013/10/13 16:37:03, I haz the power (commit-bot) wrote:
>
>> CQ is trying da patch. Follow status at
>>
https://chromium-status.**appspot.com/cq/yoav%40yoav.ws/**25105004/196001<htt...
>>
>
> It looks like the presubmit script is failing lint on TestExpectations.
>
>
https://codereview.chromium.**org/25105004/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-10-13 18:18:20]

Retried try job too often on win_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...

[commit-bot: I haz the power, 2013-10-13 18:37:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/213001

[commit-bot: I haz the power, 2013-10-13 19:40:05]

Change committed as 159544

[commit-bot: I haz the power, 2013-10-14 22:48:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoav@yoav.ws/25105004/230001

[commit-bot: I haz the power, 2013-10-15 01:38:46]

Change committed as 159622