HTMLImageElement.width / height attributes should be unsigned
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Sep 2016 19:46:10 +0000 (19:46 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Sep 2016 19:46:10 +0000 (19:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=161730

Reviewed by Alex Christensen.

LayoutTests/imported/w3c:

Rebaseline W3C test now that more checks are passing.

* web-platform-tests/html/dom/reflection-embedded-expected.txt:

Source/WebCore:

HTMLImageElement.width / height attributes should be unsigned as per
the HTML specification:
- https://html.spec.whatwg.org/#htmlimageelement

However, they are signed in WebKit. Firefox agrees with the
specification.

No new tests, rebaselined existing test.

* bindings/js/JSImageConstructor.cpp:
(WebCore::JSImageConstructor::construct):
(WebCore::createImageConstructor): Deleted.
* html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::createForJSConstructor):
(WebCore::HTMLImageElement::width):
(WebCore::HTMLImageElement::height):
(WebCore::HTMLImageElement::setHeight):
(WebCore::HTMLImageElement::setWidth):
* html/HTMLImageElement.h:
* html/HTMLImageElement.idl:
* html/ImageDocument.cpp:
(WebCore::ImageDocument::restoreImageSize):

LayoutTests:

Update existing test to reflect behavior change.

* js/dom/custom-constructors-expected.txt:
* js/dom/script-tests/custom-constructors.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205655 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/html/dom/reflection-embedded-expected.txt
LayoutTests/js/dom/custom-constructors-expected.txt
LayoutTests/js/dom/script-tests/custom-constructors.js
LayoutTests/platform/ios-simulator/imported/w3c/web-platform-tests/html/dom/reflection-embedded-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSImageConstructor.cpp
Source/WebCore/html/HTMLImageElement.cpp
Source/WebCore/html/HTMLImageElement.h
Source/WebCore/html/HTMLImageElement.idl
Source/WebCore/html/ImageDocument.cpp

index 4bb2caa..45b4faa 100644 (file)
@@ -1,3 +1,15 @@
+2016-09-08  Chris Dumez  <cdumez@apple.com>
+
+        HTMLImageElement.width / height attributes should be unsigned
+        https://bugs.webkit.org/show_bug.cgi?id=161730
+
+        Reviewed by Alex Christensen.
+
+        Update existing test to reflect behavior change.
+
+        * js/dom/custom-constructors-expected.txt:
+        * js/dom/script-tests/custom-constructors.js:
+
 2016-09-08  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r205652.
index dda6710..03ff3b1 100644 (file)
@@ -1,3 +1,14 @@
+2016-09-08  Chris Dumez  <cdumez@apple.com>
+
+        HTMLImageElement.width / height attributes should be unsigned
+        https://bugs.webkit.org/show_bug.cgi?id=161730
+
+        Reviewed by Alex Christensen.
+
+        Rebaseline W3C test now that more checks are passing.
+
+        * web-platform-tests/html/dom/reflection-embedded-expected.txt:
+
 2016-09-07  Chris Dumez  <cdumez@apple.com>
 
         Fix handling of negative radius in HTMLAreaElement's coords when in circle state
index af96998..07e2194 100644 (file)
@@ -1147,11 +1147,11 @@ PASS img.width: IDL set to "-0" should not throw
 PASS img.width: IDL set to "-0" followed by getAttribute() 
 PASS img.width: IDL set to "-0" followed by IDL get 
 PASS img.width: IDL set to 2147483648 should not throw 
-FAIL img.width: IDL set to 2147483648 followed by getAttribute() assert_equals: expected "0" but got "-2147483648"
-FAIL img.width: IDL set to 2147483648 followed by IDL get assert_equals: expected 0 but got -2147483648
+PASS img.width: IDL set to 2147483648 followed by getAttribute() 
+PASS img.width: IDL set to 2147483648 followed by IDL get 
 PASS img.width: IDL set to 4294967295 should not throw 
-FAIL img.width: IDL set to 4294967295 followed by getAttribute() assert_equals: expected "0" but got "-1"
-FAIL img.width: IDL set to 4294967295 followed by IDL get assert_equals: expected 0 but got -1
+PASS img.width: IDL set to 4294967295 followed by getAttribute() 
+PASS img.width: IDL set to 4294967295 followed by IDL get 
 PASS img.height: typeof IDL attribute 
 PASS img.height: IDL get with DOM attribute unset 
 PASS img.height: IDL set to 0 should not throw 
@@ -1170,11 +1170,11 @@ PASS img.height: IDL set to "-0" should not throw
 PASS img.height: IDL set to "-0" followed by getAttribute() 
 PASS img.height: IDL set to "-0" followed by IDL get 
 PASS img.height: IDL set to 2147483648 should not throw 
-FAIL img.height: IDL set to 2147483648 followed by getAttribute() assert_equals: expected "0" but got "-2147483648"
-FAIL img.height: IDL set to 2147483648 followed by IDL get assert_equals: expected 0 but got -2147483648
+PASS img.height: IDL set to 2147483648 followed by getAttribute() 
+PASS img.height: IDL set to 2147483648 followed by IDL get 
 PASS img.height: IDL set to 4294967295 should not throw 
-FAIL img.height: IDL set to 4294967295 followed by getAttribute() assert_equals: expected "0" but got "-1"
-FAIL img.height: IDL set to 4294967295 followed by IDL get assert_equals: expected 0 but got -1
+PASS img.height: IDL set to 4294967295 followed by getAttribute() 
+PASS img.height: IDL set to 4294967295 followed by IDL get 
 PASS img.name: typeof IDL attribute 
 PASS img.name: IDL get with DOM attribute unset 
 PASS img.name: setAttribute() to "" followed by getAttribute() 
index 3526a75..c1cca26 100644 (file)
@@ -9,8 +9,8 @@ PASS new Image().height is 0
 PASS new Image().width is 0
 PASS new Image(100).width is 100
 PASS new Image(100, 200).height is 200
-PASS new Image(-100).width is -100
-PASS new Image(-100, -200).height is -200
+PASS new Image(-100).width is 0
+PASS new Image(-100, -200).height is 0
 PASS new Image().outerHTML is "<img>"
 PASS new Image(100, 100).outerHTML.replace(/"/g, "'") is "<img width='100' height='100'>"
 PASS new Option() is non-null.
index fce2ff8..773987b 100644 (file)
@@ -10,8 +10,8 @@ shouldBe("new Image().height", "0");
 shouldBe("new Image().width", "0");
 shouldBe("new Image(100).width", "100");
 shouldBe("new Image(100, 200).height", "200");
-shouldBe("new Image(-100).width", "-100");
-shouldBe("new Image(-100, -200).height", "-200");
+shouldBe("new Image(-100).width", "0");
+shouldBe("new Image(-100, -200).height", "0");
 
 shouldBeEqualToString("new Image().outerHTML","<img>");
 // FIXME: shouldBeEqualToString strips quotes from the string.
index 6544da7..681c62a 100644 (file)
@@ -1147,11 +1147,11 @@ PASS img.width: IDL set to "-0" should not throw
 PASS img.width: IDL set to "-0" followed by getAttribute() 
 PASS img.width: IDL set to "-0" followed by IDL get 
 PASS img.width: IDL set to 2147483648 should not throw 
-FAIL img.width: IDL set to 2147483648 followed by getAttribute() assert_equals: expected "0" but got "-2147483648"
-FAIL img.width: IDL set to 2147483648 followed by IDL get assert_equals: expected 0 but got -2147483648
+PASS img.width: IDL set to 2147483648 followed by getAttribute() 
+PASS img.width: IDL set to 2147483648 followed by IDL get 
 PASS img.width: IDL set to 4294967295 should not throw 
-FAIL img.width: IDL set to 4294967295 followed by getAttribute() assert_equals: expected "0" but got "-1"
-FAIL img.width: IDL set to 4294967295 followed by IDL get assert_equals: expected 0 but got -1
+PASS img.width: IDL set to 4294967295 followed by getAttribute() 
+PASS img.width: IDL set to 4294967295 followed by IDL get 
 PASS img.height: typeof IDL attribute 
 PASS img.height: IDL get with DOM attribute unset 
 PASS img.height: IDL set to 0 should not throw 
@@ -1170,11 +1170,11 @@ PASS img.height: IDL set to "-0" should not throw
 PASS img.height: IDL set to "-0" followed by getAttribute() 
 PASS img.height: IDL set to "-0" followed by IDL get 
 PASS img.height: IDL set to 2147483648 should not throw 
-FAIL img.height: IDL set to 2147483648 followed by getAttribute() assert_equals: expected "0" but got "-2147483648"
-FAIL img.height: IDL set to 2147483648 followed by IDL get assert_equals: expected 0 but got -2147483648
+PASS img.height: IDL set to 2147483648 followed by getAttribute() 
+PASS img.height: IDL set to 2147483648 followed by IDL get 
 PASS img.height: IDL set to 4294967295 should not throw 
-FAIL img.height: IDL set to 4294967295 followed by getAttribute() assert_equals: expected "0" but got "-1"
-FAIL img.height: IDL set to 4294967295 followed by IDL get assert_equals: expected 0 but got -1
+PASS img.height: IDL set to 4294967295 followed by getAttribute() 
+PASS img.height: IDL set to 4294967295 followed by IDL get 
 PASS img.name: typeof IDL attribute 
 PASS img.name: IDL get with DOM attribute unset 
 PASS img.name: setAttribute() to "" followed by getAttribute() 
index 33e8b9e..e66f286 100644 (file)
@@ -1,3 +1,33 @@
+2016-09-08  Chris Dumez  <cdumez@apple.com>
+
+        HTMLImageElement.width / height attributes should be unsigned
+        https://bugs.webkit.org/show_bug.cgi?id=161730
+
+        Reviewed by Alex Christensen.
+
+        HTMLImageElement.width / height attributes should be unsigned as per
+        the HTML specification:
+        - https://html.spec.whatwg.org/#htmlimageelement
+
+        However, they are signed in WebKit. Firefox agrees with the
+        specification.
+
+        No new tests, rebaselined existing test.
+
+        * bindings/js/JSImageConstructor.cpp:
+        (WebCore::JSImageConstructor::construct):
+        (WebCore::createImageConstructor): Deleted.
+        * html/HTMLImageElement.cpp:
+        (WebCore::HTMLImageElement::createForJSConstructor):
+        (WebCore::HTMLImageElement::width):
+        (WebCore::HTMLImageElement::height):
+        (WebCore::HTMLImageElement::setHeight):
+        (WebCore::HTMLImageElement::setWidth):
+        * html/HTMLImageElement.h:
+        * html/HTMLImageElement.idl:
+        * html/ImageDocument.cpp:
+        (WebCore::ImageDocument::restoreImageSize):
+
 2016-09-08  Filip Pizlo  <fpizlo@apple.com>
 
         Move JSMap/JSSet over to Auxiliary MarkedSpace
index 750eef5..4082b1f 100644 (file)
@@ -58,21 +58,16 @@ template<> EncodedJSValue JSImageConstructor::construct(ExecState* state)
     // added to the window object. This is done to ensure that JSDocument::visit
     // will be called, which will cause the image element to be marked if necessary.
     toJS(state, jsConstructor->globalObject(), *document);
-    int width;
-    int height;
-    int* optionalWidth = 0;
-    int* optionalHeight = 0;
+    Optional<unsigned> width;
+    Optional<unsigned> height;
     if (state->argumentCount() > 0) {
-        width = state->argument(0).toInt32(state);
-        optionalWidth = &width;
-    }
-    if (state->argumentCount() > 1) {
-        height = state->argument(1).toInt32(state);
-        optionalHeight = &height;
+        width = state->uncheckedArgument(0).toUInt32(state);
+        if (state->argumentCount() > 1)
+            height = state->uncheckedArgument(1).toUInt32(state);
     }
 
     return JSValue::encode(asObject(toJS(state, jsConstructor->globalObject(),
-        HTMLImageElement::createForJSConstructor(*document, optionalWidth, optionalHeight))));
+        HTMLImageElement::createForJSConstructor(*document, width, height))));
 }
 
 template<> const ClassInfo JSImageConstructor::s_info = { "ImageConstructor", &Base::s_info, 0, CREATE_METHOD_TABLE(JSImageConstructor) };
index 86b3338..0ed3887 100644 (file)
@@ -89,13 +89,13 @@ HTMLImageElement::~HTMLImageElement()
     setPictureElement(nullptr);
 }
 
-Ref<HTMLImageElement> HTMLImageElement::createForJSConstructor(Document& document, const int* optionalWidth, const int* optionalHeight)
+Ref<HTMLImageElement> HTMLImageElement::createForJSConstructor(Document& document, Optional<unsigned> width, Optional<unsigned> height)
 {
-    Ref<HTMLImageElement> image = adoptRef(*new HTMLImageElement(imgTag, document));
-    if (optionalWidth)
-        image->setWidth(*optionalWidth);
-    if (optionalHeight)
-        image->setHeight(*optionalHeight);
+    auto image = adoptRef(*new HTMLImageElement(imgTag, document));
+    if (width)
+        image->setWidth(width.value());
+    if (height)
+        image->setHeight(height.value());
     return image;
 }
 
@@ -376,18 +376,17 @@ void HTMLImageElement::setPictureElement(HTMLPictureElement* pictureElement)
     gPictureOwnerMap->add(this, pictureElement->createWeakPtr());
 }
     
-int HTMLImageElement::width(bool ignorePendingStylesheets)
+unsigned HTMLImageElement::width(bool ignorePendingStylesheets)
 {
     if (!renderer()) {
         // check the attribute first for an explicit pixel value
-        bool ok;
-        int width = attributeWithoutSynchronization(widthAttr).toInt(&ok);
-        if (ok)
-            return width;
+        Optional<int> width = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(widthAttr));
+        if (width)
+            return width.value();
 
         // if the image is available, use its width
         if (m_imageLoader.image())
-            return m_imageLoader.image()->imageSizeForRenderer(renderer(), 1.0f).width();
+            return m_imageLoader.image()->imageSizeForRenderer(renderer(), 1.0f).width().toUnsigned();
     }
 
     if (ignorePendingStylesheets)
@@ -402,18 +401,17 @@ int HTMLImageElement::width(bool ignorePendingStylesheets)
     return adjustForAbsoluteZoom(snappedIntRect(contentRect).width(), *box);
 }
 
-int HTMLImageElement::height(bool ignorePendingStylesheets)
+unsigned HTMLImageElement::height(bool ignorePendingStylesheets)
 {
     if (!renderer()) {
         // check the attribute first for an explicit pixel value
-        bool ok;
-        int height = attributeWithoutSynchronization(heightAttr).toInt(&ok);
-        if (ok)
-            return height;
+        Optional<int> height = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(heightAttr));
+        if (height)
+            return height.value();
 
         // if the image is available, use its height
         if (m_imageLoader.image())
-            return m_imageLoader.image()->imageSizeForRenderer(renderer(), 1.0f).height();
+            return m_imageLoader.image()->imageSizeForRenderer(renderer(), 1.0f).height().toUnsigned();
     }
 
     if (ignorePendingStylesheets)
@@ -501,9 +499,9 @@ bool HTMLImageElement::draggable() const
     return !equalLettersIgnoringASCIICase(attributeWithoutSynchronization(draggableAttr), "false");
 }
 
-void HTMLImageElement::setHeight(int value)
+void HTMLImageElement::setHeight(unsigned value)
 {
-    setIntegralAttribute(heightAttr, value);
+    setUnsignedIntegralAttribute(heightAttr, value);
 }
 
 URL HTMLImageElement::src() const
@@ -516,9 +514,9 @@ void HTMLImageElement::setSrc(const String& value)
     setAttributeWithoutSynchronization(srcAttr, value);
 }
 
-void HTMLImageElement::setWidth(int value)
+void HTMLImageElement::setWidth(unsigned value)
 {
-    setIntegralAttribute(widthAttr, value);
+    setUnsignedIntegralAttribute(widthAttr, value);
 }
 
 int HTMLImageElement::x() const
index 9fbd4b0..c6355a4 100644 (file)
@@ -39,12 +39,12 @@ class HTMLImageElement : public HTMLElement, public FormNamedItem {
 public:
     static Ref<HTMLImageElement> create(Document&);
     static Ref<HTMLImageElement> create(const QualifiedName&, Document&, HTMLFormElement*);
-    static Ref<HTMLImageElement> createForJSConstructor(Document&, const int* optionalWidth, const int* optionalHeight);
+    static Ref<HTMLImageElement> createForJSConstructor(Document&, Optional<unsigned> width, Optional<unsigned> height);
 
     virtual ~HTMLImageElement();
 
-    WEBCORE_EXPORT int width(bool ignorePendingStylesheets = false);
-    WEBCORE_EXPORT int height(bool ignorePendingStylesheets = false);
+    WEBCORE_EXPORT unsigned width(bool ignorePendingStylesheets = false);
+    WEBCORE_EXPORT unsigned height(bool ignorePendingStylesheets = false);
 
     WEBCORE_EXPORT int naturalWidth() const;
     WEBCORE_EXPORT int naturalHeight() const;
@@ -64,7 +64,7 @@ public:
 
     WEBCORE_EXPORT const AtomicString& alt() const;
 
-    WEBCORE_EXPORT void setHeight(int);
+    WEBCORE_EXPORT void setHeight(unsigned);
 
     URL src() const;
     void setSrc(const String&);
@@ -72,7 +72,7 @@ public:
     WEBCORE_EXPORT void setCrossOrigin(const AtomicString&);
     WEBCORE_EXPORT String crossOrigin() const;
 
-    WEBCORE_EXPORT void setWidth(int);
+    WEBCORE_EXPORT void setWidth(unsigned);
 
     WEBCORE_EXPORT int x() const;
     WEBCORE_EXPORT int y() const;
index 265fa41..1c43067 100644 (file)
@@ -27,7 +27,7 @@
     [Reflect] attribute DOMString alt;
     [Reflect, TreatNullAs=EmptyString] attribute DOMString border;
     attribute DOMString? crossOrigin;
-    attribute long height;
+    attribute unsigned long height;
     [Reflect] attribute long hspace;
     [Reflect] attribute boolean isMap;
     [Reflect, URL] attribute USVString longDesc;
@@ -37,7 +37,7 @@
     readonly attribute USVString currentSrc;
     [Reflect] attribute DOMString useMap;
     [Reflect] attribute long vspace;
-    attribute long width;
+    attribute unsigned long width;
 
     // Extensions
     readonly attribute boolean complete;
index 7fd2c7c..d698040 100644 (file)
@@ -313,8 +313,8 @@ void ImageDocument::restoreImageSize()
         return;
 
     LayoutSize imageSize = this->imageSize();
-    m_imageElement->setWidth(imageSize.width());
-    m_imageElement->setHeight(imageSize.height());
+    m_imageElement->setWidth(imageSize.width().toUnsigned());
+    m_imageElement->setHeight(imageSize.height().toUnsigned());
 
     if (imageFitsInWindow())
         m_imageElement->removeInlineStyleProperty(CSSPropertyCursor);