Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(34)

Issue 2315113002: MSE: Throw TypeError instead of InvalidAccessError per spec update (Closed)

Created:
4 years, 3 months ago by wolenetz
Modified:
4 years, 3 months ago
Reviewers:
chcunningham, foolip
CC:
chromium-reviews, mlamouri+watch-blink_chromium.org, eric.carlson_apple.com, haraken, feature-media-reviews_chromium.org, blink-reviews, Srirama, foolip
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

MSE: Throw TypeError instead of InvalidAccessError per spec update Conforms to https://github.com/w3c/media-source/pull/47 spec change for certain exceptional conditions, leaving a couple pieces for later changes: 1) bug 607372 "Comply with forcing appendMode to remain 'sequence' for MSE mp3 and adts stream parsers" 2) bug 644863 "Update TrackDefault and TrackDefaultList to use TypeError correctly" BUG=623781 TEST=updated http/tests/media/media-source tests, and current related upstream w3c MSE tests Committed: https://crrev.com/bf5ee5415899e8e8c9e3ac2385d7092bbd11a3d8 Cr-Commit-Position: refs/heads/master@{#417107}

Patch Set 1 #

Patch Set 2 : addSourceBuffer(null) is really addSourceBuffer("null") in at least Blink #

Total comments: 10

Messages

Total messages: 21 (11 generated)
wolenetz
Please take a look. cc+=foolip@ as FYI around the addSourceBuffer(null) test case. With this CL, ...
4 years, 3 months ago (2016-09-07 02:09:08 UTC) #4
wolenetz
On 2016/09/07 02:09:08, wolenetz wrote: > ...(except, at the moment, the upstream > addSourceBuffer(null) test ...
4 years, 3 months ago (2016-09-07 18:32:03 UTC) #7
chcunningham
Looks pretty good. I think you're missing 3 cases in that pull request to emit ...
4 years, 3 months ago (2016-09-07 18:36:06 UTC) #9
foolip
Thanks for the context, left a comment you can ignore :) https://codereview.chromium.org/2315113002/diff/20001/third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html File third_party/WebKit/LayoutTests/http/tests/media/media-source/mediasource-addsourcebuffer.html (right): ...
4 years, 3 months ago (2016-09-07 19:42:49 UTC) #11
wolenetz
Thanks for reviews. chcunningham@, here are inline replies to your previous comment: > 1. setting ...
4 years, 3 months ago (2016-09-07 21:09:27 UTC) #13
wolenetz
On 2016/09/07 21:09:27, wolenetz wrote: > Thanks for reviews. > chcunningham@, here are inline replies ...
4 years, 3 months ago (2016-09-07 21:27:42 UTC) #14
chcunningham
lgtm
4 years, 3 months ago (2016-09-07 22:53:23 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2315113002/20001
4 years, 3 months ago (2016-09-07 22:54:01 UTC) #17
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 3 months ago (2016-09-07 22:58:33 UTC) #19
commit-bot: I haz the power
4 years, 3 months ago (2016-09-07 23:00:14 UTC) #21
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/bf5ee5415899e8e8c9e3ac2385d7092bbd11a3d8
Cr-Commit-Position: refs/heads/master@{#417107}

Powered by Google App Engine
This is Rietveld 408576698