Bug 190438 – Implement error handler of MediaRecorder

• Component: WebRTC
• Status: RESOLVED FIXED

[Wendy]

After start running the MedisRecorder, if any of the track is added or removed, the 'onerror' event will be called on javascript, and the state of MediaRecorder will be inactive.

[Wendy]

Created attachment 351991 [details]
Patch

[youenn fablet]

Wendy, could you make any new MediaRecorder bug (this one and bug 190291) as blocking bug 85851?
That will help bug 85851 observers to see progress in the implementation.

[youenn fablet]

Comment on attachment 351991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351991&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:79
> +        dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));

It should probably be asynchronous and dispatch a MediaRecorderErrorEvent.
Spec is unclear though. I filed https://github.com/w3c/mediacapture-record/issues/151.
In the meantime, dispatching Unknown exception might be good enough.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:36
> +class MediaRecorder final : public ActiveDOMObject, public RefCounted<MediaRecorder>, public EventTargetWithInlineData, public MediaStream::Observer {

Can you make it private MediaStream::Observer?

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:74
> +    void didAddOrRemoveTrack();

final

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9523081

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 351997 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9523703

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 351999 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9523717

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 352001 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9525318

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 352009 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9525559

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 352012 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 351991 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351991&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:48
> +    m_stream->addObserver(this);

Isn't it concerning that MediaRecorder adds itself as an observer but never unregisters itself? I would have expected a matching unregisterObserver() call in MediaRecorder's destructor.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:64
> +        return Exception { InvalidStateError };

Please provide a useful error message (second parameter to Exception constructor).

Also, your test does not seem to cover this case.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:66
> +        timeslice = std::numeric_limits<int>::max();

unused?

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:74
> +    if (scriptExecutionContext()->activeDOMObjectsAreSuspended() || scriptExecutionContext()->activeDOMObjectsAreStopped())

What guarantees that scriptExecutionContext() does not return nullptr?

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:79
>> +        dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
> 
> It should probably be asynchronous and dispatch a MediaRecorderErrorEvent.
> Spec is unclear though. I filed https://github.com/w3c/mediacapture-record/issues/151.
> In the meantime, dispatching Unknown exception might be good enough.

I agree this should fire a MediaRecorderErrorEvent:
https://w3c.github.io/mediacapture-record/#error-handling-principles

I also agree firing events asynchronously is better practice.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:2
> +<title>MediaRecorder Error</title>

Could please add appropriate <html> <head> <body> elements?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:11
> +        ac = new AudioContext();

Why is this global?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:19
> +        canvas = document.getElementById("canvas");

Why is this global?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:24
> +        callError = false;

Bad naming: Should be something like "wasErrorHandlerCalled"

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:25
> +        video = createVideoStream();

Why is this global?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:26
> +        audio = createAudioStream();

ditto.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:27
> +        recorder = new MediaRecorder(video);

ditto.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:29
> +            callError = true;

Ideally, you should do some basic checks on the error event as well.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:35
> +        assert_true(callError, "onerror should be fired");

"onerror should be fired" does not make much sense given that onerror is an event handler, not an event. It should either be:
- onerror should be called
or my preferred one
- error event should be fired

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:36
> +    }, "MediaRecorder will stop recording when any of track is added and onerror event will be fired");

onerror -> error

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:39
> +        callError = false;

Bad naming: Should be something like "wasErrorHandlerCalled"

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:40
> +        video = createVideoStream();

Why is this global?

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:41
> +        recorder = new MediaRecorder(video);

ditto.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:43
> +            callError = true;

Ideally, you should do some basic checks on the error event as well.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:49
> +        assert_true(callError, "onerror should be fired");

onerror -> error

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:50
> +    }, "MediaRecorder will stop recording when any of track is removed and onerror event will be fired");

onerror -> error

[EWS Watchlist]

Comment on attachment 351991 [details]
Patch

Attachment 351991 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9527389

New failing tests:
fast/mediacapturefromelement/CanvasCaptureMediaStream-imagebitmaprenderingcontext.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-framerate-0.html
fast/mediacapturefromelement/CanvasCaptureMediaStream-capture-out-of-DOM-element.html

[EWS Watchlist]

Created attachment 352023 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Wendy]

Created attachment 352079 [details]
Patch

[Wendy]

Created attachment 352083 [details]
Patch

[Chris Dumez]

Comment on attachment 352079 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352079&action=review

> Source/WebCore/ChangeLog:6
> +        Reviewed by Youenn Fablet and Chris Dumez (OOPS!).

You are only supposed to update this once you got a r+, did you get a r+ from Youenn? I know I did not :)
Also, you're supposed to drop the (OOPS!).

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:57
> +    m_state = RecordingState::Inactive;

There is no need to reset the state in the destructor.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:85
> +    if (state() != RecordingState::Inactive) {

Should likely be an early return as per coding style:
if (state() == RecordingState::Inactive)
    return;

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:77
> +    void didAddOrRemoveTrack() final;

Please do not put methods in between the data members. Data members should always come last.

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp:48
> +    , m_domError(init.error)

init.error.releaseNonNull()

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h:37
> +    DOMException* error() const { return m_domError.get(); }

We should probably move this one after the create() factories for clarity.

Also, it can return a DOMException& since I do not think the error can be null.

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h:44
> +    static Ref<MediaRecorderErrorEvent> create(const AtomicString&, CanBubble, IsCancelable, Exception&&);

I think we can omit the CanBubble, IsCancelable parameters and use the values defined in the spec inside the constructor.

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h:47
> +    EventInterface eventInterface() const override;

Can this be private?

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h:53
> +    RefPtr<DOMException> m_domError;

Can probably be a Ref<>, I do not think it can be null.

> LayoutTests/ChangeLog:6
> +        Reviewed by Youenn Fablet and Chris Dumez (OOPS!).

Same comment as earlier.

> LayoutTests/imported/w3c/ChangeLog:6
> +        Reviewed by Youenn Fablet and Chris Dumez (OOPS!).

Same comment as earlier.

[Wendy]

Created attachment 352084 [details]
Patch

[Wendy]

Created attachment 352095 [details]
Patch

[youenn fablet]

Comment on attachment 352084 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352084&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:83
> +        return;

MediaRecorder is an ActiveDOMObject.
We should probably implement MediaRecorder ::stop() and store the fact that media recorder is stopped.
Inside stop(), we would do the same stuff as done in MediaRecorder::~MediaRecorder.

That will remove the need for this specific check, which might not always be safe as scriptExecutionContext() might sometimes be null.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:87
> +        auto event = MediaRecorderErrorEvent::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No, Exception { UnknownError, "Track cannot be added to or removed from the MediaStream while recording is happening"_s });

As Chris said, Event::CanBubble::No, Event::IsCancelable::No should probably not be needed here.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:30
> +#include "GenericEventQueue.h"

We probably can forward declare GenericEventQueue.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.h:48
> +    virtual ~MediaRecorder();

s/virtual//

> LayoutTests/ChangeLog:8
> +        Since dataavailable event has not been implemented, skip three MediaRecorder-related tests now, 

s/now/now./

> LayoutTests/ChangeLog:9
> +        and need to reenable those tests once implementing the dataavailable event for MediaRecorder.

/and need to/We will /

> LayoutTests/ChangeLog:13
> +        * platform/mac/TestExpectations:

It might be simpler to skip them in LayoutTests/TestExpectations.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:30
> +        recorder.onerror = t.step_func(function (mediaRecorderErrorEvent) {

You can also use arrow function which preserves 'this' and is shorter than function.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:41
> +        }, 500);

500 milliseconds might be too small for some very slow bots.
Let's bump it to 2 seconds.
Ditto below.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:61
> +    async_test(t => {

It does not need to be an async test.
A regular test will be good enough.

[Wendy]

Created attachment 352108 [details]
Patch

[youenn fablet]

Comment on attachment 352108 [details]
Patch

This patch is almost ready.
Can you verify whether the state should be changed asynchronously or not?
If so, can you update the patch accordingly?

View in context: https://bugs.webkit.org/attachment.cgi?id=352108&action=review

> Source/WebCore/ChangeLog:22
> +        * Modules/mediarecorder/MediaRecorderErrorEvent.cpp: Copied from Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp.

s/Copied from.../Added/

> Source/WebCore/ChangeLog:27
> +        * Modules/mediarecorder/MediaRecorderErrorEvent.idl: Copied from Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp.

s/Copied from.../Added/

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:91
> +    m_state = RecordingState::Inactive;

m_state should probable be modified asynchronously as per spec.
Maybe this call to use a deferred task directly to do both state change and event dispatch.

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp:48
> +    , m_domError(init.error.releaseNonNull())

init is being moved but domError is also using it.
While it might work, it seems fragile.
I would change the caller  (MediaRecorderErrorEvent::create) so as to pass Exception&&.
Something like
auto domError = init.error.releaseNonNull();
...
new MediaRecorderErrorEvent(type, WTFMove(init), WTFMove(domError), isTrusted);

Also do we need isTrusted? Can we just use IsTrusted::No and not pass this parameter?

> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.idl:32
> +    Exposed=Window

This should be:
EnabledAtRuntime=MediaRecorder,

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:38
> +        assert_equals(recorder.state, "inactive", "MediaRecorder has been stopped after adding a track to stream");

This is probably wrong, this should be checked in recorder.onerror since state should be changed asynchronously.

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:67
> +            recorder.start();

Nice!

[Chris Dumez]

Comment on attachment 352108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352108&action=review

>> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp:48
>> +    , m_domError(init.error.releaseNonNull())
> 
> init is being moved but domError is also using it.
> While it might work, it seems fragile.
> I would change the caller  (MediaRecorderErrorEvent::create) so as to pass Exception&&.
> Something like
> auto domError = init.error.releaseNonNull();
> ...
> new MediaRecorderErrorEvent(type, WTFMove(init), WTFMove(domError), isTrusted);
> 
> Also do we need isTrusted? Can we just use IsTrusted::No and not pass this parameter?

It is an event fired by the user-agent so I think it should be *Trusted*. Untrusted ones are fired by JS.

[Chris Dumez]

(In reply to Chris Dumez from comment #25)
> Comment on attachment 352108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352108&action=review
> 
> >> Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp:48
> >> +    , m_domError(init.error.releaseNonNull())
> > 
> > init is being moved but domError is also using it.
> > While it might work, it seems fragile.
> > I would change the caller  (MediaRecorderErrorEvent::create) so as to pass Exception&&.
> > Something like
> > auto domError = init.error.releaseNonNull();
> > ...
> > new MediaRecorderErrorEvent(type, WTFMove(init), WTFMove(domError), isTrusted);
> > 
> > Also do we need isTrusted? Can we just use IsTrusted::No and not pass this parameter?
> 
> It is an event fired by the user-agent so I think it should be *Trusted*.
> Untrusted ones are fired by JS.

So, to be clear, I think we should keep the IsTrustedParameter (set it to No by default for when it is created by JS), but pass IsTrusted::Yes from your C++ call site that fires the event.

[youenn fablet]

Comment on attachment 352108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352108&action=review

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:32
> +            assert_equals(mediaRecorderErrorEvent.error.name, 'UnknownError', 'the type of error should be UnknownError when track has been added or removed');

Let's add a check to ensure the event is trusted.
Discussing with Chris, it is not clear whether the error event being created is or not.

[youenn fablet]

Comment on attachment 352108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352108&action=review

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:32
> +            assert_equals(mediaRecorderErrorEvent.error.name, 'UnknownError', 'the type of error should be UnknownError when track has been added or removed');

Let's add a check to ensure the event is trusted.
Discussing with Chris, it is not clear whether the error event being created is or not.

[Chris Dumez]

Comment on attachment 352108 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352108&action=review

> LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html:32
> +            assert_equals(mediaRecorderErrorEvent.error.name, 'UnknownError', 'the type of error should be UnknownError when track has been added or removed');

It would be good to check that mediaRecorderErrorEvent.isTrusted is true as well.

[Wendy]

Created attachment 352195 [details]
Patch

[youenn fablet]

Comment on attachment 352195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352195&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:89
> +    scheduleDeferredTask([this] {

You need to check for (!m_isActive || state() == RecordingState::Inactive) here as it might have changed.

[youenn fablet]

(In reply to youenn fablet from comment #31)
> Comment on attachment 352195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352195&action=review
> 
> > Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:89
> > +    scheduleDeferredTask([this] {
> 
> You need to check for (!m_isActive || state() == RecordingState::Inactive)
> here as it might have changed.

The test should check that the state is active after changing the track and inactive in on error event handler.

[Chris Dumez]

Comment on attachment 352195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352195&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:90
> +        auto event = MediaRecorderErrorEvent::create(eventNames().errorEvent, Exception { UnknownError, "Track cannot be added to or removed from the MediaStream while recording is happening"_s });

What if we're no longer active by then?

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:104
> +    callOnMainThread([weakThis = makeWeakPtr(*this), function = WTFMove(function)] {

Should probably be a ScriptExecutionContext::postTask()

[Chris Dumez]

Comment on attachment 352195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352195&action=review

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:54
> +    if (m_isActive)

These 2 lines basically reset m_isActive to false, which is not needed in a destructor.

[Wendy]

Created attachment 352215 [details]
Patch

[Chris Dumez]

Comment on attachment 352215 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=352215&action=review

LGTM but I have a suggestion.

> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:59
> +    m_isActive = false;

Should we remove ourselves as an observer here as well? It seems like it'd be good to do it as early as possible.

Then I think we would not need the m_isActive flag as didAddOrRemoveTrack() could not get called after we're no longer active.

[youenn fablet]

Comment on attachment 352215 [details]
Patch

LGTM too :)

View in context: https://bugs.webkit.org/attachment.cgi?id=352215&action=review

>> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:59
>> +    m_isActive = false;
> 
> Should we remove ourselves as an observer here as well? It seems like it'd be good to do it as early as possible.
> 
> Then I think we would not need the m_isActive flag as didAddOrRemoveTrack() could not get called after we're no longer active.

We discussed this with Wendy yesterday and thought that this might not be worth.
Contrary to observer being call for every frame, this observer will never be called.
But we will in most cases try to remove the observer from the observer map twice.

[Chris Dumez]

(In reply to youenn fablet from comment #37)
> Comment on attachment 352215 [details]
> Patch
> 
> LGTM too :)
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352215&action=review
> 
> >> Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp:59
> >> +    m_isActive = false;
> > 
> > Should we remove ourselves as an observer here as well? It seems like it'd be good to do it as early as possible.
> > 
> > Then I think we would not need the m_isActive flag as didAddOrRemoveTrack() could not get called after we're no longer active.
> 
> We discussed this with Wendy yesterday and thought that this might not be
> worth.
> Contrary to observer being call for every frame, this observer will never be
> called.
> But we will in most cases try to remove the observer from the observer map
> twice.

Ok

[WebKit Commit Bot]

Comment on attachment 352215 [details]
Patch

Rejecting attachment 352215 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 352215, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=352215&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=190438&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 352215 from bug 190438.
Fetching: https://bugs.webkit.org/attachment.cgi?id=352215
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.idl
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error-expected.txt
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/TestExpectations
	M	LayoutTests/imported/w3c/ChangeLog
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/DerivedSources.make
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.h
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.idl
	M	Source/WebCore/Sources.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/bindings/js/WebCoreBuiltinNames.h
	M	Source/WebCore/dom/EventNames.in
	M	Source/WebCore/dom/EventTargetFactory.in

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: b86ad68395fa839c9ff66a6dd4e0396c139fd4af and refs/remotes/origin/master differ, using rebase:
:040000 040000 77f97ebb1e4138c5024d28dea7700f347c8ba6bc 92eda367eb67787361b8f4da2a96959390505aa7 M	LayoutTests
:040000 040000 98a323ac310b04dd2019f6cfa11ecebf675dfe26 db349c7d53434e63104b36e19511df8a3253ad7d M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.idl
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error-expected.txt
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/TestExpectations
	M	LayoutTests/imported/w3c/ChangeLog
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/DerivedSources.make
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.h
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.idl
	M	Source/WebCore/Sources.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/bindings/js/WebCoreBuiltinNames.h
	M	Source/WebCore/dom/EventNames.in
	M	Source/WebCore/dom/EventTargetFactory.in

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: b86ad68395fa839c9ff66a6dd4e0396c139fd4af and refs/remotes/origin/master differ, using rebase:
:040000 040000 77f97ebb1e4138c5024d28dea7700f347c8ba6bc 92eda367eb67787361b8f4da2a96959390505aa7 M	LayoutTests
:040000 040000 98a323ac310b04dd2019f6cfa11ecebf675dfe26 db349c7d53434e63104b36e19511df8a3253ad7d M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files

Full output: https://webkit-queues.webkit.org/results/9559633

[WebKit Commit Bot]

Comment on attachment 352215 [details]
Patch

Rejecting attachment 352215 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 352215, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=352215&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=190438&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 352215 from bug 190438.
Fetching: https://bugs.webkit.org/attachment.cgi?id=352215
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.idl
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error-expected.txt
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/TestExpectations
	M	LayoutTests/imported/w3c/ChangeLog
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/DerivedSources.make
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.h
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.idl
	M	Source/WebCore/Sources.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/bindings/js/WebCoreBuiltinNames.h
	M	Source/WebCore/dom/EventNames.in
	M	Source/WebCore/dom/EventTargetFactory.in

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: da2e2722190129d719e49bf2de56959ad68c7650 and refs/remotes/origin/master differ, using rebase:
:040000 040000 77f97ebb1e4138c5024d28dea7700f347c8ba6bc 92eda367eb67787361b8f4da2a96959390505aa7 M	LayoutTests
:040000 040000 98a323ac310b04dd2019f6cfa11ecebf675dfe26 db349c7d53434e63104b36e19511df8a3253ad7d M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.cpp
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.h
	C	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp => Source/WebCore/Modules/mediarecorder/MediaRecorderErrorEvent.idl
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error-expected.txt
	A	LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-error.html
	M	LayoutTests/ChangeLog
	M	LayoutTests/TestExpectations
	M	LayoutTests/imported/w3c/ChangeLog
	M	Source/WebCore/CMakeLists.txt
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/DerivedSources.make
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.cpp
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.h
	M	Source/WebCore/Modules/mediarecorder/MediaRecorder.idl
	M	Source/WebCore/Sources.txt
	M	Source/WebCore/WebCore.xcodeproj/project.pbxproj
	M	Source/WebCore/bindings/js/WebCoreBuiltinNames.h
	M	Source/WebCore/dom/EventNames.in
	M	Source/WebCore/dom/EventTargetFactory.in

ERROR from SVN:
A repository hook failed: Commit blocked by pre-commit hook (exit code 1) with output:
Commits are currently disabled while we update infrastructure.
W: da2e2722190129d719e49bf2de56959ad68c7650 and refs/remotes/origin/master differ, using rebase:
:040000 040000 77f97ebb1e4138c5024d28dea7700f347c8ba6bc 92eda367eb67787361b8f4da2a96959390505aa7 M	LayoutTests
:040000 040000 98a323ac310b04dd2019f6cfa11ecebf675dfe26 db349c7d53434e63104b36e19511df8a3253ad7d M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.
Total errors found: 0 in 1 files

Full output: https://webkit-queues.webkit.org/results/9559987

[EWS]

Comment on attachment 352215 [details]
Patch

Rejecting attachment 352215 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[EWS]

Comment on attachment 352215 [details]
Patch

Rejecting attachment 352215 [details] from commit-queue.

[e-mail] does not have committer permissions according to https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[youenn fablet]

svn is down for maintenance.
When up again, it will be cq+able again, probably by tomorrow or Sunday.

[WebKit Commit Bot]

Comment on attachment 352215 [details]
Patch

Clearing flags on attachment: 352215

Committed r237106: <https://trac.webkit.org/changeset/237106>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45271123>