648910 - Make nsIDOMHTMLImageElement widths and heights unsigned to match the HTML5 spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mats Palmgren (inactive)]

Attachment: fix

nsIDOMHTMLImageElement.idl declares width, height, naturalWidth and
naturalHeight as long.
http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/html/nsIDOMHTMLImageElement.idl

The HTML spec declares them as unsigned long.
http://www.whatwg.org/specs/web-apps/current-work/#the-img-element

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 524998 [details] [diff] [review]
fix

>diff --git a/content/html/content/src/nsHTMLImageElement.cpp b/content/html/content/src/nsHTMLImageElement.cpp
> NS_IMETHODIMP
>-nsHTMLImageElement::GetNaturalHeight(PRInt32* aNaturalHeight)
>+nsHTMLImageElement::GetNaturalHeight(PRUint32* aNaturalHeight)
> {
>   NS_ENSURE_ARG_POINTER(aNaturalHeight);
> 
>   *aNaturalHeight = 0;
> 
>   if (!mCurrentRequest) {
>     return NS_OK;
>   }
>   
>   nsCOMPtr<imgIContainer> image;
>   mCurrentRequest->GetImage(getter_AddRefs(image));
>   if (!image) {
>     return NS_OK;
>   }
> 
>-  image->GetHeight(aNaturalHeight);
>+  PRInt32 height;
>+  if (NS_SUCCEEDED(image->GetHeight(&height))) {
>+    *aNaturalHeight = height;
>+  }
>   return NS_OK;
> }

Why do you add NS_SUCCEDED? Could you just cast aNaturalHeight when passed to GetHeight?

> NS_IMETHODIMP
>-nsHTMLImageElement::GetNaturalWidth(PRInt32* aNaturalWidth)
>+nsHTMLImageElement::GetNaturalWidth(PRUint32* aNaturalWidth)
> {
>   NS_ENSURE_ARG_POINTER(aNaturalWidth);
> 
>   *aNaturalWidth = 0;
> 
>   if (!mCurrentRequest) {
>     return NS_OK;
>   }
>   
>   nsCOMPtr<imgIContainer> image;
>   mCurrentRequest->GetImage(getter_AddRefs(image));
>   if (!image) {
>     return NS_OK;
>   }
> 
>-  image->GetWidth(aNaturalWidth);
>+  PRInt32 width;
>+  if (NS_SUCCEEDED(image->GetWidth(&width))) {
>+    *aNaturalWidth = width;
>+  }
>   return NS_OK;
> }

Same here.

Comment 2

[Mats Palmgren (inactive)]

(In reply to comment #1)
> Why do you add NS_SUCCEDED?
> Could you just cast aNaturalHeight when passed to GetHeight?

If the call fails then the out param has an undefined value and this
method must return zero when the real value is unavailable, per HTML5.

Comment 3

[Mats Palmgren (inactive)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/748c0e1c3712

Comment 4

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/748c0e1c3712

Comment 5

[Mounir Lamouri (:mounir)]

Mats, I think you could easily tests this by using content/html/content/tests/reflect.js

Comment 6

[Mats Palmgren (inactive)]

Attachment: Test using reflect.js

This test depends on the added support for readOnly in bug 670922.

Comment 7

[Mats Palmgren (inactive)]

Attachment: Test using reflect.js

... this is the right patch file.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 545381 [details] [diff] [review]
Test using reflect.js

Review of attachment 545381 [details] [diff] [review]:
-----------------------------------------------------------------

With the fixes in bug 670922, this patch would look good. Though, I'm realizing these attributes are not a full reflection of the content attributes. I would like Ms2ger's opinion on that.

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 545381 [details] [diff] [review]
Test using reflect.js

I don't think it makes much sense to try to extend reflectUnsignedInt to "support" natural*. I also don't see how the width/height tests are correct, unless our implementation is wrong.