Closed Bug 648910 Opened 10 years ago Closed 10 years ago

Make nsIDOMHTMLImageElement widths and heights unsigned to match the HTML5 spec

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mats, Assigned: mats)

References

()

Details

(Keywords: html5)

Attachments

(1 file, 2 obsolete files)

Attached patch fixSplinter Review
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
Attachment #524998 - Flags: review?(jst)
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.
Version: unspecified → Trunk
(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.
Attachment #524998 - Flags: review?(jst) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/748c0e1c3712
Whiteboard: [needs review] → [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/748c0e1c3712
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Mats, I think you could easily tests this by using content/html/content/tests/reflect.js
Depends on: 670922
Attached patch Test using reflect.js (obsolete) — Splinter Review
This test depends on the added support for readOnly in bug 670922.
Attachment #545380 - Flags: review?(mounir)
Attached patch Test using reflect.js (obsolete) — Splinter Review
... this is the right patch file.
Attachment #545380 - Attachment is obsolete: true
Attachment #545380 - Flags: review?(mounir)
Attachment #545381 - Flags: review?(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.
Attachment #545381 - Flags: feedback?(Ms2ger)
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.
Attachment #545381 - Flags: feedback?(Ms2ger) → feedback-
Attachment #545381 - Attachment is obsolete: true
Attachment #545381 - Flags: review?(mounir)
You need to log in before you can comment on or make changes to this bug.