Make nsIDOMHTMLImageElement widths and heights unsigned to match the HTML5 spec

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
--
minor
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mats, Assigned: mats)

Tracking

({html5})

Trunk
mozilla8
html5
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 524998 [details] [diff] [review]
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
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
(Assignee)

Comment 2

6 years ago
(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.

Updated

6 years ago
Attachment #524998 - Flags: review?(jst) → review+
(Assignee)

Comment 3

6 years ago
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
Last Resolved: 6 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
(Assignee)

Updated

6 years ago
Depends on: 670922
(Assignee)

Comment 6

6 years ago
Created attachment 545380 [details] [diff] [review]
Test using reflect.js

This test depends on the added support for readOnly in bug 670922.
Attachment #545380 - Flags: review?(mounir)
(Assignee)

Comment 7

6 years ago
Created attachment 545381 [details] [diff] [review]
Test using reflect.js

... 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-
(Assignee)

Updated

6 years ago
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.