|
|
Created:
4 years, 5 months ago by Srirama Modified:
4 years, 5 months ago Reviewers:
fs, foolip CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org 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 [audio, video]-autoplay-* tests
BUG=588956
Committed: https://crrev.com/2fce54e4d537bc7a36acc0bd7e514aa5a2818564
Cr-Commit-Position: refs/heads/master@{#405166}
Patch Set 1 #
Created: 4 years, 5 months ago
Messages
Total messages: 23 (8 generated)
Description was changed from ========== fix BUG= ========== to ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests Remove video-test.js dependency from [audio, video]-autoplay-* tests. 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
lgtm This CL doesn't appear to improve/aid "upstreamability", so the description should probably be updated accordingly. (Removing the TODO seems ok to me though because of what aspects of the implementation these tests test.)
On 2016/07/12 17:42:34, fs (ooo to July 18) wrote: > lgtm > > This CL doesn't appear to improve/aid "upstreamability", so the description > should probably be updated accordingly. (Removing the TODO seems ok to me though > because of what aspects of the implementation these tests test.) doesn't getting rid of video-test.js qualify for upstreaming? Only tests using testharness.js are qualified for upstreaming?
On 2016/07/13 at 07:02:28, srirama.m wrote: > On 2016/07/12 17:42:34, fs (ooo to July 18) wrote: > > lgtm > > > > This CL doesn't appear to improve/aid "upstreamability", so the description > > should probably be updated accordingly. (Removing the TODO seems ok to me though > > because of what aspects of the implementation these tests test.) > > doesn't getting rid of video-test.js qualify for upstreaming? Only tests using testharness.js are qualified for upstreaming? It may not be a strict requirement, but I'd go as far as saying it is preferable (especially for "can write as JS-only"-tests.) That being said though, these tests are for some non-uservisible "feature" ("autoplay experiment"), so can't make little sense to upstream regardless.
Description was changed from ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests Remove video-test.js dependency from [audio, video]-autoplay-* tests. This will enable to upstream these tests to web-platform-tests. BUG=588956 ========== to ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests BUG=588956 ==========
On 2016/07/13 14:27:31, fs (ooo to July 18) wrote: > On 2016/07/13 at 07:02:28, srirama.m wrote: > > On 2016/07/12 17:42:34, fs (ooo to July 18) wrote: > > > lgtm > > > > > > This CL doesn't appear to improve/aid "upstreamability", so the description > > > should probably be updated accordingly. (Removing the TODO seems ok to me > though > > > because of what aspects of the implementation these tests test.) > > > > doesn't getting rid of video-test.js qualify for upstreaming? Only tests using > testharness.js are qualified for upstreaming? > > It may not be a strict requirement, but I'd go as far as saying it is preferable > (especially for "can write as JS-only"-tests.) That being said though, these > tests are for some non-uservisible "feature" ("autoplay experiment"), so can't > make little sense to upstream regardless. Ok, understood, thanks.
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...
The CQ bit was unchecked by foolip@chromium.org
I've unchecked CQ. These are tests that cannot be upstreamed and will be removed as soon as the experiment is over, so I'd suggest instead just removing the TODOs.
On 2016/07/13 15:11:00, foolip wrote: > I've unchecked CQ. These are tests that cannot be upstreamed and will be removed > as soon as the experiment is over, so I'd suggest instead just removing the > TODOs. Sorry that I didn't check all the tests when adding the TODOs, there were so many :/
On 2016/07/13 at 15:11:22, foolip wrote: > On 2016/07/13 15:11:00, foolip wrote: > > I've unchecked CQ. These are tests that cannot be upstreamed and will be removed > > as soon as the experiment is over, so I'd suggest instead just removing the > > TODOs. > > Sorry that I didn't check all the tests when adding the TODOs, there were so many :/ I think removing all traces of video-test.js is worthwhile - leave a tool in the toolbox and someone will undoubtedly use it...
On 2016/07/13 15:14:46, fs (ooo to July 18) wrote: > On 2016/07/13 at 15:11:22, foolip wrote: > > On 2016/07/13 15:11:00, foolip wrote: > > > I've unchecked CQ. These are tests that cannot be upstreamed and will be > removed > > > as soon as the experiment is over, so I'd suggest instead just removing the > > > TODOs. > > > > Sorry that I didn't check all the tests when adding the TODOs, there were so > many :/ > > I think removing all traces of video-test.js is worthwhile - leave a tool in the > toolbox and someone will undoubtedly use it... The media fragments tests were left alone, so they're still around. But I guess if they're the only ones, then video-test.js can be renamed to media-fragment-test.js and minimized to what is needed. So LGTM then :)
The CQ bit was checked by foolip@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/13 15:21:37, foolip wrote: > On 2016/07/13 15:14:46, fs (ooo to July 18) wrote: > > On 2016/07/13 at 15:11:22, foolip wrote: > > > On 2016/07/13 15:11:00, foolip wrote: > > > > I've unchecked CQ. These are tests that cannot be upstreamed and will be > > removed > > > > as soon as the experiment is over, so I'd suggest instead just removing > the > > > > TODOs. > > > > > > Sorry that I didn't check all the tests when adding the TODOs, there were so > > many :/ > > > > I think removing all traces of video-test.js is worthwhile - leave a tool in > the > > toolbox and someone will undoubtedly use it... > > The media fragments tests were left alone, so they're still around. But I guess > if they're the only ones, then video-test.js can be renamed to > media-fragment-test.js and minimized to what is needed. So LGTM then :) Agree with both of you. And i have already thought of minimizing it after finishing all the test cases. There are just a few test cases left.
Message was sent while issue was closed.
Description was changed from ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests BUG=588956 ========== to ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests BUG=588956 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
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 [audio, video]-autoplay-* tests BUG=588956 ========== to ========== Remove video-test.js dependency from [audio, video]-autoplay-* tests BUG=588956 Committed: https://crrev.com/2fce54e4d537bc7a36acc0bd7e514aa5a2818564 Cr-Commit-Position: refs/heads/master@{#405166} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/2fce54e4d537bc7a36acc0bd7e514aa5a2818564 Cr-Commit-Position: refs/heads/master@{#405166} |