Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1945523003:
MSE: Add EventHandler attributes to MSE objects (Closed)
|
Created:
4 years, 7 months ago by wolenetz Modified:
4 years, 5 months ago CC:
blink-reviews, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, haraken, mlamouri+watch-blink_chromium.org, tkent, jochen (gone - plz use gerrit) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMSE: Add EventHandler attributes to MSE objects
Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds
EventHandler attributes for MSE objects as follows:
Attributes for MediaSource events:
* onsourceopen
* onsourceended
* onsourceclose
Attributes for SourceBuffer events:
* onupdatestart
* onupdate
* onupdateend
* onerror
* onabort
Attributes for SourceBufferList events:
* onaddsourcebuffer
* onremovesourcebuffer
[1] https://github.com/w3c/media-source/pull/75
[2] https://github.com/w3c/media-source/issues/66
[3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE
R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org
BUG=623678
Committed: https://crrev.com/00003ed19f12e054784ee59b95c863b380361ef4
Cr-Commit-Position: refs/heads/master@{#402946}
Patch Set 1 #Patch Set 2 : rebase #Patch Set 3 : rebase #Messages
Total messages: 42 (17 generated)
Please take a look: chcunningham@: all as new MSE blink modules co-OWNER (congrats!) jochen@: L/v/s/w/global-interface-listing-expected.txt mlamouri@: all (as modules OWNER), since this is the first time I've worked with blink EventHandler attributes and philipj@ is no longer MSE modules co-OWNER. I suspect I'll need to also add some layout tests to confirm the behavior of these event handler attributes, even though they're all already tested in "addEventListener" form elsewhere in the MSE layout tests. WDYT?
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/1945523003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945523003/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can you please point at the intent to ship thread?
lgtm with intent to ship in the CL description and approved :)
Intent to ship: great point! I'll go start one :)
On 2016/05/04 16:46:53, wolenetz_OoO_May5-9 wrote: > Intent to ship: great point! I'll go start one :) Sending the intent is on hold until the MSE spec PR lands and demonstrates wider intent from outside Chromium. While it's on hold and while I prepare the intent, I'd like some guidance: For a change like this, would you recommend the new attributes be conditionally exposed using --enable-experimental-web-platform-features, conditionally hidden using some new flag like --disable-media-source-eventhandler-attributes, or unconditionally exposed?
On 2016/05/04 at 18:21:49, wolenetz wrote: > On 2016/05/04 16:46:53, wolenetz_OoO_May5-9 wrote: > > Intent to ship: great point! I'll go start one :) > > Sending the intent is on hold until the MSE spec PR lands and demonstrates wider intent from outside Chromium. > > While it's on hold and while I prepare the intent, I'd like some guidance: > For a change like this, would you recommend the new attributes be conditionally exposed using --enable-experimental-web-platform-features, conditionally hidden using some new flag like --disable-media-source-eventhandler-attributes, or unconditionally exposed? It should be behind experimental web platform features
lgtm
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/v2/patch-status/codereview.chromium.or...
On 2016/05/04 10:57:33, jochen wrote: > can you please point at the intent to ship thread? Reviving this CL: Intent to implement and ship thread is now at: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE Launch tracking bug is https://crbug.com/623678 Implementation bug is still https://crbug.com/608912 jochen@ Please review. Thanks!
Description was changed from ========== MSE: Add EventHandler attributes to MSE objects Due to expected spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=608912 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to expected spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=608912, 623678 ==========
wolenetz@chromium.org changed reviewers: + rbyers@chromium.org, tkent@chromium.org
Adding a couple more API owners, per the "Ship" requirement step #5 in http://www.chromium.org/blink#launch-process.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to expected spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=608912, 623678 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to expected spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=623678 ==========
(waiting for the blink-dev thread to receive the required three approvals)
Description was changed from ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to expected spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=623678 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=623678 ==========
Intent is now approved. webexposed/ LGTM
On 2016/06/29 14:25:45, Rick Byers wrote: > Intent is now approved. > webexposed/ LGTM Thanks for the review. Question on procedure: The intent received 3 API owner approvals. Do I need to also await 3 API owner l*tms on this CL? (my c#17 assumed that, though rereading launch-process seems to imply that "3" applies to the intent and not the CL.)
On 2016/06/29 18:14:12, wolenetz wrote: > On 2016/06/29 14:25:45, Rick Byers wrote: > > Intent is now approved. > > webexposed/ LGTM > > Thanks for the review. > > Question on procedure: The intent received 3 API owner approvals. Do I need to > also await 3 API owner l*tms on this CL? (my c#17 assumed that, though rereading > launch-process seems to imply that "3" applies to the intent and not the CL.) No that would be crazy :-). The stable/webexposed/ OWNER check is just a sanity-check to help ensure the intent approval has happened. You're all good to land this from a blink launch process perspective.
On 2016/06/29 18:18:07, Rick Byers wrote: > On 2016/06/29 18:14:12, wolenetz wrote: > > On 2016/06/29 14:25:45, Rick Byers wrote: > > > Intent is now approved. > > > webexposed/ LGTM > > > > Thanks for the review. > > > > Question on procedure: The intent received 3 API owner approvals. Do I need to > > also await 3 API owner l*tms on this CL? (my c#17 assumed that, though > rereading > > launch-process seems to imply that "3" applies to the intent and not the CL.) > > No that would be crazy :-). The stable/webexposed/ OWNER check is just a > sanity-check to help ensure the intent approval has happened. You're all good > to land this from a blink launch process perspective. Thanks for clarifying :) I'll move tkent@ and jochen@ to CC and CQ this.
Description was changed from ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,jochen@chromium.org,chcunningham@chromium.org BUG=623678 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org BUG=623678 ==========
wolenetz@chromium.org changed reviewers: - jochen@chromium.org, tkent@chromium.org
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1945523003/#ps20001 (title: "rebase")
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 commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
On 2016/06/29 18:34:33, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) > android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) > android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) > cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) > chromeos_amd64-generic_chromium_compile_only_ng on > master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) > ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) > mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) > mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) > win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) > win_clang on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...) Minor rebase needed. I'll get that into CQ shortly.
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org, rbyers@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1945523003/#ps40001 (title: "rebase")
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 ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org BUG=623678 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org BUG=623678 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org BUG=623678 ========== to ========== MSE: Add EventHandler attributes to MSE objects Per intent [3], and due to recent spec change [1] for spec issue [2], this change adds EventHandler attributes for MSE objects as follows: Attributes for MediaSource events: * onsourceopen * onsourceended * onsourceclose Attributes for SourceBuffer events: * onupdatestart * onupdate * onupdateend * onerror * onabort Attributes for SourceBufferList events: * onaddsourcebuffer * onremovesourcebuffer [1] https://github.com/w3c/media-source/pull/75 [2] https://github.com/w3c/media-source/issues/66 [3] https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/dM_8nlXvJIE R=mlamouri@chromium.org,rbyers@chromium.org,chcunningham@chromium.org BUG=623678 Committed: https://crrev.com/00003ed19f12e054784ee59b95c863b380361ef4 Cr-Commit-Position: refs/heads/master@{#402946} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/00003ed19f12e054784ee59b95c863b380361ef4 Cr-Commit-Position: refs/heads/master@{#402946} |
