|
|
Created:
5 years ago by mcasas Modified:
5 years ago Reviewers:
Peter Beverloo CC:
chromium-reviews, blink-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMediaRecorder: update to spec (1/3)
Inspired by CL: https://crrev.com/1490403003/ ;)
- TreatUndefinedAs=missing is going to be removed from the
spec [1] and is already not present in Gecko implementation [2]
- Also, "BlobEvent.data is non-nullable, so the dict argument
should be required, and the data argument should be required
and non-nullable." [3]
- Removing suffix 'Enum' from RecordingStateEnum [4].
- BlobEvent has a constructor now [5], so remove the TODO.
BUG=262211
[1] https://github.com/w3c/mediacapture-record/issues/7
[2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl
[3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXirBwAJ
[4] https://github.com/w3c/mediacapture-record/issues/8
[5] https://github.com/w3c/mediacapture-record/issues/11
Committed: https://crrev.com/918bd3b360632a246a14008876a6cedd56333469
Cr-Commit-Position: refs/heads/master@{#364518}
Patch Set 1 #Patch Set 2 : s/eventInit/eventInitDict/ #
Total comments: 8
Patch Set 3 : peter@s comments: add tests for BlobEvent ctor and reordered MediaRecorder.idl to match spec #
Messages
Total messages: 27 (19 generated)
Description was changed from ========== MediaRecorder idl itches: BlobEventInit needs a ctor param, remove TreatUndefinedAs=missing Inspired by CL: https://codereview.chromium.org/1490403003/ BUG=262211 Also relates to spec bugs https://github.com/w3c/mediacapture-record/issues/7 https://github.com/w3c/mediacapture-record/issues/11 ========== to ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] BUG=262211 Also relates to spec bugs [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://github.com/w3c/mediacapture-record/issues/11 ==========
Description was changed from ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] BUG=262211 Also relates to spec bugs [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://github.com/w3c/mediacapture-record/issues/11 ========== to ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://github.com/w3c/mediacapture-record/issues/11 ==========
Description was changed from ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://github.com/w3c/mediacapture-record/issues/11 ========== to ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... ==========
Description was changed from ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... ========== to ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ==========
The CQ bit was checked by mcasas@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/1497883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497883002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== MediaRecorder idl itches: remove TreatUndefinedAs=missing; BlobEventInit needs a ctor param Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ========== to ========== MediaRecorder update to spec: remove TreatUndefinedAs=missing; add ctor param to BlobEventInit Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ==========
Description was changed from ========== MediaRecorder update to spec: remove TreatUndefinedAs=missing; add ctor param to BlobEventInit Inspired by CL: https://crrev.com/1490403003/ ;) TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implentation [2] Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ========== to ========== MediaRecorder: update to spec 1 Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ==========
Description was changed from ========== MediaRecorder: update to spec 1 Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ========== to ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
mcasas@chromium.org changed reviewers: + peter@chromium.org
peter@ PTAL (minor changes)
Patchset #3 (id:80001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 ========== to ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. - BlobEvent has a constructor now [5], so remove the TODO. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 [5] https://github.com/w3c/mediacapture-record/issues/11 ==========
lgtm \o/ https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl (right): https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl:6: required Blob data; nit: it might be interesting to add a simple test to verify that this property and the BlobEvent constructor's arguments indeed are (and remain) required. https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl (right): https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:5: // https://w3c.github.io/mediacapture-record/MediaRecorder.html#MediaRecorderAPI Mm. The GitHub repository uses both the gh-pages branch and the master branch. Is it deliberately that they differ? AFAIK most specs only use gh-pages, since these are just the editor's drafts anyway. https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec... (master) https://w3c.github.io/mediacapture-record/MediaRecorder.html#MediaRecorderAPI (gh-pages) https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:12: // TODO(mcasas): consider changing |mimeType| to a dictionary with said key, see https://github.com/w3c/mediacapture-record/issues/19 I see that Philip also pointed this out, but since Gecko already does this per the issue it's probably fine to go ahead and match their implementation. Doesn't have to be in this CL of course :). https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:21: attribute boolean ignoreMutedMedia; micro nit: it's easier to confirm how well we match the specification's IDL if we match the declaration order. You could consider moving this attribute to come after the events.
https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl (right): https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/BlobEventInit.idl:6: required Blob data; On 2015/12/09 23:11:29, Peter Beverloo wrote: > nit: it might be interesting to add a simple test to verify that this property > and the BlobEvent constructor's arguments indeed are (and remain) required. Added them to LayoutTests's MediaRecorder-BlobEvent-basic.html https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl (right): https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:5: // https://w3c.github.io/mediacapture-record/MediaRecorder.html#MediaRecorderAPI On 2015/12/09 23:11:29, Peter Beverloo wrote: > Mm. The GitHub repository uses both the gh-pages branch and the master branch. > > Is it deliberately that they differ? AFAIK most specs only use gh-pages, since > these are just the editor's drafts anyway. > > https://rawgit.com/w3c/mediacapture-record/master/MediaRecorder.html#MediaRec... > (master) > https://w3c.github.io/mediacapture-record/MediaRecorder.html#MediaRecorderAPI > (gh-pages) I was also uncertain about it so I pinged the appropriate savvy fellas in the w3c and they said both approaches are reasonable, WebRtc and GetUserMedia specs being maintained that way master->gh-pages->w3c... Personally just wanted to have a way of bundling a few minor corrections before publishing to gh-pages and then to w3c. I'm too used to having several channels I guess :) Moving on, I guess I'll it's reasonable to have both much closer. https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:12: // TODO(mcasas): consider changing |mimeType| to a dictionary with said key, see https://github.com/w3c/mediacapture-record/issues/19 On 2015/12/09 23:11:29, Peter Beverloo wrote: > I see that Philip also pointed this out, but since Gecko already does this per > the issue it's probably fine to go ahead and match their implementation. > > Doesn't have to be in this CL of course :). I agree and is being done here: http://crrev.com/1507183002/ ;) (which will come your way soon) https://codereview.chromium.org/1497883002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediarecorder/MediaRecorder.idl:21: attribute boolean ignoreMutedMedia; On 2015/12/09 23:11:29, Peter Beverloo wrote: > micro nit: it's easier to confirm how well we match the specification's IDL if > we match the declaration order. You could consider moving this attribute to come > after the events. Done.
The CQ bit was checked by mcasas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org Link to the patchset: https://codereview.chromium.org/1497883002/#ps120001 (title: "peter@s comments: add tests for BlobEvent ctor and reordered MediaRecorder.idl to match spec")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1497883002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497883002/120001
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. - BlobEvent has a constructor now [5], so remove the TODO. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 [5] https://github.com/w3c/mediacapture-record/issues/11 ========== to ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. - BlobEvent has a constructor now [5], so remove the TODO. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 [5] https://github.com/w3c/mediacapture-record/issues/11 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. - BlobEvent has a constructor now [5], so remove the TODO. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 [5] https://github.com/w3c/mediacapture-record/issues/11 ========== to ========== MediaRecorder: update to spec (1/3) Inspired by CL: https://crrev.com/1490403003/ ;) - TreatUndefinedAs=missing is going to be removed from the spec [1] and is already not present in Gecko implementation [2] - Also, "BlobEvent.data is non-nullable, so the dict argument should be required, and the data argument should be required and non-nullable." [3] - Removing suffix 'Enum' from RecordingStateEnum [4]. - BlobEvent has a constructor now [5], so remove the TODO. BUG=262211 [1] https://github.com/w3c/mediacapture-record/issues/7 [2] https://github.com/mozilla/gecko-dev/blob/master/dom/webidl/MediaRecorder.webidl [3] https://groups.google.com/a/chromium.org/d/msg/blink-dev/76HB0BIxk_o/9ZK0yXir... [4] https://github.com/w3c/mediacapture-record/issues/8 [5] https://github.com/w3c/mediacapture-record/issues/11 Committed: https://crrev.com/918bd3b360632a246a14008876a6cedd56333469 Cr-Commit-Position: refs/heads/master@{#364518} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/918bd3b360632a246a14008876a6cedd56333469 Cr-Commit-Position: refs/heads/master@{#364518} |