1965106 - GetIntrinsicSizeInAppUnits (formerly GetIntrinsicSize) impls on DynamicImage and ClippedImage fail to scale their results to reflect that they're in app units rather than CSS pixels

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Daniel Holbert [:dholbert]]

[Noticed via code inspection while working on patches related to bug 1935269]

Take a look at DynamicImage::GetIntrinsicSize, as compared to GetWidth (and GetHeight):

https://searchfox.org/mozilla-central/rev/4c065f1df299065c305fb48b36cdae571a43d97c/image/DynamicImage.cpp#79-82,100-103

NS_IMETHODIMP
DynamicImage::GetWidth(int32_t* aWidth) {
  *aWidth = mDrawable->Size().width;
  return NS_OK;
...
NS_IMETHODIMP
DynamicImage::GetIntrinsicSize(nsSize* aSize) {
  IntSize intSize(mDrawable->Size());
  *aSize = nsSize(intSize.width, intSize.height);

GetWidth and GetHeight return sizes in units of CSS pixels whereas GetIntrinsicSize returns nscoord units, but here we're not doing any sort of scaling-conversion to differentiate between those -- we're just returning the same numeric values.

Here's RasterImage for comparsion, which uses CSSPixelsToAppUnits to get into the space of nscoord units:

https://searchfox.org/mozilla-central/rev/4c065f1df299065c305fb48b36cdae571a43d97c/image/RasterImage.cpp#183-184,192,247-248,253-254

NS_IMETHODIMP
RasterImage::GetWidth(int32_t* aWidth) {
...
  *aWidth = mSize.width;
...
NS_IMETHODIMP
RasterImage::GetIntrinsicSize(nsSize* aSize) {
...
  *aSize = nsSize(nsPresContext::CSSPixelsToAppUnits(mSize.width),
                  nsPresContext::CSSPixelsToAppUnits(mSize.height));

Presumably DynamicImage should be calling nsPresContext::CSSPixelsToAppUnits() as well.

(I don't have a testcase that reveals anything broken due to this -- I don't know offhand where we use GetIntrinsicSize and I think maybe DynamicImage is only for -moz-element which IIRC isn't web-exposed.)

Comment 1

[Daniel Holbert [:dholbert]]

tnikkel noticed that ClippedImage seems to have the same problem - it directly uses a size (from mClip) in its GetIntrinsicSize impl that's at the same scale as in GetWidth and GetHeight, without scaling it, despite the fact that the return values are meant to be at different scales (nscoord vs. pixels).

Searchfox link:
https://searchfox.org/mozilla-central/rev/ced8457641a062908760d480275f76ccc4d496d1/image/ClippedImage.cpp#177,179,184,188-189,194,198-199,204

...
ClippedImage::GetWidth(int32_t* aWidth) {
...
  *aWidth = mClip.Width();
...
NS_IMETHODIMP
ClippedImage::GetHeight(int32_t* aHeight) {
...
  *aHeight = mClip.Height();
...
NS_IMETHODIMP
ClippedImage::GetIntrinsicSize(nsSize* aSize) {
...
  *aSize = nsSize(mClip.Width(), mClip.Height());

Comment 2

[Daniel Holbert [:dholbert]]

Also FWIW, the GetIntrinsicSize impl in question here is being renamed to GetIntrinsicSizeInAppUnits in Bug 1965282 (and I'm adding some comments to point back to this bug as part of the patch there); and I'm adding a new GetIntrinsicSize function in bug 1965114 that returns its value in css pixels and hence doesn't have this problem.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: Bug 1965106. Fix ClippedImage and DynamicImage GetIntrinsicSizeInAppUnits to actually be in app units. r?dholbert

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7f9a8bde9060
Fix ClippedImage and DynamicImage GetIntrinsicSizeInAppUnits to actually be in app units. r=dholbert

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8afc5b7ad56d
Revert "Bug 1965106. Fix ClippedImage and DynamicImage GetIntrinsicSizeInAppUnits to actually be in app units.  r=dholbert" for causing build bustages @ ClippedImage.cpp

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing build bustages @ ClippedImage.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/8afc5b7ad56d4f6789703a9cbc5826a4da9f6aa6

Push with failures

Failure log

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ead063bea4f
Fix ClippedImage and DynamicImage GetIntrinsicSizeInAppUnits to actually be in app units. r=dholbert

Comment 8

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/5ead063bea4f