Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1920383002:
MSE: Clean up the mediasource-preload layout test (Closed)
DescriptionMSE: Clean up the mediasource-preload layout test
Includes some clean up to make this test more readable and correct. See
https://crrev.com/b16f202a for the test's recent creation.
BUG=539707
Committed: https://crrev.com/c030c66c552934c482f2264514fa7aff26e8af0b
Cr-Commit-Position: refs/heads/master@{#390164}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addresses all remaining comments #Messages
Total messages: 18 (8 generated)
wolenetz@chromium.org changed reviewers: + philipj@opera.com, srirama.m@samsung.com
Please take a look. See https://codereview.chromium.org/1881733004/#msg43 for context. I'm also doing similar cleanup in the PR for upstreaming this test to web-platform-tests (https://github.com/w3c/web-platform-tests/pull/2896).
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920383002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920383002/1
Description was changed from ========== MSE: Clean up the mediasoure-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 ========== to ========== MSE: Clean up the mediasource-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 ==========
fyi - similar cleanup will be included in my upcoming work to upstream many of these MSE layout tests to wpt. The idea is to then import them back into blink under the imported/ layout tests area, and then get them running under automation. There will be a period of time when changes to these blink tests will need to be made both here and in wpt upstream prior to us getting the imported-back-to-blink MSE tests running under our automation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM % nit https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (left): https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:7: <link rel="stylesheet" href="/w3c/resources/testharness.css"> I think we can remove this css file. I ran it locally without this and it passes.
lgtm https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (left): https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:7: <link rel="stylesheet" href="/w3c/resources/testharness.css"> On 2016/04/27 04:56:04, Srirama wrote: > I think we can remove this css file. I ran it locally without this and it > passes. Yep, Output.prototype.show_results in testharness.js adds the stylesheet automatically. https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (right): https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:57: mediaSource.addEventListener("sourceopen", test.unreached_func("'sourceopen' should not occur")); I guess I would say "should not be fired" to make it abundantly clear that it's an event
Thanks for review - this in-depth review for this new layout test is really going to help me improve the quality of the Blink MSE layout and W3C MSE compliance test suite. https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (left): https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:7: <link rel="stylesheet" href="/w3c/resources/testharness.css"> On 2016/04/27 12:36:27, philipj_slow wrote: > On 2016/04/27 04:56:04, Srirama wrote: > > I think we can remove this css file. I ran it locally without this and it > > passes. > > Yep, Output.prototype.show_results in testharness.js adds the stylesheet > automatically. Done. I also removed the inclusion of mediasource-util.js, since it's not used here. https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (right): https://codereview.chromium.org/1920383002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:57: mediaSource.addEventListener("sourceopen", test.unreached_func("'sourceopen' should not occur")); On 2016/04/27 12:36:27, philipj_slow wrote: > I guess I would say "should not be fired" to make it abundantly clear that it's > an event Done.
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from srirama.m@samsung.com, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1920383002/#ps20001 (title: "Addresses all remaining comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1920383002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1920383002/20001
Message was sent while issue was closed.
Description was changed from ========== MSE: Clean up the mediasource-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 ========== to ========== MSE: Clean up the mediasource-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c030c66c552934c482f2264514fa7aff26e8af0b Cr-Commit-Position: refs/heads/master@{#390164}
Message was sent while issue was closed.
Description was changed from ========== MSE: Clean up the mediasource-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 ========== to ========== MSE: Clean up the mediasource-preload layout test Includes some clean up to make this test more readable and correct. See https://crrev.com/b16f202a for the test's recent creation. BUG=539707 Committed: https://crrev.com/c030c66c552934c482f2264514fa7aff26e8af0b Cr-Commit-Position: refs/heads/master@{#390164} ========== |
