Summary: | HTMLImageElement.width / height attributes should be unsigned | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
Component: | DOM | Assignee: | Chris Dumez <cdumez> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | achristensen, cdumez, commit-queue, darin, esprehn+autocc, gyuyoung.kim, kondapallykalyan, rniwa, sam | ||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | https://html.spec.whatwg.org/#htmlimageelement | ||||||||||
Attachments: |
|
Description
Chris Dumez
2016-09-07 19:57:07 PDT
Created attachment 288238 [details]
Patch
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. 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 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
Created attachment 288272 [details]
Patch
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. Comment on attachment 288272 [details] Patch Clearing flags on attachment: 288272 Committed r205655: <http://trac.webkit.org/changeset/205655> All reviewed patches have been landed. Closing bug. |