Last Comment Bug 648910 - Make nsIDOMHTMLImageElement widths and heights unsigned to match the HTML5 spec
: Make nsIDOMHTMLImageElement widths and heights unsigned to match the HTML5 spec
Status: RESOLVED FIXED
: html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla8
Assigned To: Mats Palmgren (:mats)
:
: Andrew Overholt [:overholt]
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 670922
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-10 15:53 PDT by Mats Palmgren (:mats)
Modified: 2011-07-19 13:35 PDT (History)
2 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (4.50 KB, patch)
2011-04-10 15:53 PDT, Mats Palmgren (:mats)
jst: review+
Details | Diff | Splinter Review
Test using reflect.js (2.56 KB, patch)
2011-07-12 07:33 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
Test using reflect.js (2.56 KB, patch)
2011-07-12 07:37 PDT, Mats Palmgren (:mats)
Ms2ger: feedback-
Details | Diff | Splinter Review

Description Mats Palmgren (:mats) 2011-04-10 15:53:50 PDT
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
Comment 1 Mounir Lamouri (:mounir) 2011-04-10 20:01:35 PDT
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 (:mats) 2011-04-12 14:29:18 PDT
(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 4 Mounir Lamouri (:mounir) 2011-07-12 03:49:33 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/748c0e1c3712
Comment 5 Mounir Lamouri (:mounir) 2011-07-12 03:50:25 PDT
Mats, I think you could easily tests this by using content/html/content/tests/reflect.js
Comment 6 Mats Palmgren (:mats) 2011-07-12 07:33:15 PDT
Created attachment 545380 [details] [diff] [review]
Test using reflect.js

This test depends on the added support for readOnly in bug 670922.
Comment 7 Mats Palmgren (:mats) 2011-07-12 07:37:10 PDT
Created attachment 545381 [details] [diff] [review]
Test using reflect.js

... this is the right patch file.
Comment 8 Mounir Lamouri (:mounir) 2011-07-12 07:51:10 PDT
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 (⌚ UTC+1/+2) 2011-07-13 08:39:38 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.