Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent to Implement and Ship setConfiguration: 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wh74Pd37LUQ/45Dx81Y2BgAJ

Intent to Deprecate and Remove updateIce:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqCAgAJ

BUG=chromium:587453
Review-Url: https://codereview.chromium.org/2631433002
Cr-Commit-Position: refs/heads/master@{#446512}
Committed: https://chromium.googlesource.com/chromium/src/+/9853d6c5883e860ff52b5447b17f50907ffee434

[Taylor_Brandstetter, 2017-01-12 02:02:27]

Description was changed from

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

BUG=chromium:587453
==========

to

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent thread: 
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/setconfig...

BUG=chromium:587453
==========

[Taylor_Brandstetter, 2017-01-12 02:10:27]

deadbeef@chromium.org changed reviewers:
+ dcheng@chromium.org, guidou@chromium.org, perkj@chromium.org

[Taylor_Brandstetter, 2017-01-12 02:10:29]

dcheng@chromium.org: Please review changes in third_party/WebKit/public/

perkj@chromium.org: Please review changes in content/renderer/media/

guidou@chromium.org: Please review changes in
third_party/WebKit/Source/modules/peerconnection

[perkj_chrome, 2017-01-12 08:56:19]

Can you please add a content_browsertest or blink layout test for this as well
please.

https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:194: default:
nit: remove default to make sure this has to be updated if a new type is added.

[Taylor_Brandstetter, 2017-01-12 20:32:20]

On 2017/01/12 08:56:19, perkj_chrome wrote:
> Can you please add a content_browsertest or blink layout test for this as well
> please.
> 
>
https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
> File content/renderer/media/rtc_peer_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
> content/renderer/media/rtc_peer_connection_handler.cc:194: default:
> nit: remove default to make sure this has to be updated if a new type is
added.

I tried adding a layout test, but I didn't realize that they use a mock peer
connection implementation, which won't let me test error conditions or anything
meaningful that setConfiguration does (like triggering an ICE restart in the
next offer).

Does this just mean I should use a content_browsertest instead?

[Taylor_Brandstetter, 2017-01-12 21:53:41]

On 2017/01/12 20:32:20, Taylor_Brandstetter wrote:
> On 2017/01/12 08:56:19, perkj_chrome wrote:
> > Can you please add a content_browsertest or blink layout test for this as
well
> > please.
> > 
> >
>
https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
> > content/renderer/media/rtc_peer_connection_handler.cc:194: default:
> > nit: remove default to make sure this has to be updated if a new type is
> added.
> 
> I tried adding a layout test, but I didn't realize that they use a mock peer
> connection implementation, which won't let me test error conditions or
anything
> meaningful that setConfiguration does (like triggering an ICE restart in the
> next offer).
> 
> Does this just mean I should use a content_browsertest instead?

Actually, would it be better to add a web platform test, since that will be
needed eventually anyway for WebRTC's standardization? 

https://github.com/w3c/web-platform-tests/tree/master/webrtc

[perkj_chrome, 2017-01-13 09:09:56]

Guido- Do you know that the best way is to test this?- See Taylors question.

[perkj_chrome, 2017-01-13 09:14:06]

lgtm % comment and agree with Guido how to test from js.

https://codereview.chromium.org/2631433002/diff/40001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/40001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:198: DCHECK(false) <<
"webrtc::RTCErrorType " << webrtc_error.type() <<
NOTREACHED()

[Guido Urdaneta, 2017-01-13 15:01:59]

To test the non-Blink parts of the code from JS you need a content_browsertest.
Layout and web platform tests use test_runner instead of content, so they are
not good for testing WebRTC behavior, but only the behavior of the JS API.
content_browsertests do use content.

[Taylor_Brandstetter, 2017-01-13 19:14:18]

deadbeef@chromium.org changed reviewers:
+ tommyw@chromium.org

[Taylor_Brandstetter, 2017-01-13 19:14:19]

Added content_browsertest. perkj, can you please review the test?

tommyw@chromium.org: Please review changes in test_runner.

https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_peer_connection_handler.cc:194: default:
On 2017/01/12 08:56:19, perkj_chrome wrote:
> nit: remove default to make sure this has to be updated if a new type is
added.

Done.

[dcheng, 2017-01-13 23:10:38]

https://codereview.chromium.org/2631433002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h (right):

https://codereview.chromium.org/2631433002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h:76: virtual
bool setConfiguration(const WebRTCConfiguration&, WebRTCError&) = 0;
My personal feeling is it'd feel natural for this to just return WebRTCErrorType
directly, rather than having to keep the bool and error enum in sync. WDYT?

[Taylor_Brandstetter, 2017-01-14 00:08:40]

https://codereview.chromium.org/2631433002/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h (right):

https://codereview.chromium.org/2631433002/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebRTCPeerConnectionHandler.h:76: virtual
bool setConfiguration(const WebRTCConfiguration&, WebRTCError&) = 0;
On 2017/01/13 23:10:38, dcheng wrote:
> My personal feeling is it'd feel natural for this to just return
WebRTCErrorType
> directly, rather than having to keep the bool and error enum in sync. WDYT?

You're right. It's an output parameter in webrtc mainly for backwards
compatibility, but we don't have that restriction here. Done.

[perkj_chrome, 2017-01-17 09:08:01]

Sorry about the delay. Forgot about this yesterday. 
still lgtm

Note that Tommy W is no longer working on blink.

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
File content/test/data/media/peerconnection-setConfiguration.html (right):

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
content/test/data/media/peerconnection-setConfiguration.html:29: // 
TODO(deadbeef): Once onicegatheringstatechange is implemented, use that
? void RTCPeerConnectionHandler::OnIceGatheringChange

Looks implemented to me? Is the event missing in blink?

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
content/test/data/media/peerconnection-setConfiguration.html:107:
rtcpMuxPolicy:'require', certificates:[]});
end with setting a valid configuration to verify that there is nothing else that
cause the above exceptions?

[dcheng, 2017-01-17 09:32:03]

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1452: DCHECK(ret ==
(webrtc_error.type() == webrtc::RTCErrorType::NONE));
Nit: DCHECK_EQ

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1454: return
blink_error.type();
I think we might not need this helper class anymore as well.

[Taylor_Brandstetter, 2017-01-17 18:56:19]

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1452: DCHECK(ret ==
(webrtc_error.type() == webrtc::RTCErrorType::NONE));
On 2017/01/17 09:32:03, dcheng wrote:
> Nit: DCHECK_EQ

Done.

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1454: return
blink_error.type();
On 2017/01/17 09:32:02, dcheng wrote:
> I think we might not need this helper class anymore as well.

I expect to use it in the future, even if it didn't end up needed by this CL.
For example, WebRTCVoidRequest should switch to using a WebRTCError instead of a
WebString to represent errors.

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
File content/test/data/media/peerconnection-setConfiguration.html (right):

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
content/test/data/media/peerconnection-setConfiguration.html:29: // 
TODO(deadbeef): Once onicegatheringstatechange is implemented, use that
On 2017/01/17 09:08:01, perkj_chrome wrote:
> ? void RTCPeerConnectionHandler::OnIceGatheringChange
> 
> Looks implemented to me? Is the event missing in blink? 

Yes:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/peerco...

It was a surprise to me too. I filed a bug/feature request for it.

https://codereview.chromium.org/2631433002/diff/100001/content/test/data/medi...
content/test/data/media/peerconnection-setConfiguration.html:107:
rtcpMuxPolicy:'require', certificates:[]});
On 2017/01/17 09:08:01, perkj_chrome wrote:
> end with setting a valid configuration to verify that there is nothing else
that
> cause the above exceptions? 

Done.

[Taylor_Brandstetter, 2017-01-17 18:56:38]

deadbeef@chromium.org changed reviewers:
- tommyw@chromium.org

[Taylor_Brandstetter, 2017-01-17 18:57:27]

deadbeef@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[Taylor_Brandstetter, 2017-01-17 18:57:27]

dmazzoni@chromium.org: Please review changes in test_runner/

[dcheng, 2017-01-17 23:32:59]

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler.cc:1454: return
blink_error.type();
On 2017/01/17 18:56:19, Taylor_Brandstetter wrote:
> On 2017/01/17 09:32:02, dcheng wrote:
> > I think we might not need this helper class anymore as well.
> 
> I expect to use it in the future, even if it didn't end up needed by this CL.
> For example, WebRTCVoidRequest should switch to using a WebRTCError instead of
a
> WebString to represent errors.

Perhaps WebRTCVoidRequest can just use the enum type directly as well?

[Taylor_Brandstetter, 2017-01-17 23:43:09]

On 2017/01/17 23:32:59, dcheng wrote:
>
https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
> File content/renderer/media/rtc_peer_connection_handler.cc (right):
> 
>
https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
> content/renderer/media/rtc_peer_connection_handler.cc:1454: return
> blink_error.type();
> On 2017/01/17 18:56:19, Taylor_Brandstetter wrote:
> > On 2017/01/17 09:32:02, dcheng wrote:
> > > I think we might not need this helper class anymore as well.
> > 
> > I expect to use it in the future, even if it didn't end up needed by this
CL.
> > For example, WebRTCVoidRequest should switch to using a WebRTCError instead
of
> a
> > WebString to represent errors.
> 
> Perhaps WebRTCVoidRequest can just use the enum type directly as well?

There will be other information beyond an enum type in the future. Work is still
in progress, but see: https://w3c.github.io/webrtc-pc/#rtcerror-object

[dcheng, 2017-01-18 00:28:45]

On 2017/01/17 23:43:09, Taylor_Brandstetter wrote:
> On 2017/01/17 23:32:59, dcheng wrote:
> >
>
https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
> > File content/renderer/media/rtc_peer_connection_handler.cc (right):
> > 
> >
>
https://codereview.chromium.org/2631433002/diff/100001/content/renderer/media...
> > content/renderer/media/rtc_peer_connection_handler.cc:1454: return
> > blink_error.type();
> > On 2017/01/17 18:56:19, Taylor_Brandstetter wrote:
> > > On 2017/01/17 09:32:02, dcheng wrote:
> > > > I think we might not need this helper class anymore as well.
> > > 
> > > I expect to use it in the future, even if it didn't end up needed by this
> CL.
> > > For example, WebRTCVoidRequest should switch to using a WebRTCError
instead
> of
> > a
> > > WebString to represent errors.
> > 
> > Perhaps WebRTCVoidRequest can just use the enum type directly as well?
> 
> There will be other information beyond an enum type in the future. Work is
still
> in progress, but see: https://w3c.github.io/webrtc-pc/#rtcerror-object

OK, Blink LGTM. Consider linking to the spec for WebRTCError so it's easier for
future readers to reference the correct things.

[dmazzoni, 2017-01-18 16:25:21]

test_runner lgtm

[Guido Urdaneta, 2017-01-18 16:50:10]

WebKit/Source/modules/peerconnection lgtm

[Taylor_Brandstetter, 2017-01-25 19:45:54]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-01-25 19:45:55]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org
Link to the patchset: https://codereview.chromium.org/2631433002/#ps120001
(title: "Addressing comments.")

[commit-bot: I haz the power, 2017-01-25 19:46:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-25 20:54:31]

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

[commit-bot: I haz the power, 2017-01-25 20:54:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Taylor_Brandstetter, 2017-01-25 23:12:37]

deadbeef@chromium.org changed reviewers:
+ foolip@chromium.org
- dcheng@chromium.org, dmazzoni@chromium.org, guidou@chromium.org,
perkj@chromium.org

[Taylor_Brandstetter, 2017-01-25 23:12:38]

foolip: Can you please review LayoutTests changes? It's just a couple things
going from "FAIL" to "PASS".

[foolip, 2017-01-26 04:54:03]

Description was changed from

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent thread: 
https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/setconfig...

BUG=chromium:587453
==========

to

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent to Implement and Ship setConfiguration: 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wh74Pd37LUQ/45Dx81Y2...

Intent to Deprecate and Remove updateIce:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqC...

BUG=chromium:587453
==========

[foolip, 2017-01-26 04:57:07]

LayoutTests lgtm, also checked IDL and asked for 
V8RTCPeerConnection_UpdateIce_Method cleanup.

https://codereview.chromium.org/2631433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(left):

https://codereview.chromium.org/2631433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:126:
[Measure, CallWith=ExecutionContext, RaisesException] void updateIce(optional
RTCConfiguration configuration, optional Dictionary mediaConstraints);
I added a link to
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqC...
in the description to cover this. Can you also remove
V8RTCPeerConnection_UpdateIce_Method from UseCounter.h?

[Taylor_Brandstetter, 2017-01-26 18:23:15]

https://codereview.chromium.org/2631433002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl
(left):

https://codereview.chromium.org/2631433002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:126:
[Measure, CallWith=ExecutionContext, RaisesException] void updateIce(optional
RTCConfiguration configuration, optional Dictionary mediaConstraints);
On 2017/01/26 04:57:07, foolip_UTC7 wrote:
> I added a link to
>
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqC...
> in the description to cover this. Can you also remove
> V8RTCPeerConnection_UpdateIce_Method from UseCounter.h?

Done.

[Taylor_Brandstetter, 2017-01-26 18:23:29]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-01-26 18:23:30]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org,
guidou@chromium.org, dmazzoni@chromium.org, dcheng@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2631433002/#ps200001
(title: "Remove V8RTCPeerConnection_UpdateIce_Method from UseCounter.h")

[commit-bot: I haz the power, 2017-01-26 18:24:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 19:28:02]

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

[commit-bot: I haz the power, 2017-01-26 19:28:03]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Taylor_Brandstetter, 2017-01-26 22:49:17]

The CQ bit was checked by deadbeef@chromium.org

[Taylor_Brandstetter, 2017-01-26 22:49:18]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@chromium.org,
guidou@chromium.org, dmazzoni@chromium.org, foolip@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2631433002/#ps190020
(title: "Rebase.")

[commit-bot: I haz the power, 2017-01-26 22:50:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 01:05:50]

CQ is committing da patch.

Bot data: {"patchset_id": 190020, "attempt_start_ts": 1485470957864990,
"parent_rev": "be38048f78a75a9b4ad3e10e4881c0bc3c11ecb5", "commit_rev":
"9853d6c5883e860ff52b5447b17f50907ffee434"}

[commit-bot: I haz the power, 2017-01-27 01:07:36]

Description was changed from

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent to Implement and Ship setConfiguration: 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wh74Pd37LUQ/45Dx81Y2...

Intent to Deprecate and Remove updateIce:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqC...

BUG=chromium:587453
==========

to

==========
Rename RTCPeerConnection.updateIce to setConfiguration and make it work.

In addition to the renaming/basic wiring up, this CL adds a mechanism
for passing error information between webrtc/chromium and blink.

In this CL, it's only used for an InvalidModificationError.

Intent to Implement and Ship setConfiguration: 
https://groups.google.com/a/chromium.org/d/msg/blink-dev/wh74Pd37LUQ/45Dx81Y2...

Intent to Deprecate and Remove updateIce:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/9gq36Wl4Jwo/a8-rZOqC...

BUG=chromium:587453

Review-Url: https://codereview.chromium.org/2631433002
Cr-Commit-Position: refs/heads/master@{#446512}
Committed:
https://chromium.googlesource.com/chromium/src/+/9853d6c5883e860ff52b5447b17f...
==========

[commit-bot: I haz the power, 2017-01-27 01:07:38]

Committed patchset #13 (id:190020) as
https://chromium.googlesource.com/chromium/src/+/9853d6c5883e860ff52b5447b17f...