Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257043002:
Remove video-test.js dependency from video-reflection.html (Closed)
DescriptionRemove video-test.js dependency from video-reflection.html
BUG=588956
Committed: https://crrev.com/91eead9f48de00e0744d62344d4310aca1b9178f
Cr-Commit-Position: refs/heads/master@{#412810}
Patch Set 1 #
Total comments: 4
Patch Set 2 : modify as per suggestion #
Total comments: 8
Patch Set 3 : address comments #
Messages
Total messages: 16 (7 generated)
Description was changed from ========== fix BUG= ========== to ========== Convert compositing/video tests to testharness.js Cleaning compositing/video tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
srirama.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL
https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/video/video-reflection.html (right): https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:13: async_test(function(t) { Not sure if testharness makes much sense here - this test appears to be essentially visual. testExpected() is the only thing from video-test.js right? Could just do away with that. (Other simplifications look good though, so we could keep those.) https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:23: setTimeout(t.step_func(function() { testRunner.notifyDone(); }), 150); This timeout looks very gratuitous - could we perhaps use runAfterLayoutAndPaint (from resources/run-after-layout-and-paint.js) instead?
Description was changed from ========== Convert compositing/video tests to testharness.js Cleaning compositing/video tests to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Remove video-test.js dependency from video-reflection.html BUG=588956 ==========
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/compositing/video/video-reflection.html (right): https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:13: async_test(function(t) { On 2016/08/18 08:50:45, fs wrote: > Not sure if testharness makes much sense here - this test appears to be > essentially visual. testExpected() is the only thing from video-test.js right? > Could just do away with that. (Other simplifications look good though, so we > could keep those.) Acknowledged. https://codereview.chromium.org/2257043002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:23: setTimeout(t.step_func(function() { testRunner.notifyDone(); }), 150); On 2016/08/18 08:50:45, fs wrote: > This timeout looks very gratuitous - could we perhaps use runAfterLayoutAndPaint > (from resources/run-after-layout-and-paint.js) instead? Done.
https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/video/video-reflection.html (right): https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:16: testRunner.waitUntilDone(); I'd suggest to keep these "grouped" like before (so that we separate the "setting up testRunner" bits from the actual test as much as possible.) https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:17: video.addEventListener('seeked', function seeked() { Any reason in particular to use addEventListener and not the EventListener attribute? Is the capturing required? https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:19: if (video.currentTime == 1) IIUC, previously only the first 'seeked' was actually "useful" (the testedOnce flag), so does this condition serve any purpose? Could we just call runAfterLayoutAndPaint directly? (If we don't it looks like the test will time out - getting a rendering error seems like a better indication of error.) https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:20: runAfterLayoutAndPaint(function() { testRunner.notifyDone(); }); If you pass true as the second argument, the function will do notifyDone after invoking the callback, so the callback can just be empty if so.
https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/compositing/video/video-reflection.html (right): https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:16: testRunner.waitUntilDone(); On 2016/08/18 10:39:06, fs wrote: > I'd suggest to keep these "grouped" like before (so that we separate the > "setting up testRunner" bits from the actual test as much as possible.) Done. https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:17: video.addEventListener('seeked', function seeked() { On 2016/08/18 10:39:06, fs wrote: > Any reason in particular to use addEventListener and not the EventListener > attribute? Is the capturing required? No specific reason, so removed it. https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:19: if (video.currentTime == 1) On 2016/08/18 10:39:06, fs wrote: > IIUC, previously only the first 'seeked' was actually "useful" (the testedOnce > flag), so does this condition serve any purpose? Could we just call > runAfterLayoutAndPaint directly? (If we don't it looks like the test will time > out - getting a rendering error seems like a better indication of error.) Done. https://codereview.chromium.org/2257043002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/compositing/video/video-reflection.html:20: runAfterLayoutAndPaint(function() { testRunner.notifyDone(); }); On 2016/08/18 10:39:06, fs wrote: > If you pass true as the second argument, the function will do notifyDone after > invoking the callback, so the callback can just be empty if so. Done.
lgtm
The CQ bit was checked by srirama.m@samsung.com
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 ========== Remove video-test.js dependency from video-reflection.html BUG=588956 ========== to ========== Remove video-test.js dependency from video-reflection.html BUG=588956 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Remove video-test.js dependency from video-reflection.html BUG=588956 ========== to ========== Remove video-test.js dependency from video-reflection.html BUG=588956 Committed: https://crrev.com/91eead9f48de00e0744d62344d4310aca1b9178f Cr-Commit-Position: refs/heads/master@{#412810} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/91eead9f48de00e0744d62344d4310aca1b9178f Cr-Commit-Position: refs/heads/master@{#412810} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
