Make Error Event go after ondataavailable. #118

yell0wd0g · 2017-02-14T22:40:49Z

Upon an error condition, first fire an Event called ondataavailable, then an Event called stop and finally fire an error event.

Addresses #52 .

Chrome update in progress in https://crrev.com/2697703003

Preview | Diff

…alled stop and then an Event called Error

…lable

yell0wd0g · 2017-02-14T23:00:07Z

@foolip tiny change to write what we agreed upon in the bug, ptal.

foolip · 2017-02-15T02:06:30Z

MediaRecorder.bs

-far; after raising the error, the UA will <a href="#to-fire-a-blob-event">fire a
-dataavailable event</a> with <var>blob</var>; immediately after the UA will then
-<a>fire an event</a> named <code>stop</code>.
+far; before raising the error, the UA will <a href="#to-fire-a-blob-event">fire a


The use of "will" here makes this sound not normative but rather descriptive. If this is the only place that ensures the order of things, can it say "must" more?

Also, in normative requirements, I would expect to see something like "queue a task to..." or clearly requiring that something happens synchronously. I'm not sure if the difference is observable, but basically I would expect the words here to map 1:1 to what an implementation actually does.

(After some offline chatting, we've decided that is best to beef up this subsection and make it normative with the appropriate writing, otherwise is quite misleading. New PS coming imminently.)

…b.com:miguelao/mediacapture-record into prA03__send_error_event_after_ondataavailable

yell0wd0g · 2017-02-15T03:42:51Z

Ready for review @foolip

foolip

LGTM % nits, if some other implementers takes a look that'd be good.

foolip · 2017-02-15T05:05:12Z

MediaRecorder.bs

+If at any moment the UA finds an error condition, the UA MUST run the following
+steps:
+  <ol>
+    <li>If {{state}} is {{inactive}} throw an {{InvalidStateError}}


The error probably happens in the background, so there isn't anywhere to throw the exception. Can there even be an error when inactive? Is it because state becomes inactive before it's really inactive?

This was some legacy from before where the general principles of error handling stated that we should try to throw synchronously a DOMException if possible at all before having to fire the error event. The first part is kind-of obvious and is anyway stated individually in each of the commands (start(), stop() etc), so I'll remove it and go for the meat of the section.

foolip · 2017-02-15T05:06:26Z

MediaRecorder.bs

+      <li><a href="#to-fire-a-blob-event">Fire a blob event</a> named
+        <a>dataavailable</a> at <var>target</var> with <var>blob</var>.</li>
+      <li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>
+      <li><a>Fire an error event</a> named {{MediaRecorderErrorEvent}} at


MediaRecorderErrorEvent is the name of the interface, I guess the event type is just "error"?

Yeah, changed.

yell0wd0g · 2017-02-15T17:34:00Z

@foolip, thanks, done adding your changes.

@jan-ivar can you take a look plz?

foolip · 2017-02-16T04:21:48Z

MediaRecorder.bs

+
+  <li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>
+
+  <li><a>Fire an error event</a> named <var>event</var> at


Shouldn't it be "error"? Apart from the name, the italics would also cause me to go looking for a variable named event which has a string value.

Ooops, my bad, I mean named {{UnknownError}}, next PS coming.

foolip · 2017-02-16T05:14:25Z

MediaRecorder.bs

+
+  <li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>
+
+  <li><a>Fire an error event</a> named {{UnknownError}} at <var>target</var>.


OK, now I see why this is confusing. https://w3c.github.io/mediacapture-record/MediaRecorder.html#errorevent-section is an event interface where the member is a DOMException.

The event type (the string that is the first argument to addEventListener) should be "error", so this should say 'Fire an error event named "error" with an UnknownError at target'.

I see that the two existing uses of "fire an error event" also has a slightly wrong incantation. It would be possible to just bake the type ("error") into the definition of "fire an error event", but that's an editorial decision.

Ok, I'll write something in all three cases like:
<a>Fire an error event</a> named <a>error</a> with an {{UnknownError}} at <var>target</var>

(where <a>error</a> links to the error entry in the Event Summary section)

That should do the trick IIUC :-)

yell0wd0g · 2017-02-16T18:07:41Z

@jan-ivar this should be an easy review, PTAL

yell0wd0g · 2017-02-21T15:32:40Z

@jan-ivar ping

jan-ivar

So sorry for missing this request for so long! Hopefully it is not too late to provide feedback—thanks @Pehrsons for bringing to my attention.

I agree dataavailable should fire before error, unsure about stop. Also, we seem to have normative steps already that need to be fixed.

jan-ivar · 2019-01-28T14:12:10Z

MediaRecorder.bs

-It will signal a fatal error if these limits are exceeded.
+
+If at any moment the UA finds an error condition, the UA MUST queue a task,
+using the DOM manipulation task source, that runs the following steps:


I worry this is a bit broad.

But more importantly, it looks like we already have normative steps for this as mentioned above, so with this PR there would be two competing normative steps for this condition, which I think is confusing (do we fire twice?) - I'd vote we fix the existing place instead and leave General Principles non-normative or remove it, it seems a bit redundant.

jan-ivar · 2019-01-28T14:18:35Z

MediaRecorder.bs

-        <li><a>Fire an error event</a> named {{UnknownError}} at
-        <var>target</var>.</li>
+        <li><a>Fire an error event</a> named <a>error</a> with an
+        {{UnknownError}} at <var>target</var>.</li>


I think we should fix the event order here instead.

It looks like these are the normative steps we already have for recording errors, here in step 5.4 under the start method: "If the UA at any point is unable to continue gathering data for reasons other than isolation properties or stream track set, it MUST stop gathering data, and queue a task, using the DOM manipulation task source, that runs the following steps".

jan-ivar · 2019-01-28T14:33:28Z

MediaRecorder.bs

+
+  <li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>
+
+  <li><a>Fire an error event</a> named <a>error</a> with an {{UnknownError}} at


Question: Do we really want to fire stop before error? It would make it harder to detect errors IMHO. Most code I've seen seem to treat reaching stop without error as success, and this would break that assumption.

How about this order: dataavailable, error, stop?

alvestrand · 2023-02-09T15:31:11Z

@YellowDoge is this documenting the way Chrome works now after https://crbug.com/693179 was closed (2017)?

jan-ivar · 2023-02-09T15:31:25Z

@Pehrsons do you know if Firefox works the same as Chrome here? Any objection to merging this?

Pehrsons · 2023-02-16T12:20:48Z

Firefox fires events in this order:

"start"
"dataavailable" unless there is a security error coming
"error"
"stop"

My only concern with swapping "error" and "stop" is that it could seem unclear which session the "error" comes from since there potentially could be another "start" in flight at this point.

yell0wd0g and others added 4 commits February 14, 2017 14:37

On Error, first fire an Event called ondataavailable, then an Event c…

52df3be

…alled stop and then an Event called Error

correc link

d8c7a2e

correct link

43f20b4

Merge branch 'gh-pages' into prA03__send_error_event_after_ondataavai…

8d6477f

…lable

foolip reviewed Feb 15, 2017

View reviewed changes

yell0wd0g added 2 commits February 14, 2017 19:41

made #error-handling-principles more normative

d73b629

Merge branch 'prA03__send_error_event_after_ondataavailable' of githu…

82bc34b

…b.com:miguelao/mediacapture-record into prA03__send_error_event_after_ondataavailable

foolip approved these changes Feb 15, 2017

View reviewed changes

@foolip comments

6068d71

foolip reviewed Feb 16, 2017

View reviewed changes

s/<var>event</var>/{{UnknownError}}/

ce88b78

foolip reviewed Feb 16, 2017

View reviewed changes

Correct <a>fire an error event</a> call sites

96db655

foolip approved these changes Feb 16, 2017

View reviewed changes

yell0wd0g changed the base branch from gh-pages to master March 15, 2017 01:50

jan-ivar requested changes Jan 28, 2019

View reviewed changes

jan-ivar self-assigned this Jun 27, 2019

Base automatically changed from master to main February 15, 2021 07:32

dontcallmedom linked an issue Jun 4, 2021 that may be closed by this pull request

Ensure that the error event is the last event to be fired #52

Open

alvestrand added the Test needed label Feb 9, 2023


		<li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>

		<li><a>Fire an error event</a> named <var>event</var> at


		<li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>

		<li><a>Fire an error event</a> named {{UnknownError}} at <var>target</var>.


		<li><a>Fire an event</a> named <a>stop</a> at <var>target</var></li>

		<li><a>Fire an error event</a> named <a>error</a> with an {{UnknownError}} at

Make Error Event go after ondataavailable. #118

Are you sure you want to change the base?

Make Error Event go after ondataavailable. #118

Uh oh!

Conversation

yell0wd0g commented Feb 14, 2017 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yell0wd0g commented Feb 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yell0wd0g Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yell0wd0g commented Feb 15, 2017

Uh oh!

foolip left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yell0wd0g commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yell0wd0g commented Feb 16, 2017

Uh oh!

yell0wd0g commented Feb 21, 2017

Uh oh!

jan-ivar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alvestrand commented Feb 9, 2023

Uh oh!

jan-ivar commented Feb 9, 2023

Uh oh!

Pehrsons commented Feb 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yell0wd0g commented Feb 14, 2017 •

edited by pr-preview bot

Loading

yell0wd0g Feb 15, 2017 •

edited

Loading