Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2227423002:
Use testharness.js instead of js-test.js in fast/images. (Closed)
DescriptionUse testharness.js instead of js-test.js in fast/images.
This patch uses testharness.js instead of js-test.js.
Other changes involved are
1. Move image-error-event-not-firing.html to http/tests/images/.
Make sure the destination url used in the test exists.
2.Add test images to http/tests/resources folder.
3.Change DOCTYPE declarations.
BUG=637859
.
Committed: https://crrev.com/7d3156f22a42e0da256c80134684ebfb7d99225c
Cr-Commit-Position: refs/heads/master@{#412230}
Patch Set 1 #
Total comments: 12
Patch Set 2 : comments taken care #
Total comments: 10
Patch Set 3 : removed unused images #Patch Set 4 : Changed as per comments #
Total comments: 6
Patch Set 5 : Tests changes as per the comments. #Patch Set 6 : Give correct location of resources folder. #
Total comments: 8
Patch Set 7 : Changed tests as per the comments. #
Total comments: 6
Patch Set 8 : Changed tests as per the comments. #Patch Set 9 : fix #Messages
Total messages: 48 (25 generated)
Description was changed from ========== Use testharness.js in fast/images/ Remove js-test.js and start using testharness.js. Code refactoring using testharness asserts. BUG=None. ========== to ========== Use testharness.js in fast/images/ Remove js-test.js and start using testharness.js. Code refactoring using testharness asserts. BUG=None. ==========
siva.gunturi@samsung.com changed reviewers: + srirama.m@samsung.com
https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:2: <title>"This test ensures that when an image changes, but doesn't change size (like in an Animated GIF), we don't force a Layout."</title> nit: remove the quotes. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:6: async_test(function(t) { Please check if you can use synchronous test instead of async_test by keeping the original image tag before this script element and forcing a layout using "document.documentElement.offsetTop;". Just confirm if the test is failing or not by forcefully changing the image size. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:12: assert_equals(window.internals.needsLayoutCount(), 0); nit: you can omit window prefix. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:2: <html> html, head and body are optional elements, so you can remove them https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... and move title above meta, though it doesn't change behavior it looks good. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:11: img.onerror = t.step_func_done(function() { you can directly use t.step_func_done(), no need to explicitly assert_true. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:14: img.onload = t.step_func_done(function() { this isn't right, as the test will finish here and it won't wait for error event. You can keep the image tag as in the original test and try setting this "src" directly without waiting for onload.
https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:2: <title>"This test ensures that when an image changes, but doesn't change size (like in an Animated GIF), we don't force a Layout."</title> On 2016/08/10 06:59:47, Srirama wrote: > nit: remove the quotes. Done. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:6: async_test(function(t) { On 2016/08/10 06:59:47, Srirama wrote: > Please check if you can use synchronous test instead of async_test by keeping > the original image tag before this script element and forcing a layout using > "document.documentElement.offsetTop;". Just confirm if the test is failing or > not by forcefully changing the image size. Done. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:12: assert_equals(window.internals.needsLayoutCount(), 0); On 2016/08/10 06:59:47, Srirama wrote: > nit: you can omit window prefix. Done. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:2: <html> On 2016/08/10 06:59:47, Srirama wrote: > html, head and body are optional elements, so you can remove them > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... > and move title above meta, though it doesn't change behavior it looks good. Done. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:11: img.onerror = t.step_func_done(function() { On 2016/08/10 06:59:47, Srirama wrote: > you can directly use t.step_func_done(), no need to explicitly assert_true. Done. https://codereview.chromium.org/2227423002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:14: img.onload = t.step_func_done(function() { On 2016/08/10 06:59:47, Srirama wrote: > this isn't right, as the test will finish here and it won't wait for error > event. You can keep the image tag as in the original test and try setting this > "src" directly without waiting for onload. Done.
Description was changed from ========== Use testharness.js in fast/images/ Remove js-test.js and start using testharness.js. Code refactoring using testharness asserts. BUG=None. ========== to ========== Use testharness.js in fast/images/ - [1] Remove js-test.js and start using testharness.js. Code refactoring using testharness asserts. BUG=None. ==========
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
siva.gunturi@samsung.com changed reviewers: - srirama.m@samsung.com
siva.gunturi@samsung.com changed reviewers: + srirama.m@samsung.com
LGTM with the comments addressed and after getting LGTM from another reviewer. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:12: if(img) { condition not required. you can remove it. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:15: return; you can omit the return and the assert stmt below. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:6: <img src="./color-jpeg-with-color-profile-expected.png"></img> Remove all these images from platform folders as discussed once this change lands. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:9: var img = document.querySelector("img"); remove the extra indentation. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:11: img.src = 'http://www.myfakesiteabc.com/image.png'; nit: use double quotes here or use single quotes in querySelector to be consistent within the whole script.
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:12: if(img) { On 2016/08/10 10:50:21, Srirama wrote: > condition not required. you can remove it. Done. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:15: return; On 2016/08/10 10:50:21, Srirama wrote: > you can omit the return and the assert stmt below. Done. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:6: <img src="./color-jpeg-with-color-profile-expected.png"></img> On 2016/08/10 10:50:21, Srirama wrote: > Remove all these images from platform folders as discussed once this change > lands. Done. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:9: var img = document.querySelector("img"); On 2016/08/10 10:50:21, Srirama wrote: > remove the extra indentation. Done. https://codereview.chromium.org/2227423002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:11: img.src = 'http://www.myfakesiteabc.com/image.png'; On 2016/08/10 10:50:21, Srirama wrote: > nit: use double quotes here or use single quotes in querySelector to be > consistent within the whole script. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
siva.gunturi@samsung.com changed reviewers: + fs@opera.com
@fs, ptal..
This looks mostly ok. Some comments on the CSP-related test though - not really your fault as such, but probably something that needs to be addressed. Also, maybe file a bug for this work? It could also be good to try to be a bit more specific in the description (than just "[1]") https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (left): https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:20: image.src = 'http://www.myfakesiteabc.com/image.png'; This is not great - previously we were relying on the "CONSOLE ..." output to see if CSP blocked the load - now we can't really tell. It would seem better to put this test under http/ and make sure the destination actually exists. https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:6: <img src="./color-jpeg-with-color-profile-expected.png"></img> It was arguably a bit fishy to use <random-image-in-the-same-directory-as-self>, but if it worked before... I don't think you should need to add the png file though? (That seems to cause the bots to fail too.) Preferably this should just use a more specific image than the expectation of a different test.
https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> Could you make this just "<!DOCTYPE html>" while we're at it? (Don't see anything here that require quirks mode.)
Description was changed from ========== Use testharness.js in fast/images/ - [1] Remove js-test.js and start using testharness.js. Code refactoring using testharness asserts. BUG=None. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Related changes: 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Related changes: 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
ptal.. https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (left): https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:20: image.src = 'http://www.myfakesiteabc.com/image.png'; On 2016/08/10 17:13:17, fs wrote: > This is not great - previously we were relying on the "CONSOLE ..." output to > see if CSP blocked the load - now we can't really tell. It would seem better to > put this test under http/ and make sure the destination actually exists. Done. https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2016/08/10 17:37:08, fs wrote: > Could you make this just "<!DOCTYPE html>" while we're at it? (Don't see > anything here that require quirks mode.) Done. https://codereview.chromium.org/2227423002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:6: <img src="./color-jpeg-with-color-profile-expected.png"></img> On 2016/08/10 17:13:17, fs wrote: > It was arguably a bit fishy to use <random-image-in-the-same-directory-as-self>, > but if it worked before... I don't think you should need to add the png file > though? (That seems to cause the bots to fail too.) Preferably this should just > use a more specific image than the expectation of a different test. Done. I created a 1*1 png test image to test this.
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Other changes involved are 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:13: assert_equals(internals.needsLayoutCount(), 0); I think we could make this test a bit more "future-safe" by also waiting for img.onload and then asserting the same there. So: test -> async_test (passing t to the test function) and img.onload = t.step_func_done(function() { assert_equals(internals.needsLayoutCount(), 0); }); Probably should wait for window.onload too as well before setting img.src then. Hopefully that isn't too strict. (Feel free to leave this to a follow-up.) https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html:10: assert_equals(img.naturalWidth, 0); Maybe add an additional assert of img.naturalWidth (as non-zero) before the removeAttribute call. https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:11: img.src = "http://127.0.0.1:8000/resources/dummy.html"; Please make this reference a valid image as well (could be the same image/filename as above I think.)
@fs, ptal. https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/image-change-without-resize-shouldnt-layout.html:13: assert_equals(internals.needsLayoutCount(), 0); On 2016/08/16 09:39:37, fs wrote: > I think we could make this test a bit more "future-safe" by also waiting for > img.onload and then asserting the same there. So: > > test -> async_test (passing t to the test function) > > and > > img.onload = t.step_func_done(function() { > assert_equals(internals.needsLayoutCount(), 0); > }); > > Probably should wait for window.onload too as well before setting img.src then. > Hopefully that isn't too strict. > > (Feel free to leave this to a follow-up.) Done. Will change this in a follow-up patch. https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html:10: assert_equals(img.naturalWidth, 0); On 2016/08/16 09:39:37, fs wrote: > Maybe add an additional assert of img.naturalWidth (as non-zero) before the > removeAttribute call. Done. https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:11: img.src = "http://127.0.0.1:8000/resources/dummy.html"; On 2016/08/16 09:39:37, fs wrote: > Please make this reference a valid image as well (could be the same > image/filename as above I think.) Done.
https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (left): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> This file is listed as "moved", could you make sure it's properly deleted?
LGTM w/ nit. Please also make sure that the file that was moved no longer exists in its original location. https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html (right): https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html:9: assert_greater_than(img.naturalWidth, 0, "Image width should be greater than zero"); Nit: Could probably even use the exact value from the image (16 I think.) https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:6: <img src="../resources/image-error-event-not-firing-test-image.png"></img> We could arguably make this test more robust as well by verifying that the initial reference loads ok. That's another potential TODO.
https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:6: <img src="../resources/image-error-event-not-firing-test-image.png"></img> Could probably also use one of the the square* files that already exists (to avoid adding very specific files like this one.)
@fs, ptal.. https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html (left): https://codereview.chromium.org/2227423002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2016/08/16 10:36:15, fs wrote: > This file is listed as "moved", could you make sure it's properly deleted? Done. https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html (right): https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html:9: assert_greater_than(img.naturalWidth, 0, "Image width should be greater than zero"); On 2016/08/16 10:42:42, fs wrote: > Nit: Could probably even use the exact value from the image (16 I think.) Done. https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:6: <img src="../resources/image-error-event-not-firing-test-image.png"></img> On 2016/08/16 10:44:55, fs wrote: > Could probably also use one of the the square* files that already exists (to > avoid adding very specific files like this one.) Done. https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:6: <img src="../resources/image-error-event-not-firing-test-image.png"></img> On 2016/08/16 10:42:42, fs wrote: > We could arguably make this test more robust as well by verifying that the > initial reference loads ok. That's another potential TODO. Done.
On 2016/08/16 10:42:42, fs wrote: > LGTM w/ nit. > > Please also make sure that the file that was moved no longer exists in its > original location. > > https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html > (right): > > https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/images/natural-dimensions-correct-after-image-reset.html:9: > assert_greater_than(img.naturalWidth, 0, "Image width should be greater than > zero"); > Nit: Could probably even use the exact value from the image (16 I think.) > > https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html > (right): > > https://codereview.chromium.org/2227423002/diff/120001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:6: > <img src="../resources/image-error-event-not-firing-test-image.png"></img> > We could arguably make this test more robust as well by verifying that the > initial reference loads ok. That's another potential TODO. Oh the image file is still not deleted. I will take care of that.
https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:12: img.src = "file:///../resources/square.png"; Use http://127.0.0.1:8000/resources/square.png like in PS7. (file: would theoretically work, but a relative file URL doesn't seem great.)
https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:12: img.src = "file:///../resources/square.png"; On 2016/08/16 12:47:52, fs wrote: > Use http://127.0.0.1:8000/resources/square.png like in PS7. (file: would > theoretically work, but a relative file URL doesn't seem great.) http://127.0.0.1:8000/resources/square.png points to the same origin, so onerror will never get generated. Thats why the intial test pointed to a fake url.
https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html (right): https://codereview.chromium.org/2227423002/diff/180001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/images/image-error-event-not-firing.html:12: img.src = "file:///../resources/square.png"; On 2016/08/16 at 12:51:55, sivag wrote: > On 2016/08/16 12:47:52, fs wrote: > > Use http://127.0.0.1:8000/resources/square.png like in PS7. (file: would > > theoretically work, but a relative file URL doesn't seem great.) > > http://127.0.0.1:8000/resources/square.png points to the same origin, so onerror will never get generated. Thats why the intial test pointed to a fake url. If that is the same origin you can change the port to 8080 so that it no longer is.
Patchset #12 (id:220001) has been deleted
Patchset #11 (id:200001) has been deleted
Patchset #10 (id:180001) has been deleted
Patchset #9 (id:160001) has been deleted
I am not able to mark the file third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html as deleted. Am i missing something here.
On 2016/08/16 at 13:12:29, siva.gunturi wrote: > I am not able to mark the file third_party/WebKit/LayoutTests/fast/images/image-error-event-not-firing.html as deleted. Am i missing something here. I looked at the raw file, and there it says it's deleted, so hopefully it's just an issue with the diff-viewer here. Anyway, LGTM.
The CQ bit was checked by siva.gunturi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from srirama.m@samsung.com Link to the patchset: https://codereview.chromium.org/2227423002/#ps240001 (title: "fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Other changes involved are 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Other changes involved are 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ==========
Message was sent while issue was closed.
Committed patchset #9 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Other changes involved are 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. ========== to ========== Use testharness.js instead of js-test.js in fast/images. This patch uses testharness.js instead of js-test.js. Other changes involved are 1. Move image-error-event-not-firing.html to http/tests/images/. Make sure the destination url used in the test exists. 2.Add test images to http/tests/resources folder. 3.Change DOCTYPE declarations. BUG=637859. Committed: https://crrev.com/7d3156f22a42e0da256c80134684ebfb7d99225c Cr-Commit-Position: refs/heads/master@{#412230} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/7d3156f22a42e0da256c80134684ebfb7d99225c Cr-Commit-Position: refs/heads/master@{#412230} |
