[webkit-reviews] review denied: [Bug 70304] width/height attributes of input element should be supported when the type of the input element is image. : [Attachment 139966] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri May 4 19:35:30 PDT 2012
Kent Tamura <tkent at chromium.org> has denied Dongwoo Joshua Im
<dw.im at samsung.com>'s request for review:
Bug 70304: width/height attributes of input element should be supported when
the type of the input element is image.
https://bugs.webkit.org/show_bug.cgi?id=70304
Attachment 139966: Patch
https://bugs.webkit.org/attachment.cgi?id=139966&action=review
------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139966&action=review
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age-expected.txt:5
> +TEST COMPLETE
> +PASS ('width' in e) is true
'TEST COMPLETE,' then 'PASS' looks confusing. Please follow my comments below.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:9
> +if (window.layoutTestController) {
> + layoutTestController.waitUntilDone();
> + layoutTestController.dumpAsText();
> +}
Replace these lines with:
jsTestIsAsync = true;
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:16
> +function log(text)
> +{
> + var console = document.getElementById('console');
> + console.appendChild(document.createTextNode(text));
> + console.appendChild(document.createElement('br'));
> +}
Remove this function. js-test-pre.js provides debug() function.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:20
> +<input type="image" id="imageSlow" src="resources/image-slow.pl">
This element has a renderer. You need to specify style="display:none;"
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:28
> + shouldBeTrue("('width' in e)");
> + shouldBeTrue("('height' in e)");
They are meaningless because HTMLInputElement always has width/height
properties.
You should verify what values should width/height be.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:29
> + log("Success")
This message is meaningless. We can remove it.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:30
> + layoutTestController.notifyDone()
Replace this with:
finishJSTest();
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-loaded-im
age.html:32
> + } else if (readyState == 'loading' || readyState == 'interactive') {
> + setTimeout("runTest()", 50);
Polling readyState is not efficient. You should use events such as 'load'
'readystatechange'.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loade
d-image.html:20
> +<input type="image" id="imageSlow" src="resources/image-slow.pl">
This has a renderer.
>
LayoutTests/fast/forms/input-width-height-attributes-without-renderer-not-loade
d-image.html:37
> + function runTest() {
> + readyState = document.readyState;
> + if (readyState == 'complete') {
> + log("Success")
> + layoutTestController.notifyDone()
> + } else if (readyState == 'loading' || readyState == 'interactive') {
> + shouldBeTrue("('width' in e)");
> + shouldBeTrue("('height' in e)");
> +
> + setTimeout("runTest()", 50);
> + }
> + }
> +
> + runTest();
I don't think this test needs to be asynchronous.
var e = document.getELementById("ImageSlow");
shouldBe(...)
is enough.
> LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:15
> +<input id="inputElement" type="image" width="50" height="50">
This element has a renderer.
> LayoutTests/fast/forms/input-width-height-attributes-without-renderer.html:16
> +No crash = test PASS
Why?
We should verify what width/height should be.
More information about the webkit-reviews
mailing list