Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2081513002:
Remove video-test.js dependency from track-cue-rendering* tests (Closed)
|
Created:
4 years, 6 months ago by Srirama Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, gasubic, mlamouri+watch-blink_chromium.org, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove video-test.js dependency from track-cue-rendering* tests
BUG=588956
Committed: https://crrev.com/a35861afa2ff872f98d9f4becc8c1ea02655b311
Cr-Commit-Position: refs/heads/master@{#406532}
Patch Set 1 : #Patch Set 2 : Rebase #Patch Set 3 : add reference tests #
Total comments: 1
Patch Set 4 : remove video-test.js dependency #
Messages
Total messages: 28 (12 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== fix BUG= ========== to ========== Convert track-cues-rendering* tests to testharness.js Cleaning up track-rendering* tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
Description was changed from ========== Convert track-cues-rendering* tests to testharness.js Cleaning up track-rendering* tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Convert track-cue-rendering* tests to testharness.js Cleaning up track-cue-rendering* tests in media/track to use testharness.js instead of video-test.js. This will enable to upstream these tests to web-platform-tests. BUG=588956 ==========
The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081513002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by srirama.m@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2081513002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
srirama.m@samsung.com changed reviewers: + foolip@chromium.org, fs@opera.com
PTAL. This is the last one from track folder.
It looks like these are a poor fit for testharness tests (and video-test...)? Maybe reftests instead?
On 2016/06/19 17:32:07, fs wrote: > It looks like these are a poor fit for testharness tests (and video-test...)? > Maybe reftests instead? ok, i will try making reference tests.
Added reference tests. I followed track-cue-rendering-after-controls-added.html. But is this correct? Can we use cue properties like line, vertical in reference tests?
On 2016/07/19 at 14:03:09, srirama.m wrote:
> Added reference tests. I followed
track-cue-rendering-after-controls-added.html. But is this correct? Can we use
cue properties like line, vertical in reference tests?
I think we want something that does not use the cue rendering code as the
reference. There are some other tests, like
track-cue-rendering-position-auto{,-expected}.html, that uses a more elaborate
setup. I think we could consider just removing the video-test.js dependency as a
first step, and then look at the ref-test conversion.
https://codereview.chromium.org/2081513002/diff/60001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-vertical.html
(right):
https://codereview.chromium.org/2081513002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/track/track-cue-rendering-vertical.html:9:
testRunner.waitUntilDone();
I'd suggest moving this down to just before setting up the event-handler that
calls notifyDone. This way the test will work better even when there's no
testRunner. Same in the *-horizontal test.
On 2016/07/19 at 14:17:05, fs wrote:
> On 2016/07/19 at 14:03:09, srirama.m wrote:
> > Added reference tests. I followed
track-cue-rendering-after-controls-added.html. But is this correct? Can we use
cue properties like line, vertical in reference tests?
>
> I think we want something that does not use the cue rendering code as the
reference. There are some other tests, like
track-cue-rendering-position-auto{,-expected}.html, that uses a more elaborate
setup. I think we could consider just removing the video-test.js dependency as a
first step, and then look at the ref-test conversion.
(There also a bunch of, probably better examples, in
media/track/opera/track/webvtt/rendering/reftest/)
Description was changed from ========== Convert track-cue-rendering* tests to testharness.js Cleaning up track-cue-rendering* tests in media/track 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 track-cue-rendering* tests BUG=588956 ==========
On 2016/07/19 14:19:20, fs wrote:
> On 2016/07/19 at 14:17:05, fs wrote:
> > On 2016/07/19 at 14:03:09, srirama.m wrote:
> > > Added reference tests. I followed
> track-cue-rendering-after-controls-added.html. But is this correct? Can we use
> cue properties like line, vertical in reference tests?
> >
> > I think we want something that does not use the cue rendering code as the
> reference. There are some other tests, like
> track-cue-rendering-position-auto{,-expected}.html, that uses a more elaborate
> setup. I think we could consider just removing the video-test.js dependency as
a
> first step, and then look at the ref-test conversion.
>
> (There also a bunch of, probably better examples, in
> media/track/opera/track/webvtt/rendering/reftest/)
ok, i will look into these test cases, for now removed video-test.js dependency
as suggested.
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 track-cue-rendering* tests BUG=588956 ========== to ========== Remove video-test.js dependency from track-cue-rendering* tests BUG=588956 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Remove video-test.js dependency from track-cue-rendering* tests BUG=588956 ========== to ========== Remove video-test.js dependency from track-cue-rendering* tests BUG=588956 Committed: https://crrev.com/a35861afa2ff872f98d9f4becc8c1ea02655b311 Cr-Commit-Position: refs/heads/master@{#406532} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a35861afa2ff872f98d9f4becc8c1ea02655b311 Cr-Commit-Position: refs/heads/master@{#406532} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
