Bug 161730 – HTMLImageElement.width / height attributes should be unsigned

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 288238 [details]
Patch

[Alex Christensen]

Comment on attachment 288238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288238&action=review

> Source/WebCore/html/HTMLImageElement.cpp:408
> +        Optional<int> height = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(heightAttr));

auto

> Source/WebCore/html/HTMLImageElement.h:42
> -    static Ref<HTMLImageElement> createForJSConstructor(Document&, const int* optionalWidth, const int* optionalHeight);
> +    static Ref<HTMLImageElement> createForJSConstructor(Document&, Optional<unsigned> width, Optional<unsigned> optionalHeight);

optionalHeight could just be height.

[Build Bot]

Comment on attachment 288238 [details]
Patch

Attachment 288238 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2031121

New failing tests:
imported/w3c/web-platform-tests/html/dom/reflection-embedded.html

[Build Bot]

Created attachment 288246 [details]
Archive of layout-test-results from ews126 for ios-simulator-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-elcapitan-wk2  Platform: Mac OS X 10.11.5

[Chris Dumez]

Created attachment 288272 [details]
Patch

[Chris Dumez]

Comment on attachment 288238 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288238&action=review

>> Source/WebCore/html/HTMLImageElement.cpp:408
>> +        Optional<int> height = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(heightAttr));
> 
> auto

I actually do not like using auto for Optional<> type because I find the resulting code a bit confusing:
auto height = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(heightAttr));
if (height)
  // ...

It makes it look like you're checking if height is != 0, which is not the case. One way to work around this would be to keep 'optional' in the variable name:
auto optionalHeight = parseHTMLNonNegativeInteger(attributeWithoutSynchronization(heightAttr));
if (optionalHeight)
  // ...

But I don't think it is better than using an explicit Optional<> type and a shorter 'height' variable name.

>> Source/WebCore/html/HTMLImageElement.h:42
>> +    static Ref<HTMLImageElement> createForJSConstructor(Document&, Optional<unsigned> width, Optional<unsigned> optionalHeight);
> 
> optionalHeight could just be height.

Oops, fixed.

[Chris Dumez]

Comment on attachment 288272 [details]
Patch

Clearing flags on attachment: 288272

Committed r205655: <http://trac.webkit.org/changeset/205655>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.