Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1255873002:
MediaRecorder Blink part (Closed)
|
Created:
5 years, 5 months ago by mcasas Modified:
5 years, 3 months ago CC:
blink-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, tommyw+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionMediaRecorder Blink part
This CL adds the Blink classes needed for implementing
MediaRecorder API and two tests.This API allows for
recording a MediaStream, i.e. a local or remote feed of
live media(s). This CL does not include a BlobEvent to
communicate recorded data to JS.
See DD @ https://goo.gl/vSjzC5 (*) for the plan and
https://codereview.chromium.org/1211973012/ for a
hack of all Chrome parts.
(*) Used to be https://goo.gl/kreaQj
BUG=262211
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201727
Patch Set 1 : perkj@s original patch http://crrev.com/596653005 #Patch Set 2 : #
Total comments: 42
Patch Set 3 : peter@ and guidou@ comments #
Total comments: 6
Patch Set 4 : tommi@s comments. Added dummy Source/platform/exported/WebMediaRecorderHandlerClient.cpp #
Total comments: 30
Patch Set 5 : esprehn@ comments #
Total comments: 30
Patch Set 6 : mlamouri@ comments #
Total comments: 2
Patch Set 7 : 4 spaces indentation and rebased. #
Total comments: 42
Patch Set 8 : esprehn@ comments #
Total comments: 12
Patch Set 9 : peter@ comments. Removed BlobEvent, moved MediaRecorder* to Source/modules/mediarecorder #
Total comments: 30
Patch Set 10 : peter@ comments and nits #
Total comments: 8
Patch Set 11 : peter@ nits and tried removing WebMediaRecorderHandlerClient.cpp #
Total comments: 6
Patch Set 12 : added peter@ to mediarecorder/OWNERS #Patch Set 13 : esprehn@ last comments #Messages
Total messages: 73 (35 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
Patchset #2 (id:200001) has been deleted
Patchset #2 (id:220001) has been deleted
Patchset #2 (id:120002) has been deleted
mcasas@chromium.org changed reviewers: + guidou@chromium.org
guidou@ PTAL
https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:16: shouldBe('MediaRecorder.canRecordMimeType("video/webm;codecs="vp8")', '"maybe"'); You should fix the syntax error here https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:19: shouldBe('MediaRecorder.canRecordMimeType("audio/webm;codecs="opus"")', '"maybe"'); and here
guidou@chromium.org changed reviewers: + peter@chromium.org
Hi Peter, can you take a look at this CL?
https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> nit: We generally use <!DOCTYPE HTML> in new layout tests. Ideally you would convert these to testharness.js tests, so that you don't need the -expected files. (Then again, because they're expected to fail right now you'd still need to add them.) https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:21: static PassRefPtrWillBeRawPtr<BlobEvent> create(); nit: Given that they're trivial, you could inline the bodies of these methods. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:5: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#blob-event nit: https:// https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:71: "No MediaRecorder handler can be created."); I find this more often in media code, but if I were a Web Developer and I'd get the message "No MediaRecorder handler can be created" that wouldn't be very helpful to me. For now, we don't support this because the Chromium-side is not here. However, more specifically to line 77, when would that ever be a valid situation to be in? Should we ASSERT() on successful initialization instead? (Once Chromium implements createMediaRecorderHandler(), I think we should also ASSERT() on m_recordedHandler.) https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:169: RawPtr<WebMediaRecorderHandler> handler = Platform::current()->createMediaRecorderHandler(); This is a bit unfortunate. The embedder could end up creating MediaRecordedHandler for two reasons - actually using one, or checking whether a mime type could be supported. It would simplify your code if you add two APIs to Platform.h instead. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:173: return handler->canSupportMimeType(mimeType) ? "maybe" : ""; This is a tri-state: (1) The empty string if the UA knows it can't record |mimeType|. (2) "probably" if it's confident it can record |mimeType|. (3) "maybe" if it's not certain whether it can record |mimeType|. Is it deliberate that we don't want to support "probably"? That should be documented in the APIs header in that case. (Reading a "bool canSupport()" function returning TRUE doesn't strike me as meaning "maybe".) https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:269: createBlobEvent(BlobData::create()); Is this correct? Suppose that you have this code: =============== var recorder = new MediaRecorder(stream); recorder.start(); // ... ondataavailable gets invoked, stream is still active. recorder.stop() // ... ondataavailable gets invoked with an empty Blob,. =============== When calling .stop(), the spec says that it should fire dataavailable "containing the Blob of data that has been gathered". It's left ambiguous whether that entails *all* data (the linguistic interpretation), or just the data since the last event (in which case this may be correct). https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:31: static MediaRecorder* create(ExecutionContext*, MediaStream*, const String&, ExceptionState&); What is "const String&"? We do use argument names in Blink if the meaning can't be read from the type. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:50: void start(int&, ExceptionState&); dito re: argument meaning, for int& https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:56: static String canRecordMimeType(const String&); dito re: argument meaning, for String& https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:88: String m_state; I'm not a huge fan of using a String to maintain the state (even AtomicString). Why not use an enum instead, and have some stateToString() method when the textual representation is necessary? https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:29: DECLARE_VIRTUAL_TRACE(); nit: Since it only traces the parent class, you could inline it instead: DEFINE_INLINE_VIRTUAL_TRACE() { Event::trace(visitor); } https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:34: bool cancelable, const String& name, const String& message); nit: Either don't break lines, or do it consistently (per Chromium style). https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:6: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#idl-def-Recording... nit: https https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:8: enum RecordingErrorNameEnum { Huh? I find this odd API design. Why doesn't the specification drop all of 6.2-6.4 and use actual DOMExceptions instead? (I realize that you may not have the answer.) https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:20: readonly attribute DOMString? message; nit: four space indent. https://codereview.chromium.org/1255873002/diff/250001/public/platform/Platfo... File public/platform/Platform.h (right): https://codereview.chromium.org/1255873002/diff/250001/public/platform/Platfo... public/platform/Platform.h:86: class WebMediaRecorderHandlerClient; Should the client be passed in createMediaRecorderHandler()? https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:16: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandler { nit: ++comments please :) https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:22: const WebString& mimeType) = 0; nit: Please do use argument names in the public API. (Personally I'm fine with *always* using them.)
Patchset #3 (id:270001) has been deleted
peter@ PTAL https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: We generally use <!DOCTYPE HTML> in new layout tests. > > Ideally you would convert these to testharness.js tests, so that you don't need > the -expected files. (Then again, because they're expected to fail right now > you'd still need to add them.) > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-... Acknowledged. https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:16: shouldBe('MediaRecorder.canRecordMimeType("video/webm;codecs="vp8")', '"maybe"'); On 2015/07/28 12:28:26, Guido Urdaneta wrote: > You should fix the syntax error here canRecordMimeType() gets a String as parameter and returns a string, either an empty one ("") or a maybe ("maybe"). https://codereview.chromium.org/1255873002/diff/250001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:19: shouldBe('MediaRecorder.canRecordMimeType("audio/webm;codecs="opus"")', '"maybe"'); On 2015/07/28 12:28:26, Guido Urdaneta wrote: > and here Ditto. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:21: static PassRefPtrWillBeRawPtr<BlobEvent> create(); On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: Given that they're trivial, you could inline the bodies of these methods. Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:5: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#blob-event On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: https:// Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:71: "No MediaRecorder handler can be created."); On 2015/07/29 16:54:22, Peter Beverloo wrote: > I find this more often in media code, but if I were a Web Developer and I'd get > the message "No MediaRecorder handler can be created" that wouldn't be very > helpful to me. > > For now, we don't support this because the Chromium-side is not here. However, > more specifically to line 77, when would that ever be a valid situation to be > in? I can imagine an unsupported mimeType, f.i. recording in an unsupported video codec. For the experimental version, only VP8 is in the radar. > Should we ASSERT() on successful initialization instead? > > (Once Chromium implements createMediaRecorderHandler(), I think we should also > ASSERT() on m_recordedHandler.) Done. Added a TODO() to ASSERT() for it once the Cr parts are landed. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:169: RawPtr<WebMediaRecorderHandler> handler = Platform::current()->createMediaRecorderHandler(); On 2015/07/29 16:54:22, Peter Beverloo wrote: > This is a bit unfortunate. The embedder could end up creating > MediaRecordedHandler for two reasons - actually using one, or checking whether a > mime type could be supported. It would simplify your code if you add two APIs to > Platform.h instead. I know :( Is there any way to have a WebMediaStreamHandler static method? https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:173: return handler->canSupportMimeType(mimeType) ? "maybe" : ""; On 2015/07/29 16:54:22, Peter Beverloo wrote: > This is a tri-state: > > (1) The empty string if the UA knows it can't record |mimeType|. > (2) "probably" if it's confident it can record |mimeType|. > (3) "maybe" if it's not certain whether it can record |mimeType|. > > Is it deliberate that we don't want to support "probably"? That should be > documented in the APIs header in that case. > > (Reading a "bool canSupport()" function returning TRUE doesn't strike me as > meaning "maybe".) Yes but the same doc says: "Implementors are encouraged to return "maybe" unless the type can be confidently established as being supported or not." The encoder creation itself could fail for a number of reasons, so I believe "maybe" and "" are the only possible outcomes. I'll add some info everywhere. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:269: createBlobEvent(BlobData::create()); On 2015/07/29 16:54:22, Peter Beverloo wrote: > Is this correct? Suppose that you have this code: > > =============== > var recorder = new MediaRecorder(stream); > recorder.start(); > > // ... ondataavailable gets invoked, stream is still active. > > recorder.stop() > > // ... ondataavailable gets invoked with an empty Blob,. > > =============== > > When calling .stop(), the spec says that it should fire dataavailable > "containing the Blob of data that has been gathered". It's left ambiguous > whether that entails *all* data (the linguistic interpretation), or just the > data since the last event (in which case this may be correct). I presume it has to do with the way start() is called: - if a |timeSlice| is passed as an argument, we will ping back every so often and then, the last result is gathered and sent after stop() has been, asynchronously, invoked. The asynchronicity is the point here that justifies the after-stop-onDataAvailable(). - If, on the other hand, start() is called with no arguments, then the draft is less clear IMHO. It could equate |timeSlice| to infinite, in which case the JS might get a humongous Blob after stop() containing all the recorded stuff, or it might assume |timeSlice = 0|, so that onDataAvailable() is called everytime there's something to be stored. But in all cases, stop() is understood as stopdAndFlushRecordedData(). Makes sense? https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:31: static MediaRecorder* create(ExecutionContext*, MediaStream*, const String&, ExceptionState&); On 2015/07/29 16:54:22, Peter Beverloo wrote: > What is "const String&"? We do use argument names in Blink if the meaning can't > be read from the type. Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:50: void start(int&, ExceptionState&); On 2015/07/29 16:54:22, Peter Beverloo wrote: > dito re: argument meaning, for int& Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:56: static String canRecordMimeType(const String&); On 2015/07/29 16:54:22, Peter Beverloo wrote: > dito re: argument meaning, for String& Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:88: String m_state; On 2015/07/29 16:54:22, Peter Beverloo wrote: > I'm not a huge fan of using a String to maintain the state (even AtomicString). > Why not use an enum instead, and have some stateToString() method when the > textual representation is necessary? Done. I noticed that there's a textual String representation of the State in MediaRecorder.idl and another, otherwise unrelated, in MediaRecorder.cpp. Do you know any way to link them both? https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:29: DECLARE_VIRTUAL_TRACE(); On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: Since it only traces the parent class, you could inline it instead: > > DEFINE_INLINE_VIRTUAL_TRACE() { Event::trace(visitor); } Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:34: bool cancelable, const String& name, const String& message); On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: Either don't break lines, or do it consistently (per Chromium style). Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:6: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#idl-def-Recording... On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: https Done. https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:8: enum RecordingErrorNameEnum { On 2015/07/29 16:54:22, Peter Beverloo wrote: > Huh? I find this odd API design. > > Why doesn't the specification drop all of 6.2-6.4 and use actual DOMExceptions > instead? (I realize that you may not have the answer.) I don't know, the W3C spec is a bit old so it's not impossible that is outdated. Then again, this MediaRecorderErrorEvent was added after the original Draft was written. Should I add a TODO() and ping the W3C, or any other suggestions...? https://codereview.chromium.org/1255873002/diff/250001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:20: readonly attribute DOMString? message; On 2015/07/29 16:54:22, Peter Beverloo wrote: > nit: four space indent. Done. https://codereview.chromium.org/1255873002/diff/250001/public/platform/Platfo... File public/platform/Platform.h (right): https://codereview.chromium.org/1255873002/diff/250001/public/platform/Platfo... public/platform/Platform.h:86: class WebMediaRecorderHandlerClient; On 2015/07/29 16:54:23, Peter Beverloo wrote: > Should the client be passed in createMediaRecorderHandler()? The current M.O. is: WebMediaRecorderHandler* handler = createMediaRecorderHandler(); handler->initialize( client*, ...); (called in two places in MediaRecorder.cpp) But it could perfectly be: WebMediaRecorderHandler* handler = createMediaRecorderHandler(client*); handler->initialize(...); Any preference? :) https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:16: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandler { On 2015/07/29 16:54:23, Peter Beverloo wrote: > nit: ++comments please :) Done. Classes in public/platform are seldom commented :? https://codereview.chromium.org/1255873002/diff/250001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:22: const WebString& mimeType) = 0; On 2015/07/29 16:54:23, Peter Beverloo wrote: > nit: Please do use argument names in the public API. > > (Personally I'm fine with *always* using them.) Done.
peter@ ping
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
Peter is on holidays, you might want to find another reviewer.
mcasas@chromium.org changed reviewers: + philipj@opera.com
philipj@ PTAL Mounir: thanks!
On 2015/08/11 07:09:43, mcasas (slow to review) wrote: > philipj@ PTAL I'm afraid I don't have much time right now, may I suggest getting review from the Source/modules/ owners and then asking for a rubberstamp of the rest once it's in order?
tommi@chromium.org changed reviewers: + tommi@chromium.org
looks generally good but I'll wait for Peter's comments https://codereview.chromium.org/1255873002/diff/290001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/290001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:21: { nit: consistent { https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:31: Blob* data() const { return m_blob.get(); } can we return const Blob* or make the method non-const? (returning a non const ptr from a const method is generally avoided) https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:42: String mimeType() const { return m_mimeType; } const String& ?
https://codereview.chromium.org/1255873002/diff/290001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/290001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:21: { On 2015/08/11 08:46:50, tommi wrote: > nit: consistent { Done. https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:31: Blob* data() const { return m_blob.get(); } On 2015/08/11 08:46:50, tommi wrote: > can we return const Blob* or make the method non-const? (returning a non const > ptr from a const method is generally avoided) Done. https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/290001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:42: String mimeType() const { return m_mimeType; } On 2015/08/11 08:46:50, tommi wrote: > const String& ? Done.
Patchset #5 (id:330001) has been deleted
Patchset #4 (id:310001) has been deleted
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Can you write a better description, linking to a goo.gl link is not enough. What does this feature do? What is implemented in this patch? https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:3: <head> leave out html, head and body. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:8: <div id="console"></div> it creates these for you, leave out description and console. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:58: window.jsTestIsAsync = true; Move to top. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:59: window.successfullyParsed = true; Don't set this yourself, it should do it for you. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:3: <head> ditto for the test https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:81: String MediaRecorder::state() const { braces go on the next line https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:91: m_state = MediaRecorderState::Recording; You don't need the prefix since the enum is nested I think. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:171: // [1] http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods We should get probably removed from the spec if we never plan to return it. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:242: // blobData (Move-only) -> BlobDataHandle (ref-cnt) -> Blob remove comment https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:244: RefPtr<BlobDataHandle> blob_data_handle = blobDataHandle, no _ names in blink. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:30: enum class MediaRecorderState { rename to just State, no reason to add the class name again. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:56: void start(int& timeSlice, ExceptionState&); This shouldn't be a reference to an int. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:83: MediaRecorder(ExecutionContext*, MediaStream*, const String&, ExceptionState&); add argument name for the string. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:89: void dispatchScheduledEvent(); I think you want an EventSender instead of this. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:94: bool m_ignoreMutedMedia; combine bools so this packs better.
esprehn@ PTAL https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:3: <head> On 2015/08/12 22:27:34, esprehn wrote: > leave out html, head and body. Done. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:8: <div id="console"></div> On 2015/08/12 22:27:34, esprehn wrote: > it creates these for you, leave out description and console. Done. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:58: window.jsTestIsAsync = true; On 2015/08/12 22:27:34, esprehn wrote: > Move to top. Done. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:59: window.successfullyParsed = true; On 2015/08/12 22:27:34, esprehn wrote: > Don't set this yourself, it should do it for you. Done. https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/350001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:3: <head> On 2015/08/12 22:27:34, esprehn wrote: > ditto for the test Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:81: String MediaRecorder::state() const { On 2015/08/12 22:27:34, esprehn wrote: > braces go on the next line Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:91: m_state = MediaRecorderState::Recording; On 2015/08/12 22:27:34, esprehn wrote: > You don't need the prefix since the enum is nested I think. I do because State is an enum class. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:171: // [1] http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods On 2015/08/12 22:27:34, esprehn wrote: > We should get probably removed from the spec if we never plan to return it. I agree, this is a source of confusion, and might potentially waste JS devs time by having them consider "probably" as a potential return value... https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:242: // blobData (Move-only) -> BlobDataHandle (ref-cnt) -> Blob On 2015/08/12 22:27:34, esprehn wrote: > remove comment Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:244: RefPtr<BlobDataHandle> blob_data_handle = On 2015/08/12 22:27:34, esprehn wrote: > blobDataHandle, no _ names in blink. Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:30: enum class MediaRecorderState { On 2015/08/12 22:27:34, esprehn wrote: > rename to just State, no reason to add the class name again. Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:56: void start(int& timeSlice, ExceptionState&); On 2015/08/12 22:27:34, esprehn wrote: > This shouldn't be a reference to an int. Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:83: MediaRecorder(ExecutionContext*, MediaStream*, const String&, ExceptionState&); On 2015/08/12 22:27:34, esprehn wrote: > add argument name for the string. Done. https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:89: void dispatchScheduledEvent(); On 2015/08/12 22:27:34, esprehn wrote: > I think you want an EventSender instead of this. I'm not sure I understand how that class would be used here. Do you mean removing these two methods in l.88-89 and the associated members in l.100-101 and instead using an internal EventSender? https://codereview.chromium.org/1255873002/diff/350001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.h:94: bool m_ignoreMutedMedia; On 2015/08/12 22:27:34, esprehn wrote: > combine bools so this packs better. Done.
I didn't look into the details of the MediaRecorder implementation but left a few comments. https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <script src="../../resources/js-test.js"></script> Could you use testharness so we can share the tests? https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:1: <script src="../../resources/js-test.js"></script> ditto https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:25: } Please, do not inline methods except getters/setters. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:29: } ditto https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:34: virtual const AtomicString& interfaceName() const final { return EventNames::BlobEvent; } ditto https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:42: , m_blob(blob) { } ditto https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:21: return adoptRefWillBeNoop(new MediaRecorderErrorEvent); same as BlobEvent regarding inline methods https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:20: readonly attribute DOMString? message; That's really odd. Could that event carry a DOMException instead of creating something so close to a DOMException? https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/WindowMediaStream.idl (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/WindowMediaStream.idl:9: [RuntimeEnabled=MediaRecorder] attribute MediaRecorderConstructor webkitMediaRecorder; Why do we add prefixed methods? https://codereview.chromium.org/1255873002/diff/370001/public/platform/Platfo... File public/platform/Platform.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/Platfo... public/platform/Platform.h:86: class WebMediaRecorderHandlerClient; nit: you don't seem to need to fwd declare the client here. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:19: virtual ~WebMediaRecorderHandler() { } = default; instead of {} should do. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:24: virtual bool start(int timeslice) = 0; I think the Chromium/Blink style guide do not recommend method override. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:40: #endif nit: empty line https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:24: virtual ~WebMediaRecorderHandlerClient() { } nit: you can use = default; https://codereview.chromium.org/1255873002/diff/370001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:91: BLINK_EXPORT static void enableMediaRecorder(bool); Do you really need that?
Patchset #6 (id:390001) has been deleted
PTAL https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <script src="../../resources/js-test.js"></script> On 2015/08/13 13:31:47, Mounir Lamouri wrote: > Could you use testharness so we can share the tests? Done. https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/370001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:1: <script src="../../resources/js-test.js"></script> On 2015/08/13 13:31:47, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.h (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:25: } On 2015/08/13 13:31:47, Mounir Lamouri wrote: > Please, do not inline methods except getters/setters. Done. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:29: } On 2015/08/13 13:31:47, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:34: virtual const AtomicString& interfaceName() const final { return EventNames::BlobEvent; } On 2015/08/13 13:31:47, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.h:42: , m_blob(blob) { } On 2015/08/13 13:31:47, Mounir Lamouri wrote: > ditto Done. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.h:21: return adoptRefWillBeNoop(new MediaRecorderErrorEvent); On 2015/08/13 13:31:47, Mounir Lamouri wrote: > same as BlobEvent regarding inline methods Done. And also a few on MediaRecorder. (I was over-zealous to remove MediaRecorderErrorEvent.cpp). https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:20: readonly attribute DOMString? message; On 2015/08/13 13:31:47, Mounir Lamouri wrote: > That's really odd. Could that event carry a DOMException instead of creating > something so close to a DOMException? You tell me :) This is just a verbatim copy of the W3C doco in l.5. https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... File Source/modules/mediastream/WindowMediaStream.idl (right): https://codereview.chromium.org/1255873002/diff/370001/Source/modules/mediast... Source/modules/mediastream/WindowMediaStream.idl:9: [RuntimeEnabled=MediaRecorder] attribute MediaRecorderConstructor webkitMediaRecorder; On 2015/08/13 13:31:47, Mounir Lamouri wrote: > Why do we add prefixed methods? Hmmm I guess this was a legacy of the webkitGetUserMedia() times. I don't think is needed very much, so I'm removing it, thanks. https://codereview.chromium.org/1255873002/diff/370001/public/platform/Platfo... File public/platform/Platform.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/Platfo... public/platform/Platform.h:86: class WebMediaRecorderHandlerClient; On 2015/08/13 13:31:48, Mounir Lamouri wrote: > nit: you don't seem to need to fwd declare the client here. Done. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:19: virtual ~WebMediaRecorderHandler() { } On 2015/08/13 13:31:48, Mounir Lamouri wrote: > = default; instead of {} should do. Done. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:24: virtual bool start(int timeslice) = 0; On 2015/08/13 13:31:48, Mounir Lamouri wrote: > I think the Chromium/Blink style guide do not recommend method override. Chromium is mildly against them [1] and in this case is needed to implement the W3C API... [1] http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Over... https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:40: #endif On 2015/08/13 13:31:48, Mounir Lamouri wrote: > nit: empty line Done. https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:24: virtual ~WebMediaRecorderHandlerClient() { } On 2015/08/13 13:31:48, Mounir Lamouri wrote: > nit: you can use = default; Done. https://codereview.chromium.org/1255873002/diff/370001/public/web/WebRuntimeF... File public/web/WebRuntimeFeatures.h (right): https://codereview.chromium.org/1255873002/diff/370001/public/web/WebRuntimeF... public/web/WebRuntimeFeatures.h:91: BLINK_EXPORT static void enableMediaRecorder(bool); On 2015/08/13 13:31:48, Mounir Lamouri wrote: > Do you really need that? I wouldn't know how to answer but seeing your surprise I'd say I don't -- what's the logic behind this enables? I see at least one "stable" that is in this list, i.e. MediaSource.
https://codereview.chromium.org/1255873002/diff/410001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/410001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:188: m_dispatchScheduledEventRunner.suspend(); 4-space indentation?
Looking better! I'll make sure to look this over tomorrow. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Patchset #7 (id:420001) has been deleted
On 2015/08/14 08:56:37, esprehn wrote: > Looking better! I'll make sure to look this over tomorrow. > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:blink-reviews+unsubscribe@chromium.org. ping guys :)
ping, anyone? https://codereview.chromium.org/1255873002/diff/410001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/410001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:188: m_dispatchScheduledEventRunner.suspend(); On 2015/08/14 08:33:01, Guido Urdaneta wrote: > 4-space indentation? Done.
I'll make sure to look this over today. Large patches in general take a long time (this one is hundreds of lines). In the future doing patches in steps means you get reviews faster.
Your Google doc is not public, it needs to be public before this can land. Also this spec seems like it needs a bunch of work. Please don't invent your own error events like this, use DOMException. Also the event model seems like it needs better specing and some review. Normally you post tasks to fire events (your code does this), but you don't abort the events if you stop/cancel usually, but maybe this spec is special? Also it seems weird that the stop method doesn't fire a stop event. That usually leads to buggy author code and clumsy workarounds. If you need help with that talk to domenic@ and slightlyoff@. https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt:2: FAIL checks the video-only MediaRecorder API. assert_unreached: Exception while creating MediaRecorder: NotSupportedError: Failed to construct 'MediaRecorder': No MediaRecorder handler can be created. Reached unreachable code seems your tests don't work? https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <script src=../../resources/testharness.js></script> <!DOCTYPE html> always use standards mode https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:3: FAIL check MediaRecorder.canRecordMimeType() with video/webm and vp8 assert_equals: expected "maybe" but got "" fail? https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:5: FAIL check MediaRecorder.canRecordMimeType() with audio/webm and opus assert_equals: expected "maybe" but got "" fail? https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:1: <script src=../../resources/testharness.js></script> <!DOCTYPE html> https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:10: interface BlobEvent : Event { This is a very generic event name. Is this going to get reused by other specs? https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:21: static String StateToString(MediaRecorder::State state) stateToString(), functions start with lowercase names. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:58: , m_stopped(false) true https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:69: m_stopped = true; delete both of these https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:75: m_stopped = true; remove this https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:79: } m_stopped = false https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:164: return ""; emptyString() https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:173: return handler->canSupportMimeType(mimeType) ? "maybe" : ""; "maybe" : emptyString(); https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:205: m_dispatchScheduledEventRunner.stop(); This should fire a stop event, the spec should be fixed. Not firing a stop event when you manually call stop() means authors end up writing silly workarounds. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:259: void MediaRecorder::doStop() Having both stop and doStop() is confusing, what does this one actually do? Can you give this a better name? https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:280: if (m_stopped) Hmm, this means if you call stop() before one of the other events fires that you'll never get them even if that event has happened. That doesn't seem right, that's not usually how web specs work either. When a thing happens you queue a task to fire an event, if you call stop() before that you don't cancel the event. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:12: Constructor (MediaStream stream,[TreatUndefinedAs=Missing] optional DOMString mimeType), space after , https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:27: attribute boolean ignoreMutedMedia; new line between this property and the event handlers. Actually move this up with the properties above (ex. state) https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:29: [RaisesException] void start (optional long timeslice); remove space between name and argument list https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:34: static DOMString canRecordMimeType (DOMString mimeType); remove space between name and argument list https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:19: readonly attribute RecordingErrorNameEnum name; Having a name property like this is very weird, I think you want to just fire a DOMException and reuse the existing types for that. You're recreating that same thing here, we already have InvalidState and SecurityError types. If you need more error types please have them added for the DOMException. https://codereview.chromium.org/1255873002/diff/440001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/440001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:23: virtual bool start() = 0; I usually suggest not making these pure virtual and adding default methods instead. It means adding a new method is simpler, otherwise you need add the method here as a non-pure virtual, then add it in Chromium, then switch this back to pure virtual. (3 patches)
esprehn@ thanks for the review, PTAL. I moved the DD to my chromium.org account, so it should be publicly accessible and comment-able (will ping blink-dev@ later today). About the DOMException, I'm happy to change it to whatever is blink customary, I just copied the W3C [1]. Modified the stop() event launching. Also, I can divide the CL in smaller ones if that'd ease the review, I kept it in one since, apart from MediaRecorder.cpp, the files have very little entity, and even that one is basically a billboard of blob/event launching + forwarding and internal state accounting. [1] https://w3c.github.io/mediacapture-record/MediaRecorder.html#MediaRecorderErr... https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt:2: FAIL checks the video-only MediaRecorder API. assert_unreached: Exception while creating MediaRecorder: NotSupportedError: Failed to construct 'MediaRecorder': No MediaRecorder handler can be created. Reached unreachable code On 2015/08/21 08:46:12, esprehn wrote: > seems your tests don't work? I might be understanding things the wrong way: Platform::current()->createMediaRecorderHandler() returns nullptr right now, but that's because there's no Chromium part implemented yet. So this creation would and should fail. When the Cr part is landed, these expectations should be updated. Right? Or should I provide a mock for it, and where? https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:1: <script src=../../resources/testharness.js></script> On 2015/08/21 08:46:12, esprehn wrote: > <!DOCTYPE html> > > always use standards mode Done. https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/440001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:1: <script src=../../resources/testharness.js></script> On 2015/08/21 08:46:12, esprehn wrote: > <!DOCTYPE html> Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:10: interface BlobEvent : Event { On 2015/08/21 08:46:12, esprehn wrote: > This is a very generic event name. Is this going to get reused by other specs? I'm just following the draft API, but AFAIK is used nowhere else. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:21: static String StateToString(MediaRecorder::State state) On 2015/08/21 08:46:13, esprehn wrote: > stateToString(), functions start with lowercase names. Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:58: , m_stopped(false) On 2015/08/21 08:46:13, esprehn wrote: > true Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:69: m_stopped = true; On 2015/08/21 08:46:13, esprehn wrote: > delete both of these Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:75: m_stopped = true; On 2015/08/21 08:46:13, esprehn wrote: > remove this Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:79: } On 2015/08/21 08:46:13, esprehn wrote: > m_stopped = false Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:164: return ""; On 2015/08/21 08:46:13, esprehn wrote: > emptyString() Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:173: return handler->canSupportMimeType(mimeType) ? "maybe" : ""; On 2015/08/21 08:46:12, esprehn wrote: > "maybe" : emptyString(); Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:205: m_dispatchScheduledEventRunner.stop(); On 2015/08/21 08:46:13, esprehn wrote: > This should fire a stop event, the spec should be fixed. Not firing a stop event > when you manually call stop() means authors end up writing silly workarounds. Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:259: void MediaRecorder::doStop() On 2015/08/21 08:46:12, esprehn wrote: > Having both stop and doStop() is confusing, what does this one actually do? Can > you give this a better name? doStop() is supposed to do the bottom half of stopping the recording on MediaRecorder::stop() or error condition. Renamed, let's see. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.cpp:280: if (m_stopped) On 2015/08/21 08:46:13, esprehn wrote: > Hmm, this means if you call stop() before one of the other events fires that > you'll never get them even if that event has happened. That doesn't seem right, > that's not usually how web specs work either. When a thing happens you queue a > task to fire an event, if you call stop() before that you don't cancel the > event. Removed, so even if it's stop()ed we'll fwd events. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorder.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:12: Constructor (MediaStream stream,[TreatUndefinedAs=Missing] optional DOMString mimeType), On 2015/08/21 08:46:13, esprehn wrote: > space after , Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:27: attribute boolean ignoreMutedMedia; On 2015/08/21 08:46:13, esprehn wrote: > new line between this property and the event handlers. Actually move this up > with the properties above (ex. state) Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:29: [RaisesException] void start (optional long timeslice); On 2015/08/21 08:46:13, esprehn wrote: > remove space between name and argument list Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorder.idl:34: static DOMString canRecordMimeType (DOMString mimeType); On 2015/08/21 08:46:13, esprehn wrote: > remove space between name and argument list Done. https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/440001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:19: readonly attribute RecordingErrorNameEnum name; On 2015/08/21 08:46:13, esprehn wrote: > Having a name property like this is very weird, I think you want to just fire a > DOMException and reuse the existing types for that. You're recreating that same > thing here, we already have InvalidState and SecurityError types. If you need > more error types please have them added for the DOMException. Seems like MediaRecordingError raises eyebrows in everyone reading it :) but I just took it verbatim from the linked W3C draft. Please advise, scrap it and reuse or comply? https://codereview.chromium.org/1255873002/diff/440001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/440001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:23: virtual bool start() = 0; On 2015/08/21 08:46:13, esprehn wrote: > I usually suggest not making these pure virtual and adding default methods > instead. It means adding a new method is simpler, otherwise you need add the > method here as a non-pure virtual, then add it in Chromium, then switch this > back to pure virtual. (3 patches) Done inlined, please instruct otherwise.
reviewers friendly ping.
Sorry for the delays - I'm back from holiday now. I'll take a more detailed look this weekend. In general, it'd be grand if the patch could be significantly smaller. It's not easy to review. https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.cpp (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.cpp:36: , m_blob(blob) { } micro nit: {} on their own lines. https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:8: NoInterfaceObject The spec defines this as having a constructor and having an interface object (which means whether it's exposed on the window global). Perhaps split out the BlobEvent(Init) in a different CL, since that can be landed stand-alone of the rest? https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/MediaRecorderErrorEvent.idl:17: NoInterfaceObject Would you happen to know why the spec defines this to be NoInterfaceObject? Generally that's strongly discouraged. https://codereview.chromium.org/1255873002/diff/460001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1255873002/diff/460001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:95: MediaRecorder status=experimental Does this do anything yet? We generally prefer setting status=experimental when the feature is largely working. (You could use status=test so that it continues to be available in layout tests.) https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:20: virtual bool initialize(WebMediaRecorderHandlerClient* client, nit: no wrapping. (Blink style) https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:40: #endif micro nit: // WebMediaRecorderHandler_h, + blank line above
Patchset #9 (id:480001) has been deleted
Patchset #9 (id:500001) has been deleted
Removed BlobEvent to ease reviewing. Moved MediaRecorder* to its own folder Source/modules/mediarecorder since it's an API on its own. https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.cpp (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.cpp:36: , m_blob(blob) { } On 2015/08/28 17:18:24, Peter Beverloo wrote: > micro nit: {} on their own lines. Done. https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:8: NoInterfaceObject On 2015/08/28 17:18:24, Peter Beverloo wrote: > The spec defines this as having a constructor and having an interface object > (which means whether it's exposed on the window global). > > Perhaps split out the BlobEvent(Init) in a different CL, since that can be > landed stand-alone of the rest? Ok, following the suggestions I'll leave BlobEvent out of this CL. In my defense I'd say that I wanted to land chunks at a time and was instructed to try and land full functionalities ;) https://codereview.chromium.org/1255873002/diff/460001/Source/platform/Runtim... File Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1255873002/diff/460001/Source/platform/Runtim... Source/platform/RuntimeEnabledFeatures.in:95: MediaRecorder status=experimental On 2015/08/28 17:18:24, Peter Beverloo wrote: > Does this do anything yet? We generally prefer setting status=experimental when > the feature is largely working. (You could use status=test so that it continues > to be available in layout tests.) Done. https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:20: virtual bool initialize(WebMediaRecorderHandlerClient* client, On 2015/08/28 17:18:24, Peter Beverloo wrote: > nit: no wrapping. (Blink style) Done. https://codereview.chromium.org/1255873002/diff/460001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:40: #endif On 2015/08/28 17:18:24, Peter Beverloo wrote: > micro nit: // WebMediaRecorderHandler_h, + blank line above Done and in WebMediaRecorderHandlerClient.h as well.
For the changes I asked that don't match the spec, have you made bugs against the spec?
Patchset #9 (id:520001) has been deleted
Filed four W3C bugs: Bug 29102 - MediaStream Recording API: remove RecordingErrorNameEnum and use DomException instead. Bug 29103 - MediaStream Recording API: MediaRecorderErrorEvent is a NoInterfaceObject, consider removing Bug 29104 - BlobEvent is defined in both MediaStream Recorder and MediaStream ImageCapture, consider factoring them out. Bug 29105 - MediaStream Recording API: Consider not using BlobEvent, using Blob and FileReader instead
On 2015/08/31 19:56:57, mcasas wrote: > Filed four W3C bugs: > > Bug 29102 - MediaStream Recording API: remove RecordingErrorNameEnum and use > DomException instead. > Bug 29103 - MediaStream Recording API: MediaRecorderErrorEvent is a > NoInterfaceObject, consider removing > Bug 29104 - BlobEvent is defined in both MediaStream Recorder and MediaStream > ImageCapture, consider factoring them out. > Bug 29105 - MediaStream Recording API: Consider not using BlobEvent, using Blob > and FileReader instead Was instructed to file them in https://github.com/w3c/mediacapture-record/issues/created_by/miguelao
Hi Miguel - thanks for the update, and thank you for filing these issues. I think we're nearly there, and left mostly nits. I'm OK landing this as-is while the spec issues get resolved, but we should resolve them prior to shipping. https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... File Source/modules/mediastream/BlobEvent.idl (right): https://codereview.chromium.org/1255873002/diff/460001/Source/modules/mediast... Source/modules/mediastream/BlobEvent.idl:8: NoInterfaceObject On 2015/08/29 01:12:57, mcasas wrote: > On 2015/08/28 17:18:24, Peter Beverloo wrote: > > The spec defines this as having a constructor and having an interface object > > (which means whether it's exposed on the window global). > > > > Perhaps split out the BlobEvent(Init) in a different CL, since that can be > > landed stand-alone of the rest? > > Ok, following the suggestions I'll leave BlobEvent out of this CL. > > In my defense I'd say that I wanted to land chunks at a time and > was instructed to try and land full functionalities ;) I think experience has taught us that small CLs get reviewed *much* more quickly as it's easier for reviewers to put them between other tasks :-). I certainly prefer them. https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:18: recorder = null Do we need this line if it's followed immediately by an assert_unreached()? (Thus failing the test.) If so, add a semi-colon. ASI is hard. https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:7: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods micro nit: https https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:59: , m_ignoreMutedMedia(true) We currently don't use this property, and only reflect it in JavaScript, right? Will we support it eventually? It would be good to add a TODO to make sure we don't forget about this. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:74: exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native MediaRecorder."); The specification states the following: "If the UA does not support the format or any of the parameters specified, it must raise an UnsupportedOption Exception." Putting aside the UnsupportedOption bit (I replied to one of your GitHub issues), throwing a NotSupportedError in that case seems good. I would, however, like to see a clearer error message, so that developers know what's going on. Perhaps copy the MediaSource message? ("The type provided (..m_mimeType..) is unsupported.") https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:223: scheduleDispatchEvent(MediaRecorderErrorEvent::create( nit: note that you don't have to wrap the line. I don't mind too much either way. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.h:52: bool ignoreMutedMedia() const { return m_ignoreMutedMedia; } nit: maybe move these to line 44? That way the header file order matches the IDL. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.cpp:31: , m_message(message) { } micro nit: {} on their own lines. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.h:15: class MODULES_EXPORT MediaRecorderErrorEvent final : public Event { Perhaps add a TODO for the GitHub issue, given that this event could go away in its entirety if the specification would switch to DOMException? https://github.com/w3c/mediacapture-record/issues/14 https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.idl:8: enum RecordingErrorNameEnum { dito: Perhaps add a TODO? I'm OK with landing as-is, but this is something that would be very good to resolve prior to shipping. https://codereview.chromium.org/1255873002/diff/540001/Source/platform/export... File Source/platform/exported/WebMediaRecorderHandlerClient.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/platform/export... Source/platform/exported/WebMediaRecorderHandlerClient.cpp:10: // constructor/destructor and should be in Source/platform/exported. The compile logs aren't available anymore (I can't repo on Linux, but we knew that), but we should be able to get rid of this file. Other features have similar set-ups, and don't need a file like this. Let me comment further in the WebMediaRecorderHandlerClient file. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:18: public: All these methods, aside from the destructor, can be pure virtual. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:33: // [1] http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods micro nit: https:// https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:15: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandlerClient { I don't think you need BLINK_PLATFORM_EXPORT here. This is what may be causing your linking error. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:21: virtual void failOtherRecordingError(const WebString& message) {} Since this class shouldn't be instantiated by itself, why not make all these virtuals, aside from the destructor, pure virtual?
On 2015/09/01 17:17:12, Peter Beverloo wrote: > I think we're nearly there, and left mostly nits. I'm OK landing this as-is > while the spec issues get resolved, but we should resolve them prior to > shipping. To clarify - as-is in regards to the unresolved spec issues, not the rest of the review.
Patchset #10 (id:560001) has been deleted
Patchset #10 (id:580001) has been deleted
peter@ PTAL. LGTM %nits pretty please? https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html (right): https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video.html:18: recorder = null On 2015/09/01 17:17:12, Peter Beverloo wrote: > Do we need this line if it's followed immediately by an assert_unreached()? > (Thus failing the test.) > > If so, add a semi-colon. ASI is hard. Done removed. https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html (right): https://codereview.chromium.org/1255873002/diff/540001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType.html:7: // http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods On 2015/09/01 17:17:12, Peter Beverloo wrote: > micro nit: https Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:59: , m_ignoreMutedMedia(true) On 2015/09/01 17:17:12, Peter Beverloo wrote: > We currently don't use this property, and only reflect it in JavaScript, right? > Will we support it eventually? > > It would be good to add a TODO to make sure we don't forget about this. Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:74: exceptionState.throwDOMException(NotSupportedError, "Failed to initialize native MediaRecorder."); On 2015/09/01 17:17:12, Peter Beverloo wrote: > The specification states the following: > > "If the UA does not support the format or any of the parameters specified, it > must raise an UnsupportedOption Exception." > > Putting aside the UnsupportedOption bit (I replied to one of your GitHub > issues), throwing a NotSupportedError in that case seems good. I would, however, > like to see a clearer error message, so that developers know what's going on. > > Perhaps copy the MediaSource message? ("The type provided (..m_mimeType..) is > unsupported.") Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:223: scheduleDispatchEvent(MediaRecorderErrorEvent::create( On 2015/09/01 17:17:12, Peter Beverloo wrote: > nit: note that you don't have to wrap the line. I don't mind too much either > way. Acknowledged. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.h (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.h:52: bool ignoreMutedMedia() const { return m_ignoreMutedMedia; } On 2015/09/01 17:17:12, Peter Beverloo wrote: > nit: maybe move these to line 44? That way the header file order matches the > IDL. Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.cpp:31: , m_message(message) { } On 2015/09/01 17:17:12, Peter Beverloo wrote: > micro nit: {} on their own lines. Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.h (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.h:15: class MODULES_EXPORT MediaRecorderErrorEvent final : public Event { On 2015/09/01 17:17:12, Peter Beverloo wrote: > Perhaps add a TODO for the GitHub issue, given that this event could go away in > its entirety if the specification would switch to DOMException? > > https://github.com/w3c/mediacapture-record/issues/14 Done. https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorderErrorEvent.idl (right): https://codereview.chromium.org/1255873002/diff/540001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorderErrorEvent.idl:8: enum RecordingErrorNameEnum { On 2015/09/01 17:17:12, Peter Beverloo wrote: > dito: Perhaps add a TODO? > > I'm OK with landing as-is, but this is something that would be very good to > resolve prior to shipping. Done. https://codereview.chromium.org/1255873002/diff/540001/Source/platform/export... File Source/platform/exported/WebMediaRecorderHandlerClient.cpp (right): https://codereview.chromium.org/1255873002/diff/540001/Source/platform/export... Source/platform/exported/WebMediaRecorderHandlerClient.cpp:10: // constructor/destructor and should be in Source/platform/exported. On 2015/09/01 17:17:12, Peter Beverloo wrote: > The compile logs aren't available anymore (I can't repo on Linux, but we knew > that), but we should be able to get rid of this file. Other features have > similar set-ups, and don't need a file like this. > > Let me comment further in the WebMediaRecorderHandlerClient file. Acknowledged. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandler.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:18: public: On 2015/09/01 17:17:12, Peter Beverloo wrote: > All these methods, aside from the destructor, can be pure virtual. Absolutely. esprehn@ suggested a few PSs ago to provide empty implementations so adding new methods here would not need corresponding changes in Chromium (avoiding rolls etc). https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandler.h:33: // [1] http://w3c.github.io/mediacapture-record/MediaRecorder.html#methods On 2015/09/01 17:17:12, Peter Beverloo wrote: > micro nit: https:// Done. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:15: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandlerClient { On 2015/09/01 17:17:12, Peter Beverloo wrote: > I don't think you need BLINK_PLATFORM_EXPORT here. This is what may be causing > your linking error. I'm afraid Windows is not happy to have MediaRecorder as a WebMediaRecorderHandlerClient and not see a .cpp file. Win compiler doesn't generate an object file at all for the base class if it doesn't see a .cpp file. Sad, but I've found it before, see below. (The /.../ are mine). e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : error C2220: warning treated as error - no 'object' file generated e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : warning C4275: non dll-interface class 'blink::WebMediaRecorderHandlerClient' used as base for dll-interface class 'blink::MediaRecorder' e:\b\build\slave\...\public\platform\webmediarecorderhandlerclient.h(15) : see declaration of 'blink::WebMediaRecorderHandlerClient' e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(23) : see declaration of 'blink::MediaRecorder' ninja: build stopped: subcommand failed. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:21: virtual void failOtherRecordingError(const WebString& message) {} On 2015/09/01 17:17:12, Peter Beverloo wrote: > Since this class shouldn't be instantiated by itself, why not make all these > virtuals, aside from the destructor, pure virtual? Done.
lgtm % nits :) I would really prefer to see the WebMediaRecorderHandlerClient linking error resolved before landing, however, and suspect the Platform OWNERS will tell you the same. https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:15: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandlerClient { On 2015/09/02 03:10:18, mcasas wrote: > On 2015/09/01 17:17:12, Peter Beverloo wrote: > > I don't think you need BLINK_PLATFORM_EXPORT here. This is what may be causing > > your linking error. > > I'm afraid Windows is not happy to have MediaRecorder as a > WebMediaRecorderHandlerClient and not see a .cpp file. Win > compiler doesn't generate an object file at all for the base > class if it doesn't see a .cpp file. Sad, but I've found it before, > see below. (The /.../ are mine). > > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : error > C2220: warning treated as error - no 'object' file generated > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : warning > C4275: non dll-interface class 'blink::WebMediaRecorderHandlerClient' used as > base for dll-interface class 'blink::MediaRecorder' > > e:\b\build\slave\...\public\platform\webmediarecorderhandlerclient.h(15) > : see declaration of 'blink::WebMediaRecorderHandlerClient' > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(23) : > see declaration of 'blink::MediaRecorder' > ninja: build stopped: subcommand failed. What I don't understand is why you need this at all. Did those errors occur without BLINK_PLATFORM_EXPORT? To illustrate, the Notification in Source/modules/notifications/Notification.h implements WebNotificationDelegate which is logically identical to your class. However, it doesn't need a .cpp file. There are various other examples of this in Blink as well. (I've created https://codereview.chromium.org/1311353010 to see if the destructor might make the difference.) https://codereview.chromium.org/1255873002/diff/600001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt (right): https://codereview.chromium.org/1255873002/diff/600001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:3: FAIL check MediaRecorder.canRecordMimeType() with video/webm and vp8 assert_equals: expected "maybe" but got "" Out of interest, aren't these tests going to fail when you implement the Chromium-side of this change? If so, you may want to remove them now, and add them after that lands. It saves you from a 3-sided patch, and means that you don't have to check in failing expectations. https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:282: dispatchEvent((*it).release()); nit: since both RefPtr and Member exercise the same memory semantics, you can just pass in the Event w/o using release(). That will happen anyway then |events| goes out of scope (the .clear() call below isn't necessary either). https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.idl (right): https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.idl:7: enum RecordingStateEnum { "inactive", "recording", "paused"}; micro nit: consistent whitespace (add space before }). https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.idl:12: Constructor (MediaStream stream, [TreatUndefinedAs=Missing] optional DOMString mimeType), micro nit: no space before (
Patchset #11 (id:620001) has been deleted
mcasas@chromium.org changed reviewers: - philipj@opera.com, tommi@chromium.org
peter@ thanks, I managed to get rid of the loathsome .cpp :) esprehn@ can you RS Lgtm/PTAL? (I'm removing philipj@ and tommi@ to trim down the reviewers list) https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... File public/platform/WebMediaRecorderHandlerClient.h (right): https://codereview.chromium.org/1255873002/diff/540001/public/platform/WebMed... public/platform/WebMediaRecorderHandlerClient.h:15: class BLINK_PLATFORM_EXPORT WebMediaRecorderHandlerClient { On 2015/09/02 16:39:37, Peter Beverloo wrote: > On 2015/09/02 03:10:18, mcasas wrote: > > On 2015/09/01 17:17:12, Peter Beverloo wrote: > > > I don't think you need BLINK_PLATFORM_EXPORT here. This is what may be > causing > > > your linking error. > > > > I'm afraid Windows is not happy to have MediaRecorder as a > > WebMediaRecorderHandlerClient and not see a .cpp file. Win > > compiler doesn't generate an object file at all for the base > > class if it doesn't see a .cpp file. Sad, but I've found it before, > > see below. (The /.../ are mine). > > > > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : error > > C2220: warning treated as error - no 'object' file generated > > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(25) : > warning > > C4275: non dll-interface class 'blink::WebMediaRecorderHandlerClient' used as > > base for dll-interface class 'blink::MediaRecorder' > > > > > e:\b\build\slave\...\public\platform\webmediarecorderhandlerclient.h(15) > > : see declaration of 'blink::WebMediaRecorderHandlerClient' > > e:\b\build\slave\...\source\modules\mediarecorder\mediarecorder.h(23) > : > > see declaration of 'blink::MediaRecorder' > > ninja: build stopped: subcommand failed. > > What I don't understand is why you need this at all. Did those errors occur > without BLINK_PLATFORM_EXPORT? > > To illustrate, the Notification in Source/modules/notifications/Notification.h > implements WebNotificationDelegate which is logically identical to your class. > However, it doesn't need a .cpp file. There are various other examples of this > in Blink as well. > > (I've created https://codereview.chromium.org/1311353010 to see if the > destructor might make the difference.) PS10 removes the .cpp and digs a bit further in the issue, thanks for pushing :) -- The problem was the destructor coupled with the BLINK_PLATFORM_EXPORT. I followed your example and DocumentWebSocketChannel, which is a WebSocketHandleClient and is MODULES_EXPORTed; the catch is that the base class has no destructor. https://codereview.chromium.org/1255873002/diff/600001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt (right): https://codereview.chromium.org/1255873002/diff/600001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-canRecordMimeType-expected.txt:3: FAIL check MediaRecorder.canRecordMimeType() with video/webm and vp8 assert_equals: expected "maybe" but got "" On 2015/09/02 16:39:37, Peter Beverloo wrote: > Out of interest, aren't these tests going to fail when you implement the > Chromium-side of this change? If so, you may want to remove them now, and add > them after that lands. It saves you from a 3-sided patch, and means that you > don't have to check in failing expectations. Yeah, this was a conundrum, I didn't want to land all this code with no hints as to how it was supposed to work, in the form of tests, but then I need to check the FAIL expectations. It'd be great if I could commit a single CL that would "connect" with the Chrome classes and update the ...-expected.txt, ideas? https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:282: dispatchEvent((*it).release()); On 2015/09/02 16:39:37, Peter Beverloo wrote: > nit: since both RefPtr and Member exercise the same memory semantics, you can > just pass in the Event w/o using release(). That will happen anyway then > |events| goes out of scope (the .clear() call below isn't necessary either). Cleaned up. https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.idl (right): https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.idl:7: enum RecordingStateEnum { "inactive", "recording", "paused"}; On 2015/09/02 16:39:37, Peter Beverloo wrote: > micro nit: consistent whitespace (add space before }). Done. https://codereview.chromium.org/1255873002/diff/600001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.idl:12: Constructor (MediaStream stream, [TreatUndefinedAs=Missing] optional DOMString mimeType), On 2015/09/02 16:39:37, Peter Beverloo wrote: > micro nit: no space before ( Done.
thakis@chromium.org changed reviewers: + thakis@chromium.org
stampy lgtm (but I'm not an OWNER in public/ for example) https://codereview.chromium.org/1255873002/diff/640001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt (right): https://codereview.chromium.org/1255873002/diff/640001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt:2: FAIL checks the video-only MediaRecorder API. assert_unreached: Exception while creating MediaRecorder: NotSupportedError: Failed to construct 'MediaRecorder': No MediaRecorder handler can be created. Reached unreachable code This test is expected to fail? …hm, apparently that's pretty common ( https://code.google.com/p/chromium/codesearch#search/&q=FAIL%20file:-expected... ), but it seems a bit strange to me to have this in a new test. Is the plan to make this PASS in a follow-up change? https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... File Source/modules/mediarecorder/OWNERS (right): https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... Source/modules/mediarecorder/OWNERS:1: mcasas@chromium.org owner files should contain at least 2 people. can you find someone to co-own this with you? (beverloo maybe?)
Patchset #12 (id:660001) has been deleted
esprehn@chromium.org changed reviewers: - thakis@chromium.org
lgtm, make sure to follow up on the bugs we need to make sure Edge and Firefox ship the same thing. https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.cpp (right): https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.cpp:21: static String stateToString(MediaRecorder::State state) You actually don't need both the anonymous namespace and the static. https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... File Source/modules/mediarecorder/MediaRecorder.idl (right): https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... Source/modules/mediarecorder/MediaRecorder.idl:12: Constructor(MediaStream stream, [TreatUndefinedAs=Missing] optional DOMString mimeType), You should file a bug in the spec that this should probably take (stream, options) where options is a dictionary that accepts mimeType as a key. That leaves this open to expand it later.
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, thakis@chromium.org, esprehn@chromium.org Link to the patchset: https://codereview.chromium.org/1255873002/#ps700001 (title: "esprehn@ last comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1255873002/700001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1255873002/700001
https://codereview.chromium.org/1255873002/diff/640001/LayoutTests/fast/media... File LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt (right): https://codereview.chromium.org/1255873002/diff/640001/LayoutTests/fast/media... LayoutTests/fast/mediarecorder/MediaRecorder-basic-video-expected.txt:2: FAIL checks the video-only MediaRecorder API. assert_unreached: Exception while creating MediaRecorder: NotSupportedError: Failed to construct 'MediaRecorder': No MediaRecorder handler can be created. Reached unreachable code On 2015/09/03 01:47:07, Nico (vacation Thu Sep 2) wrote: > This test is expected to fail? > > …hm, apparently that's pretty common ( > https://code.google.com/p/chromium/codesearch#search/&q=FAIL%20file:-expected... > ), but it seems a bit strange to me to have this in a new test. Is the plan to > make this PASS in a follow-up change? Absolutely. It might not be the next CL, but very soon. https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... File Source/modules/mediarecorder/OWNERS (right): https://codereview.chromium.org/1255873002/diff/640001/Source/modules/mediare... Source/modules/mediarecorder/OWNERS:1: mcasas@chromium.org On 2015/09/03 01:47:07, Nico (vacation Thu Sep 2) wrote: > owner files should contain at least 2 people. can you find someone to co-own > this with you? (beverloo maybe?) Added peter@.
Message was sent while issue was closed.
Committed patchset #13 (id:700001) as https://src.chromium.org/viewvc/blink?view=rev&revision=201727 |
