Report errors in RTCPeerConnection legacy functions via the the failure callback instead of excepti…

Report errors in RTCPeerConnection legacy functions via the the failure callback instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and will reduce unexpected surprises when their return value changes to Promise<void>.

BUG=499126
Committed: https://crrev.com/e332af368744cceb112fcd228f74a53b381acec2
Cr-Commit-Position: refs/heads/master@{#380614}

[Guido Urdaneta, 2016-02-19 11:34:37]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[Guido Urdaneta, 2016-02-19 11:35:50]

Description was changed from

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the
spec.

BUG=499126
==========

to

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void> in M51.

BUG=499126
==========

[commit-bot: I haz the power, 2016-02-19 11:36:09]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713953002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713953002/20001

[Guido Urdaneta, 2016-02-19 12:30:18]

guidou@chromium.org changed reviewers:
+ philipj@opera.com

[Guido Urdaneta, 2016-02-19 12:32:55]

guidou@chromium.org changed reviewers:
+ jochen@chromium.org

[Guido Urdaneta, 2016-02-19 12:32:56]

Hi, PTAL.

[commit-bot: I haz the power, 2016-02-19 15:40:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 15:40:54]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[philipj_slow, 2016-02-22 10:03:42]

So this seems quite tricky, part of a broader issue that RTCErrorCallback needs
to be brought in line with the spec's RTCPeerConnectionErrorCallback. If that is
done, it wouldn't even compile to try using a String or a TypeError here.

Not sure what to advise, do you think it would be risky to go directly from the
current state where exceptions are thrown to rejecting promises?

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:382:
asyncCallErrorCallback(errorCallback, "Invalid offerToReceiveVideo");
Before this would throw a TypeError, and if I'm reading ErrorCallbackTask it
will now call the error callback with a string argument? I failed to pay
attention to this in the previous review, but that doesn't match what the spec
says, and I think the other calls to asyncCallErrorCallback have the same
problem.

Because the handling of RTCOfferOptions should eventually move into generated
bindings, this will presumably end up as a promise rejected with a TypeError
when everything has settled down. Because that should be handled by bindings, it
wouldn't be passed to the error callback, which should only ever take a
DOMException per spec.

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:474:
ASSERT(errorCallback);
The nullability of errorCallback didn't change with this CL, I guess this was
changed earlier without changing the implementation?

[Guido Urdaneta, 2016-02-22 11:37:50]

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:382:
asyncCallErrorCallback(errorCallback, "Invalid offerToReceiveVideo");
On 2016/02/22 10:03:41, philipj_UTC7 wrote:
> Before this would throw a TypeError, and if I'm reading ErrorCallbackTask it
> will now call the error callback with a string argument? I failed to pay
> attention to this in the previous review, but that doesn't match what the spec
> says, and I think the other calls to asyncCallErrorCallback have the same
> problem.
> 

Yes. The spec uses RTCPeerConnectionErrorCallback, which takes a DOMException.
Chromium is using the older RTCErrorCallback, which takes a String. We cannot
migrate to RTCPeerConnectionErrorCallback right away, as it will break all
existing applications, which expect a string.

So, the plan is:
1. Make the promise-based versions fully spec compliant.
2. Continue supporting the existing legacy version with as few changes as
possible.
3. Make the legacy version more and more compliant with the spec as uncompliant
use cases become less popular. If we are lucky, at some point we will make it
fully compliant.

> Because the handling of RTCOfferOptions should eventually move into generated
> bindings, this will presumably end up as a promise rejected with a TypeError
> when everything has settled down. Because that should be handled by bindings,
it
> wouldn't be passed to the error callback, which should only ever take a
> DOMException per spec.

The TypeErrors that are currently being thrown (and which I am proposing to
report via the callback) correspond to use cases that are not compliant with the
spec but which are commonplace today. In a fully compliant implementation those
TypeErrors would not exist.
The only TypeErrors that would exist in a 100% spec-compliant implementation
will be the cases were it is impossible to know which of the different versions
of the methods is being requested, so in those cases there would be no need to
invoke the error callback.
However, we are not there yet. There are a couple of errors that our legacy
implementation is reporting via exceptions and those will not be able to be
handled once we introduce the promise-based versions since we will have to
change the legacy method to return Promise<void>.

I think the best  way to avoid surprises once we do that is to report those
errors using the callback. The fact that we are still on the old
RTCErrorCallback that gets strings instead of DOMExceptions is actually an
advantage during the transition.

I guess the plan for the legacy methods would be:
1. Report constraint and old-style RTCOfferOptions parsing errors via the error
callback as strings. (This CL)
2. Remove support for constraints arguments.
3. Remove support for old-style RTCOfferOptions.
At this point, asyncCallErrorCallback will be called only when the connection
state is closed.
4. Replace RTCErrorCallback with RTCPeerConnectionErrorCallback and report
errors are DOMExceptions instead of strings.

Steps 2, 3 and 4 can proceed only when the respective use cases become rare,
which is not the case at the moment.

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:474:
ASSERT(errorCallback);
On 2016/02/22 10:03:41, philipj_UTC7 wrote:
> The nullability of errorCallback didn't change with this CL, I guess this was
> changed earlier without changing the implementation?

Correct. There was a previous CL making the error callback mandatory which only
changed the IDL, but not the implementation.
If we decide not to land this CL, we can prepare a separate CL that only
assumes/asserts that the error callbacks are not null.

[philipj_slow, 2016-02-23 09:13:54]

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:474:
ASSERT(errorCallback);
On 2016/02/22 11:37:50, Guido Urdaneta wrote:
> On 2016/02/22 10:03:41, philipj_UTC7 wrote:
> > The nullability of errorCallback didn't change with this CL, I guess this
was
> > changed earlier without changing the implementation?
> 
> Correct. There was a previous CL making the error callback mandatory which
only
> changed the IDL, but not the implementation.
> If we decide not to land this CL, we can prepare a separate CL that only
> assumes/asserts that the error callbacks are not null.

Sounds fine to do here, just checking.

[philipj_slow, 2016-02-23 09:39:50]

Thanks for the detailed reply. This is a tricky situation to navigate. Both the
overall plan for promises and the plan specifically for RTCOfferOptions depend
on usage dropping, but I think we should also prepare for the case where removal
is not possible and these things have to be standardized.

Let's look at the specifics.

offerToReceiveAudio and offerToReceiveVideo are treated as signed integers,
although it looks like ultimately they're treated merely as boolean values.
These are actually in Gecko's RTCPeerConnection.webidl:

dictionary RTCOfferOptions : RTCOfferAnswerOptions {
  long    offerToReceiveVideo;
  long    offerToReceiveAudio;
  ...
}

I think the reasonable thing would be to assume that this could be what ends up
being supported indefinitely, and make Blink behave exactly as if this were
specified using IDL and handled by bindings, so that it can later be moved to
bindings. 

In some ad-hoc testing with new MouseEvent('click', { screenX: *value* })almost
any non-number *value* I try converts to 0. The only exception I can find by
consulting the specs is a Symbol, which throws TypeError.

So I think it would be fine to just never throw for offerToReceiveVideo,
treating anything invalid as 0. Converting that to a proper dictionary should be
straightforward.

That leaves the two cases of MediaConstraintsImpl::create where a
MediaErrorState is used. AFAICT, the error can only be set in
parseOldStyleNames, because "Illegal value for constraint" or "Unknown name of
constraint detected". The first is apparently only used for testing, and the
second seems to throw an exception for an unrecognized dictionary member. If
that were moved to an IDL dictionary, it simply wouldn't do anything at all.

So in summary, it looks to me like these cases should all just silently do the
right thing, and not throw exceptions, invoke the error callback or (in the
future) reject the promise. Would that make sense?

[Guido Urdaneta, 2016-02-23 10:01:46]

On 2016/02/23 09:39:50, philipj_UTC7 wrote:
> Thanks for the detailed reply. This is a tricky situation to navigate. Both
the
> overall plan for promises and the plan specifically for RTCOfferOptions depend
> on usage dropping, but I think we should also prepare for the case where
removal
> is not possible and these things have to be standardized.
> 
> Let's look at the specifics.
> 
> offerToReceiveAudio and offerToReceiveVideo are treated as signed integers,
> although it looks like ultimately they're treated merely as boolean values.
> These are actually in Gecko's RTCPeerConnection.webidl:
> 
> dictionary RTCOfferOptions : RTCOfferAnswerOptions {
>   long    offerToReceiveVideo;
>   long    offerToReceiveAudio;
>   ...
> }
> 
> I think the reasonable thing would be to assume that this could be what ends
up
> being supported indefinitely, and make Blink behave exactly as if this were
> specified using IDL and handled by bindings, so that it can later be moved to
> bindings. 
> 
> In some ad-hoc testing with new MouseEvent('click', { screenX: *value*
})almost
> any non-number *value* I try converts to 0. The only exception I can find by
> consulting the specs is a Symbol, which throws TypeError.
> 
> So I think it would be fine to just never throw for offerToReceiveVideo,
> treating anything invalid as 0. Converting that to a proper dictionary should
be
> straightforward.
> 
> That leaves the two cases of MediaConstraintsImpl::create where a
> MediaErrorState is used. AFAICT, the error can only be set in
> parseOldStyleNames, because "Illegal value for constraint" or "Unknown name of
> constraint detected". The first is apparently only used for testing, and the
> second seems to throw an exception for an unrecognized dictionary member. If
> that were moved to an IDL dictionary, it simply wouldn't do anything at all.
> 
> So in summary, it looks to me like these cases should all just silently do the
> right thing, and not throw exceptions, invoke the error callback or (in the
> future) reject the promise. Would that make sense?

If I understand correctly, your proposal is to make the manual parsing of the
Dictionary behave like what IDL bindings would.
This implies:
1. Treat any invalid offerToReceive value as 0.
2. Don't throw or call error callback when a constraint parsing error is found.

In the future, perhaps M51 (or even this CL), convert these to proper
dictionaries.

I'm going to try this and see how it goes.

[philipj_slow, 2016-02-23 10:38:24]

On 2016/02/23 10:01:46, Guido Urdaneta wrote:
> On 2016/02/23 09:39:50, philipj_UTC7 wrote:
> > Thanks for the detailed reply. This is a tricky situation to navigate. Both
> the
> > overall plan for promises and the plan specifically for RTCOfferOptions
depend
> > on usage dropping, but I think we should also prepare for the case where
> removal
> > is not possible and these things have to be standardized.
> > 
> > Let's look at the specifics.
> > 
> > offerToReceiveAudio and offerToReceiveVideo are treated as signed integers,
> > although it looks like ultimately they're treated merely as boolean values.
> > These are actually in Gecko's RTCPeerConnection.webidl:
> > 
> > dictionary RTCOfferOptions : RTCOfferAnswerOptions {
> >   long    offerToReceiveVideo;
> >   long    offerToReceiveAudio;
> >   ...
> > }
> > 
> > I think the reasonable thing would be to assume that this could be what ends
> up
> > being supported indefinitely, and make Blink behave exactly as if this were
> > specified using IDL and handled by bindings, so that it can later be moved
to
> > bindings. 
> > 
> > In some ad-hoc testing with new MouseEvent('click', { screenX: *value*
> })almost
> > any non-number *value* I try converts to 0. The only exception I can find by
> > consulting the specs is a Symbol, which throws TypeError.
> > 
> > So I think it would be fine to just never throw for offerToReceiveVideo,
> > treating anything invalid as 0. Converting that to a proper dictionary
should
> be
> > straightforward.
> > 
> > That leaves the two cases of MediaConstraintsImpl::create where a
> > MediaErrorState is used. AFAICT, the error can only be set in
> > parseOldStyleNames, because "Illegal value for constraint" or "Unknown name
of
> > constraint detected". The first is apparently only used for testing, and the
> > second seems to throw an exception for an unrecognized dictionary member. If
> > that were moved to an IDL dictionary, it simply wouldn't do anything at all.
> > 
> > So in summary, it looks to me like these cases should all just silently do
the
> > right thing, and not throw exceptions, invoke the error callback or (in the
> > future) reject the promise. Would that make sense?
> 
> If I understand correctly, your proposal is to make the manual parsing of the
> Dictionary behave like what IDL bindings would.
> This implies:
> 1. Treat any invalid offerToReceive value as 0.
> 2. Don't throw or call error callback when a constraint parsing error is
found.
> 
> In the future, perhaps M51 (or even this CL), convert these to proper
> dictionaries.
> 
> I'm going to try this and see how it goes.

Yep, that is what I propose, at least the offerToReceiveAudio/Video case looks
very simple. The constraints also *look* simple, but there could be something
I'm overlooking.

[Guido Urdaneta, 2016-02-23 12:30:27]

guidou@chromium.org changed reviewers:
+ hta@chromium.org

[Guido Urdaneta, 2016-02-23 13:06:44]

On 2016/02/23 10:38:24, philipj_UTC7 wrote:
> On 2016/02/23 10:01:46, Guido Urdaneta wrote:
> > On 2016/02/23 09:39:50, philipj_UTC7 wrote:
> > > Thanks for the detailed reply. This is a tricky situation to navigate.
Both
> > the
> > > overall plan for promises and the plan specifically for RTCOfferOptions
> depend
> > > on usage dropping, but I think we should also prepare for the case where
> > removal
> > > is not possible and these things have to be standardized.
> > > 
> > > Let's look at the specifics.
> > > 
> > > offerToReceiveAudio and offerToReceiveVideo are treated as signed
integers,
> > > although it looks like ultimately they're treated merely as boolean
values.
> > > These are actually in Gecko's RTCPeerConnection.webidl:
> > > 
> > > dictionary RTCOfferOptions : RTCOfferAnswerOptions {
> > >   long    offerToReceiveVideo;
> > >   long    offerToReceiveAudio;
> > >   ...
> > > }
> > > 
> > > I think the reasonable thing would be to assume that this could be what
ends
> > up
> > > being supported indefinitely, and make Blink behave exactly as if this
were
> > > specified using IDL and handled by bindings, so that it can later be moved
> to
> > > bindings. 
> > > 
> > > In some ad-hoc testing with new MouseEvent('click', { screenX: *value*
> > })almost
> > > any non-number *value* I try converts to 0. The only exception I can find
by
> > > consulting the specs is a Symbol, which throws TypeError.
> > > 
> > > So I think it would be fine to just never throw for offerToReceiveVideo,
> > > treating anything invalid as 0. Converting that to a proper dictionary
> should
> > be
> > > straightforward.
> > > 
> > > That leaves the two cases of MediaConstraintsImpl::create where a
> > > MediaErrorState is used. AFAICT, the error can only be set in
> > > parseOldStyleNames, because "Illegal value for constraint" or "Unknown
name
> of
> > > constraint detected". The first is apparently only used for testing, and
the
> > > second seems to throw an exception for an unrecognized dictionary member.
If
> > > that were moved to an IDL dictionary, it simply wouldn't do anything at
all.
> > > 
> > > So in summary, it looks to me like these cases should all just silently do
> the
> > > right thing, and not throw exceptions, invoke the error callback or (in
the
> > > future) reject the promise. Would that make sense?
> > 
> > If I understand correctly, your proposal is to make the manual parsing of
the
> > Dictionary behave like what IDL bindings would.
> > This implies:
> > 1. Treat any invalid offerToReceive value as 0.
> > 2. Don't throw or call error callback when a constraint parsing error is
> found.
> > 
> > In the future, perhaps M51 (or even this CL), convert these to proper
> > dictionaries.
> > 
> > I'm going to try this and see how it goes.
> 
> Yep, that is what I propose, at least the offerToReceiveAudio/Video case looks
> very simple. The constraints also *look* simple, but there could be something
> I'm overlooking.

The offerToReceiveAudio/Video case is straightforward indeed. If we pass any
integer greater than zero, it will be treated as true. Otherwise it will be
treated as false.
The constraints case looks a bit trickier. In addition to the errors in
parseOldStyleNames(), there is the "Malformed constraints object" in parse().
It is not clear to me that we should pass a constraints object that failed to be
parsed to the lower layers. 
In addition, this kind of constraints parsing is used in other places like the
constructor and updateIce(), where an invalid constraints object is not passed
to the lower layers.

Adding hta@ for his input on this.

[hta - Chromium, 2016-02-23 14:55:50]

Note that offerToReceive* has been changed in the spec from booleans to
integers, and have subsequently been deleted, with the expectation that the
functionality will be available via addTransceiver. We still have to accept true
and false for backwards compatibility.

The way I read the spec for dictionaries, if we get something that can't be
parsed according to its type (typical: the string "true" where a number is
expected), we should call the error callback with "typeError", and not pass the
call to lower layers.

Dictionaries are defined to ignore all fields in the dictionary they don't have
parsers for, but for the ones they parse, they are required to report TypeError
as appropriate.

[philipj_slow, 2016-02-23 17:03:35]

On 2016/02/23 14:55:50, hta - Chromium wrote:
> Note that offerToReceive* has been changed in the spec from booleans to
> integers, and have subsequently been deleted, with the expectation that the
> functionality will be available via addTransceiver. We still have to accept
true
> and false for backwards compatibility.

Where are true and false handled in the code? I can only see it handled as an
integer, and in that context true/false would be converted to 1/0.

> The way I read the spec for dictionaries, if we get something that can't be
> parsed according to its type (typical: the string "true" where a number is
> expected), we should call the error callback with "typeError", and not pass
the
> call to lower layers.
> 
> Dictionaries are defined to ignore all fields in the dictionary they don't
have
> parsers for, but for the ones they parse, they are required to report
TypeError
> as appropriate.

Whether TypeError should be thrown actually depends on the types. Most things
are silently converted to strings for example, and the same is true for numbers.
You can try experimenting with MouseEvent('click', { screenX: *value* }).screenX
to see this. This is defined by http://heycam.github.io/webidl/#es-long and
https://tc39.github.io/ecma262/#sec-toint32 and AFAICT Symbol is the only thing
that would throw a TypeError exception.

[Guido Urdaneta, 2016-02-26 13:37:33]

On 2016/02/23 17:03:35, philipj_UTC7 wrote:
> On 2016/02/23 14:55:50, hta - Chromium wrote:
> > Note that offerToReceive* has been changed in the spec from booleans to
> > integers, and have subsequently been deleted, with the expectation that the
> > functionality will be available via addTransceiver. We still have to accept
> true
> > and false for backwards compatibility.
> 
> Where are true and false handled in the code? I can only see it handled as an
> integer, and in that context true/false would be converted to 1/0.
> 

But that's the behavior we want, no?

> > The way I read the spec for dictionaries, if we get something that can't be
> > parsed according to its type (typical: the string "true" where a number is
> > expected), we should call the error callback with "typeError", and not pass
> the
> > call to lower layers.
> > 
> > Dictionaries are defined to ignore all fields in the dictionary they don't
> have
> > parsers for, but for the ones they parse, they are required to report
> TypeError
> > as appropriate.
> 
> Whether TypeError should be thrown actually depends on the types. Most things
> are silently converted to strings for example, and the same is true for
numbers.
> You can try experimenting with MouseEvent('click', { screenX: *value*
}).screenX
> to see this. This is defined by http://heycam.github.io/webidl/#es-long and
> https://tc39.github.io/ecma262/#sec-toint32 and AFAICT Symbol is the only
thing
> that would throw a TypeError exception.

Given that:
1. We want to remove support for passing constraints to createAnswer and
createOffer, as it is not compliant.
2. Changing MediaConstraintsImpl to not fail in certain cases where it is
failing now is a nontrivial task that should be done in a separate CL, if at
all.
3. The legacy createOffer and createAnswer methods will be changed to return a
Promise<void>, so the TypeErrors that are currently being thrown are going to be
swallowed.
Isn't it better to instead return an exception via the errorCallback when the
parsing of the constraints fails?

[philipj_slow, 2016-02-26 13:52:55]

On 2016/02/26 13:37:33, Guido Urdaneta wrote:
> On 2016/02/23 17:03:35, philipj_UTC7 wrote:
> > On 2016/02/23 14:55:50, hta - Chromium wrote:
> > > Note that offerToReceive* has been changed in the spec from booleans to
> > > integers, and have subsequently been deleted, with the expectation that
the
> > > functionality will be available via addTransceiver. We still have to
accept
> > true
> > > and false for backwards compatibility.
> > 
> > Where are true and false handled in the code? I can only see it handled as
an
> > integer, and in that context true/false would be converted to 1/0.
> > 
> 
> But that's the behavior we want, no?

Yes, and it's the only thing that could be done using WebIDL, just wasn't sure
if hta@'s "We still have to accept true
and false for backwards compatibility" implied some special handling somewhere?

> > > The way I read the spec for dictionaries, if we get something that can't
be
> > > parsed according to its type (typical: the string "true" where a number is
> > > expected), we should call the error callback with "typeError", and not
pass
> > the
> > > call to lower layers.
> > > 
> > > Dictionaries are defined to ignore all fields in the dictionary they don't
> > have
> > > parsers for, but for the ones they parse, they are required to report
> > TypeError
> > > as appropriate.
> > 
> > Whether TypeError should be thrown actually depends on the types. Most
things
> > are silently converted to strings for example, and the same is true for
> numbers.
> > You can try experimenting with MouseEvent('click', { screenX: *value*
> }).screenX
> > to see this. This is defined by http://heycam.github.io/webidl/#es-long and
> > https://tc39.github.io/ecma262/#sec-toint32 and AFAICT Symbol is the only
> thing
> > that would throw a TypeError exception.
> 
> Given that:
> 1. We want to remove support for passing constraints to createAnswer and
> createOffer, as it is not compliant.
> 2. Changing MediaConstraintsImpl to not fail in certain cases where it is
> failing now is a nontrivial task that should be done in a separate CL, if at
> all.
> 3. The legacy createOffer and createAnswer methods will be changed to return a
> Promise<void>, so the TypeErrors that are currently being thrown are going to
be
> swallowed.
> Isn't it better to instead return an exception via the errorCallback when the
> parsing of the constraints fails?

Until the usage data for RTCPeerConnectionCreateOfferLegacyConstraints and
RTCPeerConnectionCreateAnswerLegacyConstraints becomes clear when M49 reaches
stable, I wouldn't assume that removing support for these constraints is
actually possible. The pre-stable trend for createOffer in particular is not
encouraging.

If none of the current TypeError cases would throw if this were all in WebIDL in
a hypothetical future, then simply not throwing really looks like the best path
to me. It takes us directly to a state that is both very unlikely to cause
problems with existing sites and one that could be maintained over the long term
if necessary.

Also, since TypeError is not a DOMException, and you have already converted the
error callback to take DOMException. At this point, would it even be possible to
pass a real TypeError here? A DOMException that just has the name "TypeError"
would be a very naughty workaround.

[Guido Urdaneta, 2016-02-26 14:23:52]

> Until the usage data for RTCPeerConnectionCreateOfferLegacyConstraints and
> RTCPeerConnectionCreateAnswerLegacyConstraints becomes clear when M49 reaches
> stable, I wouldn't assume that removing support for these constraints is
> actually possible. The pre-stable trend for createOffer in particular is not
> encouraging.
> 

Correct. 
I'm setting my hopes on people migrating to the promise-based version, which
will not support constraints.


> If none of the current TypeError cases would throw if this were all in WebIDL
in
> a hypothetical future, then simply not throwing really looks like the best
path
> to me. It takes us directly to a state that is both very unlikely to cause
> problems with existing sites and one that could be maintained over the long
term
> if necessary.
> 
> Also, since TypeError is not a DOMException, and you have already converted
the
> error callback to take DOMException. At this point, would it even be possible
to
> pass a real TypeError here? A DOMException that just has the name "TypeError"
> would be a very naughty workaround.

I was thinking about passing an OperationError, which is the only error
createOffer and createAnswer are supposed to return.
The stronger checks performed by MediaConstraintsImpl could be seen as a step
beyond normal type checking and would probably need to still be in place even if
we used a regular dictionary.

[philipj_slow, 2016-02-26 15:12:31]

On 2016/02/26 14:23:52, Guido Urdaneta wrote:
> > Until the usage data for RTCPeerConnectionCreateOfferLegacyConstraints and
> > RTCPeerConnectionCreateAnswerLegacyConstraints becomes clear when M49
reaches
> > stable, I wouldn't assume that removing support for these constraints is
> > actually possible. The pre-stable trend for createOffer in particular is not
> > encouraging.
> > 
> 
> Correct. 
> I'm setting my hopes on people migrating to the promise-based version, which
> will not support constraints.
> 
> 
> > If none of the current TypeError cases would throw if this were all in
WebIDL
> in
> > a hypothetical future, then simply not throwing really looks like the best
> path
> > to me. It takes us directly to a state that is both very unlikely to cause
> > problems with existing sites and one that could be maintained over the long
> term
> > if necessary.
> > 
> > Also, since TypeError is not a DOMException, and you have already converted
> the
> > error callback to take DOMException. At this point, would it even be
possible
> to
> > pass a real TypeError here? A DOMException that just has the name
"TypeError"
> > would be a very naughty workaround.
> 
> I was thinking about passing an OperationError, which is the only error
> createOffer and createAnswer are supposed to return.
> The stronger checks performed by MediaConstraintsImpl could be seen as a step
> beyond normal type checking and would probably need to still be in place even
if
> we used a regular dictionary.

I think these checks would actually be impossible to add if these were regular
WebIDL dictionaries. If bindings didn't throw a TypeError, then what the
internal code sees would just be an int for offerToReceiveVideo and
offerToReceiveAudio, and in the case of parseOldStyleNames, any unrecognized
dictionary members would simply have been ignored, there's no way to iterate the
"extra" members and emit errors/warnings for them once you are past the
generated bindings.

Just to spell it out, what is your main reservation against dropping the
TypeError cases? Does it look like a complicated/risky change, or do you think
that there's value in emitting these errors that would justify sticking with
custom handling indefinitely? Something else? I'm just trying to assume the
worst case and having as few web-exposed "steps" as possible, as there's a (very
small) risk of getting stuck at each step.

[Guido Urdaneta, 2016-02-26 16:26:15]

On 2016/02/26 15:12:31, philipj_UTC7 wrote:
> On 2016/02/26 14:23:52, Guido Urdaneta wrote:
> > > Until the usage data for RTCPeerConnectionCreateOfferLegacyConstraints and
> > > RTCPeerConnectionCreateAnswerLegacyConstraints becomes clear when M49
> reaches
> > > stable, I wouldn't assume that removing support for these constraints is
> > > actually possible. The pre-stable trend for createOffer in particular is
not
> > > encouraging.
> > > 
> > 
> > Correct. 
> > I'm setting my hopes on people migrating to the promise-based version, which
> > will not support constraints.
> > 
> > 
> > > If none of the current TypeError cases would throw if this were all in
> WebIDL
> > in
> > > a hypothetical future, then simply not throwing really looks like the best
> > path
> > > to me. It takes us directly to a state that is both very unlikely to cause
> > > problems with existing sites and one that could be maintained over the
long
> > term
> > > if necessary.
> > > 
> > > Also, since TypeError is not a DOMException, and you have already
converted
> > the
> > > error callback to take DOMException. At this point, would it even be
> possible
> > to
> > > pass a real TypeError here? A DOMException that just has the name
> "TypeError"
> > > would be a very naughty workaround.
> > 
> > I was thinking about passing an OperationError, which is the only error
> > createOffer and createAnswer are supposed to return.
> > The stronger checks performed by MediaConstraintsImpl could be seen as a
step
> > beyond normal type checking and would probably need to still be in place
even
> if
> > we used a regular dictionary.
> 
> I think these checks would actually be impossible to add if these were regular
> WebIDL dictionaries. If bindings didn't throw a TypeError, then what the
> internal code sees would just be an int for offerToReceiveVideo and
> offerToReceiveAudio, and in the case of parseOldStyleNames, any unrecognized
> dictionary members would simply have been ignored, there's no way to iterate
the
> "extra" members and emit errors/warnings for them once you are past the
> generated bindings.
> 
> Just to spell it out, what is your main reservation against dropping the
> TypeError cases? Does it look like a complicated/risky change, or do you think
> that there's value in emitting these errors that would justify sticking with
> custom handling indefinitely? Something else? I'm just trying to assume the
> worst case and having as few web-exposed "steps" as possible, as there's a
(very
> small) risk of getting stuck at each step.

I have no concerns with offerToReceiveAudio/Video, as the change is simple and
the lower layer will handle without problem the values that were previously
rejected.
My concern is with constraints. To avoid emitting errors with constraints, we
need to either make a new parser that does not emit errors, or make a nontrivial
change to the current parser so that it does not emit errors. In the latter
case, we have to make sure this does not break UserMediaRequest, which also
relies on this parser. 
I think both options are riskier/more complicated than continuing to emit
errors, even if we now have to report them as OperationErrors via the error
callback instead of TypeErrors.

[philipj_slow, 2016-02-26 18:25:54]

On 2016/02/26 16:26:15, Guido Urdaneta wrote:
> On 2016/02/26 15:12:31, philipj_UTC7 wrote:
> > On 2016/02/26 14:23:52, Guido Urdaneta wrote:
> > > > Until the usage data for RTCPeerConnectionCreateOfferLegacyConstraints
and
> > > > RTCPeerConnectionCreateAnswerLegacyConstraints becomes clear when M49
> > reaches
> > > > stable, I wouldn't assume that removing support for these constraints is
> > > > actually possible. The pre-stable trend for createOffer in particular is
> not
> > > > encouraging.
> > > > 
> > > 
> > > Correct. 
> > > I'm setting my hopes on people migrating to the promise-based version,
which
> > > will not support constraints.
> > > 
> > > 
> > > > If none of the current TypeError cases would throw if this were all in
> > WebIDL
> > > in
> > > > a hypothetical future, then simply not throwing really looks like the
best
> > > path
> > > > to me. It takes us directly to a state that is both very unlikely to
cause
> > > > problems with existing sites and one that could be maintained over the
> long
> > > term
> > > > if necessary.
> > > > 
> > > > Also, since TypeError is not a DOMException, and you have already
> converted
> > > the
> > > > error callback to take DOMException. At this point, would it even be
> > possible
> > > to
> > > > pass a real TypeError here? A DOMException that just has the name
> > "TypeError"
> > > > would be a very naughty workaround.
> > > 
> > > I was thinking about passing an OperationError, which is the only error
> > > createOffer and createAnswer are supposed to return.
> > > The stronger checks performed by MediaConstraintsImpl could be seen as a
> step
> > > beyond normal type checking and would probably need to still be in place
> even
> > if
> > > we used a regular dictionary.
> > 
> > I think these checks would actually be impossible to add if these were
regular
> > WebIDL dictionaries. If bindings didn't throw a TypeError, then what the
> > internal code sees would just be an int for offerToReceiveVideo and
> > offerToReceiveAudio, and in the case of parseOldStyleNames, any unrecognized
> > dictionary members would simply have been ignored, there's no way to iterate
> the
> > "extra" members and emit errors/warnings for them once you are past the
> > generated bindings.
> > 
> > Just to spell it out, what is your main reservation against dropping the
> > TypeError cases? Does it look like a complicated/risky change, or do you
think
> > that there's value in emitting these errors that would justify sticking with
> > custom handling indefinitely? Something else? I'm just trying to assume the
> > worst case and having as few web-exposed "steps" as possible, as there's a
> (very
> > small) risk of getting stuck at each step.
> 
> I have no concerns with offerToReceiveAudio/Video, as the change is simple and
> the lower layer will handle without problem the values that were previously
> rejected.
> My concern is with constraints. To avoid emitting errors with constraints, we
> need to either make a new parser that does not emit errors, or make a
nontrivial
> change to the current parser so that it does not emit errors. In the latter
> case, we have to make sure this does not break UserMediaRequest, which also
> relies on this parser. 
> I think both options are riskier/more complicated than continuing to emit
> errors, even if we now have to report them as OperationErrors via the error
> callback instead of TypeErrors.

Isn't it a trivial change? There seem to be only two places that emit errors,
one of them for testing and one to fail on unrecognized options. It *seems* to
be as simple as removing those lines, is there anything more to it?

[Guido Urdaneta, 2016-02-29 12:50:03]

Description was changed from

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void> in M51.

BUG=499126
==========

to

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void>.

BUG=499126
==========

[Guido Urdaneta, 2016-02-29 13:29:23]

> Isn't it a trivial change? There seem to be only two places that emit errors,
> one of them for testing and one to fail on unrecognized options. It *seems* to
> be as simple as removing those lines, is there anything more to it?

I don't think it's a trivial change. It would imply changing most places with a
"return false" statement (and their call sites) in MediaConstratinsImpl so that
parsing continues without returning errors.
Such a change would also have an impact on the behavior of getUserMedia, which
relies on the same constraints parser.

[philipj_slow, 2016-02-29 17:13:54]

On 2016/02/29 13:29:23, Guido Urdaneta wrote:
> > Isn't it a trivial change? There seem to be only two places that emit
errors,
> > one of them for testing and one to fail on unrecognized options. It *seems*
to
> > be as simple as removing those lines, is there anything more to it?
> 
> I don't think it's a trivial change. It would imply changing most places with
a
> "return false" statement (and their call sites) in MediaConstratinsImpl so
that
> parsing continues without returning errors.
> Such a change would also have an impact on the behavior of getUserMedia, which
> relies on the same constraints parser.

Ah, I see the source of my confusion now! I only spotted the
errorState.throwConstraintError calls, but overlooked the three
errorState.throwTypeError calls, two of "Malformed constraints object." and one
"Malformed constraint: Cannot use both optional/mandatory and specific
constraints."

That's a huge set of conditions for "return false", some of which could be
maintained if the dictionary were defined with WebIDL, some which could not, and
a lot that I can't tell at a quick glance.

How about getting rid of the easy errorState.throwConstraintError cases and
keeping all of errorState.throwTypeError cases for now? A subset of those
couldn't be preserved in generated bindings, but to sort that out it's probably
easiest to just try moving it to bindings, which I understand you hope won't be
necessary because it will be removed. I'm a pessimist on such matters, but let's
wait and see.

The remaining question would be what to do about the fact that what remains is
TypeError and not DOMException. You've suggested OperationError and that seems
OK. Some specs do throw TypeError in prose, but to do the equivalent of that
you'd need to change the signature of the error callback, so meh.

[Guido Urdaneta, 2016-02-29 18:49:56]

> How about getting rid of the easy errorState.throwConstraintError cases and
> keeping all of errorState.throwTypeError cases for now? 

Done. Fortunately, MediaConstraintsImpl already had a method to ignore
ConstraintErrors.

> A subset of those
> couldn't be preserved in generated bindings, but to sort that out it's
probably
> easiest to just try moving it to bindings, which I understand you hope won't
be
> necessary because it will be removed. I'm a pessimist on such matters, but
let's
> wait and see.
> 

I agree with you that if we decide that we have to support Constraints
indefinitely then we should change the IDL and use the specific dictionary
types.
Like you say, let's first add the promise-based methods and see how it goes.


> The remaining question would be what to do about the fact that what remains is
> TypeError and not DOMException. You've suggested OperationError and that seems
> OK. Some specs do throw TypeError in prose, but to do the equivalent of that
> you'd need to change the signature of the error callback, so meh.

OperationError it is then.

[philipj_slow, 2016-03-01 04:53:06]

OK, almost there :)

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:104: class
ErrorCallbackTask : public WebTaskRunner::Task {
This class is gone, needs rebase.

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(mediaErrorState.canGenerateException()) {
It's a bit subtle why this works. Isn't it a bug in MediaErrorState.cpp that
canGenerateException returns false for ConstraintError when raiseException in
fact *can* generate an exception for it?

I was assuming you'd remove the error cases from MediaConstraintsImpl.cpp
entirely, then you could also remove those bits from MediaErrorState. Is there a
reason to avoid doing that?

[Guido Urdaneta, 2016-03-07 09:46:09]

On 2016/03/01 04:53:06, philipj_UTC7 wrote:
> OK, almost there :)
> 
>
https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
> (right):
> 
>
https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:104: class
> ErrorCallbackTask : public WebTaskRunner::Task {
> This class is gone, needs rebase.
> 
>
https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
> (mediaErrorState.canGenerateException()) {
> It's a bit subtle why this works. Isn't it a bug in MediaErrorState.cpp that
> canGenerateException returns false for ConstraintError when raiseException in
> fact *can* generate an exception for it?
> 
> I was assuming you'd remove the error cases from MediaConstraintsImpl.cpp
> entirely, then you could also remove those bits from MediaErrorState. Is there
a
> reason to avoid doing that?

I was assuming the behavior of canGenerateException() was a feature instead of a
bug :)
hta@, can you clarify?

[hta - Chromium, 2016-03-07 15:15:47]

Tried to explain why the presence of the "constraint name" extension in
constrainteerror forces some convolution in how we generate callback/reject
arguments.

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(mediaErrorState.canGenerateException()) {
On 2016/03/01 04:53:05, philipj_UTC7 wrote:
> It's a bit subtle why this works. Isn't it a bug in MediaErrorState.cpp that
> canGenerateException returns false for ConstraintError when raiseException in
> fact *can* generate an exception for it?
> 
> I was assuming you'd remove the error cases from MediaConstraintsImpl.cpp
> entirely, then you could also remove those bits from MediaErrorState. Is there
a
> reason to avoid doing that?

The reason for canGenerateException is that the DOMException class can't be used
for a constraintError - it doesn't have a constraintname field, and isn't
extensible to add one. However, the error callback *can* pass a constraintError,
which has the constraintName field.

The spec says that the name of the failed constraint has to be reported. So we
must use a mechanism that is able to pass it - ie the callback. (We can also
reject the promise with a constraintError, given the lack of strong typing on
promise rejection - up to you whether you consider that an ECMAscript bug or a
feature.)

But we can't use DOMException::create() to create the error argument.

[philipj_slow, 2016-03-09 13:15:31]

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(mediaErrorState.canGenerateException()) {
On 2016/03/07 15:15:47, hta - Chromium wrote:
> On 2016/03/01 04:53:05, philipj_UTC7 wrote:
> > It's a bit subtle why this works. Isn't it a bug in MediaErrorState.cpp that
> > canGenerateException returns false for ConstraintError when raiseException
in
> > fact *can* generate an exception for it?
> > 
> > I was assuming you'd remove the error cases from MediaConstraintsImpl.cpp
> > entirely, then you could also remove those bits from MediaErrorState. Is
there
> a
> > reason to avoid doing that?
> 
> The reason for canGenerateException is that the DOMException class can't be
used
> for a constraintError - it doesn't have a constraintname field, and isn't
> extensible to add one. However, the error callback *can* pass a
constraintError,
> which has the constraintName field.
> 
> The spec says that the name of the failed constraint has to be reported. So we
> must use a mechanism that is able to pass it - ie the callback. (We can also
> reject the promise with a constraintError, given the lack of strong typing on
> promise rejection - up to you whether you consider that an ECMAscript bug or a
> feature.)
> 
> But we can't use DOMException::create() to create the error argument.

OK, so this is related to NavigatorUserMediaError and the fact that
MediaErrorState is also used for getUserMedia().

Maybe an isContraintError() and createConstraintError() pair would make this
more obvious at the call sites.

I guess changing getUserMedia at the same time would be needless risk, but can
you comment which errors this is suppressing and what "canGenerateException" is
used to mean in this context?

[Guido Urdaneta, 2016-03-09 15:04:35]

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:104: class
ErrorCallbackTask : public WebTaskRunner::Task {
On 2016/03/01 04:53:05, philipj_UTC7 wrote:
> This class is gone, needs rebase.

Done.

https://codereview.chromium.org/1713953002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:483: if
(mediaErrorState.canGenerateException()) {
On 2016/03/09 13:15:31, philipj_UTC7 wrote:
> On 2016/03/07 15:15:47, hta - Chromium wrote:
> > On 2016/03/01 04:53:05, philipj_UTC7 wrote:
> > > It's a bit subtle why this works. Isn't it a bug in MediaErrorState.cpp
that
> > > canGenerateException returns false for ConstraintError when raiseException
> in
> > > fact *can* generate an exception for it?
> > > 
> > > I was assuming you'd remove the error cases from MediaConstraintsImpl.cpp
> > > entirely, then you could also remove those bits from MediaErrorState. Is
> there
> > a
> > > reason to avoid doing that?
> > 
> > The reason for canGenerateException is that the DOMException class can't be
> used
> > for a constraintError - it doesn't have a constraintname field, and isn't
> > extensible to add one. However, the error callback *can* pass a
> constraintError,
> > which has the constraintName field.
> > 
> > The spec says that the name of the failed constraint has to be reported. So
we
> > must use a mechanism that is able to pass it - ie the callback. (We can also
> > reject the promise with a constraintError, given the lack of strong typing
on
> > promise rejection - up to you whether you consider that an ECMAscript bug or
a
> > feature.)
> > 
> > But we can't use DOMException::create() to create the error argument.
> 
> OK, so this is related to NavigatorUserMediaError and the fact that
> MediaErrorState is also used for getUserMedia().
> 
> Maybe an isContraintError() and createConstraintError() pair would make this
> more obvious at the call sites.
> 
> I guess changing getUserMedia at the same time would be needless risk, but can
> you comment which errors this is suppressing and what "canGenerateException"
is
> used to mean in this context?

Done.

[philipj_slow, 2016-03-10 15:53:00]

philipj@opera.com changed reviewers:
+ tommi@chromium.org

[philipj_slow, 2016-03-10 15:53:01]

Non-module-owner LGTM. Adding tommi@ for owners review.

https://codereview.chromium.org/1713953002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (left):

https://codereview.chromium.org/1713953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:446:
Deprecation::countDeprecation(context,
UseCounter::RTCPeerConnectionCreateOfferLegacyNoFailureCallback);
Can you remove RTCPeerConnectionCreateOfferLegacyNoFailureCallback from
UseCounter.h and Deprecation.cpp as well?

https://codereview.chromium.org/1713953002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:488:
Deprecation::countDeprecation(context,
UseCounter::RTCPeerConnectionCreateAnswerLegacyNoFailureCallback);
RTCPeerConnectionCreateAnswerLegacyNoFailureCallback too.

[tommi (sloooow) - chröme, 2016-03-10 22:01:53]

lgtm

https://codereview.chromium.org/1713953002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-createOffer.html
(right):

https://codereview.chromium.org/1713953002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-createOffer.html:22:
function  expectedCreateOfferFailed7(error)
nit: fix whitespace. same below.

[Guido Urdaneta, 2016-03-11 07:49:31]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-03-11 07:49:32]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1713953002/#ps140001
(title: "Fix whitespace")

[commit-bot: I haz the power, 2016-03-11 07:49:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713953002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713953002/140001

[commit-bot: I haz the power, 2016-03-11 07:52:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 07:52:36]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Guido Urdaneta, 2016-03-11 08:37:29]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2016-03-11 08:37:30]

The patchset sent to the CQ was uploaded after l-g-t-m from philipj@opera.com,
tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1713953002/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-11 08:37:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713953002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713953002/160001

[Guido Urdaneta, 2016-03-11 08:39:06]

https://codereview.chromium.org/1713953002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-createOffer.html
(right):

https://codereview.chromium.org/1713953002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-createOffer.html:22:
function  expectedCreateOfferFailed7(error)
On 2016/03/10 22:01:52, tommi-chromium wrote:
> nit: fix whitespace. same below.

Done.

[commit-bot: I haz the power, 2016-03-11 10:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 10:59:03]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Guido Urdaneta, 2016-03-11 11:00:32]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2016-03-11 11:00:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1713953002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1713953002/160001

[commit-bot: I haz the power, 2016-03-11 13:00:40]

Description was changed from

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void>.

BUG=499126
==========

to

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void>.

BUG=499126
==========

[commit-bot: I haz the power, 2016-03-11 13:00:42]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-03-11 13:02:02]

Description was changed from

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void>.

BUG=499126
==========

to

==========
Report errors in RTCPeerConnection legacy functions via the the failure callback
instead of exceptions.

This makes createAnswer() and createOffer() more compliant with the spec and
will reduce unexpected surprises when their return value changes to
Promise<void>.

BUG=499126

Committed: https://crrev.com/e332af368744cceb112fcd228f74a53b381acec2
Cr-Commit-Position: refs/heads/master@{#380614}
==========

[commit-bot: I haz the power, 2016-03-11 13:02:03]

Patchset 9 (id:??) landed as
https://crrev.com/e332af368744cceb112fcd228f74a53b381acec2
Cr-Commit-Position: refs/heads/master@{#380614}