Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1881733004:
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching (Closed)
|
Created:
4 years, 8 months ago by wolenetz Modified:
4 years, 8 months ago CC:
chromium-reviews, nessy, mlamouri+watch-blink_chromium.org, blink-reviews-html_chromium.org, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, blink-reviews, vcarbune.chromium, philipj_slow Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
Committed: https://crrev.com/b16f202aa67513f74576312f7f8a3ad17f60458a
Cr-Commit-Position: refs/heads/master@{#389649}
Patch Set 1 #Patch Set 2 : Fixes indentation in test script #
Total comments: 19
Patch Set 3 : just rebase #Patch Set 4 : Just rebase #Patch Set 5 : Addressed comments in the *.html file; need to update .cpp and add MediaStream+invalidBlobURL tests #Patch Set 6 : Updated .cpp, added invalidBlobURL tests, MediaStream test PR needs prep at web-platform-tests upst… #
Total comments: 6
Patch Set 7 : rebase #Patch Set 8 : Address remainder of comments to date. Pending potential PR of MediaStream test to wpt [2] #
Total comments: 1
Patch Set 9 : Fix more '->" in the new test. [2] is now pending review at wpt. #
Messages
Total messages: 43 (19 generated)
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/1881733004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881733004/20001
wolenetz@chromium.org changed reviewers: + philipj@opera.com
Please take a look. I think I've made this as surgical/precise as possible, with a test that should enable regression detection in future. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
friendly ping :)
On 2016/04/13 17:42:54, wolenetz wrote: > friendly ping :) Spec change ([1] from the CL description) has now landed.
Description was changed from ========== MSE: Ignore mediaElement preload attribute on resource fetching Prevents the usage of the optional resource fetch algorithm steps for when preload='none' in the MSE case. Complies with expected update to MSE spec [1] and fixes the related crbug.com/539707. Includes test coverage. [1] https://github.com/w3c/media-source/pull/64 BUG=539707 TEST=http/tests/media/media-source/mediasource-attach-preload.html ========== to ========== MSE: Ignore mediaElement preload attribute on resource fetching Prevents the usage of the optional resource fetch algorithm steps for when preload='none' in the MSE case. Complies with recent MSE spec update [1] and fixes the related crbug.com/539707. Includes test coverage. [1] https://github.com/w3c/media-source/pull/64 BUG=539707 TEST=http/tests/media/media-source/mediasource-attach-preload.html ==========
srirama.m@samsung.com changed reviewers: + srirama.m@samsung.com
https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:7: <link rel='stylesheet' href='/w3c/resources/testharness.css'> nit: may be use double quotes here to be consistent. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:10: <div id="log"></div> do we need this div?
https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1098: if (!m_mediaSource && effectivePreloadType() == WebMediaPlayer::PreloadNone) { I would like it very much if this matched the behavior of https://github.com/whatwg/html/pull/1037 even if not done in precisely the same place. In other words, it'd be nice if MediaStream was also exempted here. Can you use isObjectOrBlobURL perhaps? That doesn't quite match the spec because it'll be true even for a bogus blob: URL, but maybe that's a better solution anyway. If it's a bogus blob: URL the load will just fail quicker.
A similar test for MediaStream and an invalid blob: URL would also be good if you go that route. Feedback on https://github.com/whatwg/html/pull/1037 is of course also appreciated. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:10: <div id="log"></div> On 2016/04/14 05:13:07, Srirama wrote: > do we need this div? Nope :) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:16: var mediaTag = document.createElement('video'); s/mediaTag/video/ since it's always a video https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:27: if (removeMediaElement) { It looks like what you want for this is test.add_cleanup, you shouldn't need to replace the done method. You also shouldn't need the removeMediaElement bookkeeping with that. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:38: test.waitForExpectedEvents(function() If this is the only reason you need yo use media_test, then just mediaSource.onsourceopen = test.step_func(...) would save a line and remove the extra dependency :) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:40: URL.revokeObjectURL(mediaSourceURL); Maybe put this in the cleanup callback in case the test times out or otherwise fails before this point? https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:46: attachWithPreloadTest(); I don't think it's necessary to test the default behavior, since it's going to be one of auto/metadata/none. Also makes test slightly simpler above.
Description was changed from ========== MSE: Ignore mediaElement preload attribute on resource fetching Prevents the usage of the optional resource fetch algorithm steps for when preload='none' in the MSE case. Complies with recent MSE spec update [1] and fixes the related crbug.com/539707. Includes test coverage. [1] https://github.com/w3c/media-source/pull/64 BUG=539707 TEST=http/tests/media/media-source/mediasource-attach-preload.html ========== to ========== MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching Prevents the usage of the optional resource fetch algorithm steps for when preload='none' in the MSE, MediaStream source, or any blob URL cases. Complies with expected update to MSE spec [1] and fixes the related crbug.com/539707. Includes test coverage for MSE and invalid (revoked MSE object) blob URLs. MediaStream test coverage is expected to come from later import of a new web-platform-tests [2]. [1] https://github.com/w3c/media-source/pull/64 [2] TODO BUG=539707 TEST=http/tests/media/media-source/mediasource-preload.html ==========
Please take a look. I plan to prep a PR for the w3c/web-platform-tests/media-stream for adding the preload tests there separately from this CL. I'd like to reference that PR in the description before landing this (see current 'TODO' in the description). https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:7: <link rel='stylesheet' href='/w3c/resources/testharness.css'> On 2016/04/14 05:13:07, Srirama wrote: > nit: may be use double quotes here to be consistent. Copy-paste from another media-source layout test. I'll fix those in a different cleanup CL (possibly as part of the round-tripping of these layout test to w3c/web-platform-tests and back into blink imported.) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:10: <div id="log"></div> On 2016/04/14 13:16:30, philipj_slow wrote: > On 2016/04/14 05:13:07, Srirama wrote: > > do we need this div? > > Nope :) Yet another copy-paste from another media-source layout test. I'll fix those in a different cleanup CL (possibly as part of the round-tripping of these layout test to w3c/web-platform-tests and back into blink imported.) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:16: var mediaTag = document.createElement('video'); On 2016/04/14 13:16:30, philipj_slow wrote: > s/mediaTag/video/ since it's always a video Done. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:27: if (removeMediaElement) { On 2016/04/14 13:16:30, philipj_slow wrote: > It looks like what you want for this is test.add_cleanup, you shouldn't need to > replace the done method. You also shouldn't need the removeMediaElement > bookkeeping with that. Yet another copy-paste from another media-source layout test (and from similar in mediasource-util.js). I'll fix those in a different cleanup CL (possibly as part of the round-tripping of these layout test to w3c/web-platform-tests and back into blink imported.) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:38: test.waitForExpectedEvents(function() On 2016/04/14 13:16:30, philipj_slow wrote: > If this is the only reason you need yo use media_test, then just > mediaSource.onsourceopen = test.step_func(...) would save a line and remove the > extra dependency :) Done, though "mediaSource.onsourceopen = ..." didn't work. We've used addEventListener/removeEventListener solely throughout media-source layout tests. Is "onsourceopen" attribute setting expected to work for MSE? If so, seems there's a bug to fix here too :) https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:40: URL.revokeObjectURL(mediaSourceURL); On 2016/04/14 13:16:30, philipj_slow wrote: > Maybe put this in the cleanup callback in case the test times out or otherwise > fails before this point? Done. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:46: attachWithPreloadTest(); On 2016/04/14 13:16:30, philipj_slow wrote: > I don't think it's necessary to test the default behavior, since it's going to > be one of auto/metadata/none. Also makes test slightly simpler above. Done. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1098: if (!m_mediaSource && effectivePreloadType() == WebMediaPlayer::PreloadNone) { On 2016/04/14 12:39:27, philipj_slow wrote: > I would like it very much if this matched the behavior of > https://github.com/whatwg/html/pull/1037 even if not done in precisely the same > place. In other words, it'd be nice if MediaStream was also exempted here. > > Can you use isObjectOrBlobURL perhaps? That doesn't quite match the spec because > it'll be true even for a bogus blob: URL, but maybe that's a better solution > anyway. If it's a bogus blob: URL the load will just fail quicker. I've added isObjectOrBlobURL usage here, but adding a related MediaStream layout test is interesting: it looks like that would best be done in the web-platform-tests repo, and then imported into Blink the way other media-stream imported tests are. I'll put together such a wpt test PR shortly and eventual import of it should hopefully satisfy the mediastream portion of this CR question. I've added an invalid blob url non-MSE/non-MS test with preload=none into this CL.
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/1881733004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881733004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:38: test.waitForExpectedEvents(function() On 2016/04/21 21:37:30, wolenetz wrote: > On 2016/04/14 13:16:30, philipj_slow wrote: > > If this is the only reason you need yo use media_test, then just > > mediaSource.onsourceopen = test.step_func(...) would save a line and remove > the > > extra dependency :) > > Done, though "mediaSource.onsourceopen = ..." didn't work. We've used > addEventListener/removeEventListener solely throughout media-source layout > tests. Is "onsourceopen" attribute setting expected to work for MSE? If so, > seems there's a bug to fix here too :) Ah, I just assumed that the MediaSource interface would have event handler attributes, but it doesn't. Filed https://github.com/w3c/media-source/issues/66 so addEventListener will do for now. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (right): https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:15: var videoTag = document.createElement('video'); I was thinking just "video", I take "tag" to mean the string "<video>" that appears in markup and not the actual object it (or createElement) creates. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:28: mediaSource.removeEventListener("sourceopen", listener); Do you need to remove the event listener, can the event fire again? If not, then just leaving it and using test.step_func_done() as the event listener below makes this a bit smaller. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:64: }, "error occurs with bogus (revoked) object URL and element preload=" + preload); This is good, I didn't think to test revoked URLs. Can you also add a test for even more bogus URLs like blob:a that don't even look like a valid blob URL, but would still ignore preload=none?
Thanks for review. I'll get to work on the wpt upstream PR (and fill in [2] here). If that becomes delayed, I'll just remove the note to [2] here and land this. https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html (right): https://codereview.chromium.org/1881733004/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-attach-preload.html:38: test.waitForExpectedEvents(function() On 2016/04/22 09:22:15, philipj_slow wrote: > On 2016/04/21 21:37:30, wolenetz wrote: > > On 2016/04/14 13:16:30, philipj_slow wrote: > > > If this is the only reason you need yo use media_test, then just > > > mediaSource.onsourceopen = test.step_func(...) would save a line and remove > > the > > > extra dependency :) > > > > Done, though "mediaSource.onsourceopen = ..." didn't work. We've used > > addEventListener/removeEventListener solely throughout media-source layout > > tests. Is "onsourceopen" attribute setting expected to work for MSE? If so, > > seems there's a bug to fix here too :) > > Ah, I just assumed that the MediaSource interface would have event handler > attributes, but it doesn't. Filed https://github.com/w3c/media-source/issues/66 > so addEventListener will do for now. Acknowledged. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (right): https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:15: var videoTag = document.createElement('video'); On 2016/04/22 09:22:15, philipj_slow wrote: > I was thinking just "video", I take "tag" to mean the string "<video>" that > appears in markup and not the actual object it (or createElement) creates. Done. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:28: mediaSource.removeEventListener("sourceopen", listener); On 2016/04/22 09:22:15, philipj_slow wrote: > Do you need to remove the event listener, can the event fire again? If not, then > just leaving it and using test.step_func_done() as the event listener below > makes this a bit smaller. Done. https://codereview.chromium.org/1881733004/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:64: }, "error occurs with bogus (revoked) object URL and element preload=" + preload); On 2016/04/22 09:22:15, philipj_slow wrote: > This is good, I didn't think to test revoked URLs. Can you also add a test for > even more bogus URLs like blob:a that don't even look like a valid blob URL, but > would still ignore preload=none? Done. (blob:a and real unrevoked MediaSource objectURL with a char appended to the end)
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/1881733004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881733004/140001
srirama.m@: did you want to take another look at this?
https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html (right): https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:74: errorWithPreloadTest('auto', 'blob:a'); Ugh. It looks like I need to s/'/" a bit more here....
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with expected update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] TODO
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with expected update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with expected update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with expected update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with expected update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
Under the assumption that srirama.m@'s earlier comments were drive-by nits, I'll land this shortly with TBR for srirama.m@. Thanks for reviews.
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com Link to the patchset: https://codereview.chromium.org/1881733004/#ps160001 (title: "Fix more '->" in the new test. [2] is now pending review at wpt.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1881733004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1881733004/160001
Message was sent while issue was closed.
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
==========
to
==========
MSE, MS, and any blob URL: Ignore preload 'none' on resource fetching
Prevents the usage of the optional resource fetch algorithm steps for
when preload='none' in the MSE, MediaStream source, or any blob URL
cases. Complies with recent update to MSE spec [1] and fixes the
related crbug.com/539707. Includes test coverage for MSE and invalid
(revoked MSE object) blob URLs. MediaStream test coverage is expected to
come from later import of a new web-platform-tests [2].
[1] https://github.com/w3c/media-source/pull/64
[2] https://github.com/w3c/web-platform-tests/pull/2895
R=philipj@opera.com
TBR=srirama.m@samsung.com
BUG=539707
TEST=http/tests/media/media-source/mediasource-preload.html
Committed: https://crrev.com/b16f202aa67513f74576312f7f8a3ad17f60458a
Cr-Commit-Position: refs/heads/master@{#389649}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b16f202aa67513f74576312f7f8a3ad17f60458a Cr-Commit-Position: refs/heads/master@{#389649}
Message was sent while issue was closed.
On 2016/04/25 20:41:05, wolenetz wrote: > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html > (right): > > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:74: > errorWithPreloadTest('auto', 'blob:a'); > Ugh. It looks like I need to s/'/" a bit more here.... Sorry for the late response. I am GMT+5:30 time zone. As long as we are consistent within script code it is fine. HTML and Script can still use separate quotes (All double quotes in HTML and all single quotes in Script or vice versa, only thing is we need to be consistent within HTML and Script). All the changes look fine except a few nits though not so important. As Philip pointed out we can replace ************************************************* test.step_func(function(event) { test.done(); }); ************************************************* with ********************** test.step_func_done(); ********************** To make it more simpler and readable. Never mind, i will take care of it :), anyway i am working on cleaning up of media layout tests.
Message was sent while issue was closed.
On 2016/04/26 05:03:18, Srirama wrote: > On 2016/04/25 20:41:05, wolenetz wrote: > > > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > > File > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html > > (right): > > > > > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:74: > > errorWithPreloadTest('auto', 'blob:a'); > > Ugh. It looks like I need to s/'/" a bit more here.... > > Sorry for the late response. I am GMT+5:30 time zone. > As long as we are consistent within script code it is fine. HTML and Script can > still use separate quotes (All double quotes in HTML and all single quotes in > Script or vice versa, only thing is we need to be consistent within HTML and > Script). > All the changes look fine except a few nits though not so important. As Philip > pointed out we can replace > > ************************************************* > test.step_func(function(event) { test.done(); }); > ************************************************* > > with > > ********************** > test.step_func_done(); > ********************** > > To make it more simpler and readable. > Never mind, i will take care of it :), anyway i am working on cleaning up of > media layout tests. Thanks Srirama. test.done() is prevalent everywhere in media layout tests currently and I missed the shortcut again while addressing philipj@'s other comments.
Message was sent while issue was closed.
On 2016/04/26 17:56:35, wolenetz wrote: > On 2016/04/26 05:03:18, Srirama wrote: > > On 2016/04/25 20:41:05, wolenetz wrote: > > > > > > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > > > File > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html > > > (right): > > > > > > > > > https://codereview.chromium.org/1881733004/diff/140001/third_party/WebKit/Lay... > > > > > > third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-preload.html:74: > > > errorWithPreloadTest('auto', 'blob:a'); > > > Ugh. It looks like I need to s/'/" a bit more here.... > > > > Sorry for the late response. I am GMT+5:30 time zone. > > As long as we are consistent within script code it is fine. HTML and Script > can > > still use separate quotes (All double quotes in HTML and all single quotes in > > Script or vice versa, only thing is we need to be consistent within HTML and > > Script). > > All the changes look fine except a few nits though not so important. As Philip > > pointed out we can replace > > > > ************************************************* > > test.step_func(function(event) { test.done(); }); > > ************************************************* > > > > with > > > > ********************** > > test.step_func_done(); > > ********************** > > > > To make it more simpler and readable. > > Never mind, i will take care of it :), anyway i am working on cleaning up of > > media layout tests. > > Thanks Srirama. test.done() is prevalent everywhere in media layout tests > currently and I missed the shortcut again while addressing philipj@'s other > comments. There's at least one real issue here: test.fail() isn't defined. I'll post a CL to fix that and include these other cleanups shortly. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
