Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

BUG=webrtc:5222
Review-Url: https://codereview.chromium.org/1542443002
Cr-Commit-Position: refs/heads/master@{#459241}
Committed: https://chromium.googlesource.com/chromium/src/+/b9782202a54cad6bc4e03610cd2ec1cadcb9a02b

[joachim, 2017-03-13 19:26:11]

Description was changed from

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

NOTE: This needs https://codereview.webrtc.org/1528843005/ to land and
roll into Chromium first.

BUG=webrtc:5222
==========

to

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

NOTE: This needs https://codereview.webrtc.org/2743353002/ to land and
roll into Chromium first.

BUG=webrtc:5222
==========

[joachim, 2017-03-14 21:47:33]

jbauch@webrtc.org changed reviewers:
+ juberti@chromium.org

[joachim, 2017-03-14 21:47:34]

In preparation for when the libsrtp change has rolled, here is a CL to allow
enabling GCM for WebRTC in Chromium.

juberti@ could you please check the wording of the flag ("enable-webrtc-gcm")
and the description? I'm assuming you want to have the feature behind a flag
first so people can test first.

I will then add OWNERS of the various components for reviewing the changes.

[joachim, 2017-03-16 14:38:53]

Description was changed from

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

NOTE: This needs https://codereview.webrtc.org/2743353002/ to land and
roll into Chromium first.

BUG=webrtc:5222
==========

to

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

BUG=webrtc:5222
==========

[joachim, 2017-03-16 23:18:34]

On 2017/03/14 21:47:34, joachim wrote:
> In preparation for when the libsrtp change has rolled, here is a CL to allow
> enabling GCM for WebRTC in Chromium.
> 
> juberti@ could you please check the wording of the flag ("enable-webrtc-gcm")
> and the description? I'm assuming you want to have the feature behind a flag
> first so people can test first.
> 
> I will then add OWNERS of the various components for reviewing the changes.

Ping?

[joachim, 2017-03-20 19:42:26]

jbauch@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[joachim, 2017-03-20 19:42:27]

+pthatcher, maybe you can provide some initial feedback in case juberti is busy?

[juberti, 2017-03-21 05:37:06]

juberti@google.com changed reviewers:
+ juberti@google.com

[juberti, 2017-03-21 05:37:08]

sorry, didn't realize this was waiting on me

https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
content/public/common/content_switches.cc:928: const char kEnableWebRtcGcm[] =
"enable-webrtc-gcm";
call this enable-webrtc-srtp-aes-gcm (GCM is an overloaded term)

https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:265: if
(cmd_line->HasSwitch(switches::kEnableWebRtcGcm)) {
I prefer the style in the lines above, e.g. x = HasSwitch(y)

https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <int value="243655727"
label="enable-webrtc-gcm"/>
see naming in other file

[pthatcher2, 2017-03-21 06:24:55]

I agree with Justin's comments and the rest looks good to me.

On 2017/03/21 05:37:08, juberti wrote:
> sorry, didn't realize this was waiting on me
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
> content/public/common/content_switches.cc:928: const char kEnableWebRtcGcm[] =
> "enable-webrtc-gcm";
> call this enable-webrtc-srtp-aes-gcm (GCM is an overloaded term)
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
> File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
> (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
> content/renderer/media/webrtc/peer_connection_dependency_factory.cc:265: if
> (cmd_line->HasSwitch(switches::kEnableWebRtcGcm)) {
> I prefer the style in the lines above, e.g. x = HasSwitch(y)
> 
>
https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:99422: +  <int value="243655727"
> label="enable-webrtc-gcm"/>
> see naming in other file

[joachim, 2017-03-21 21:56:58]

All changed, ptal.

https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
content/public/common/content_switches.cc:928: const char kEnableWebRtcGcm[] =
"enable-webrtc-gcm";
On 2017/03/21 05:37:07, juberti wrote:
> call this enable-webrtc-srtp-aes-gcm (GCM is an overloaded term)

Done.

https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:265: if
(cmd_line->HasSwitch(switches::kEnableWebRtcGcm)) {
On 2017/03/21 05:37:07, juberti wrote:
> I prefer the style in the lines above, e.g. x = HasSwitch(y)

Done.

https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:99422: +  <int value="243655727"
label="enable-webrtc-gcm"/>
On 2017/03/21 05:37:07, juberti wrote:
> see naming in other file

Done.

[joachim, 2017-03-22 17:34:08]

On 2017/03/21 21:56:58, joachim wrote:
> All changed, ptal.
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/public/common/c...
> content/public/common/content_switches.cc:928: const char kEnableWebRtcGcm[] =
> "enable-webrtc-gcm";
> On 2017/03/21 05:37:07, juberti wrote:
> > call this enable-webrtc-srtp-aes-gcm (GCM is an overloaded term)
> 
> Done.
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
> File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
> (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/content/renderer/media/...
> content/renderer/media/webrtc/peer_connection_dependency_factory.cc:265: if
> (cmd_line->HasSwitch(switches::kEnableWebRtcGcm)) {
> On 2017/03/21 05:37:07, juberti wrote:
> > I prefer the style in the lines above, e.g. x = HasSwitch(y)
> 
> Done.
> 
>
https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1542443002/diff/40001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:99422: +  <int value="243655727"
> label="enable-webrtc-gcm"/>
> On 2017/03/21 05:37:07, juberti wrote:
> > see naming in other file
> 
> Done.

Just checking back if you are fine with the updated wording, or if I should
change something else before I add others for OWNER review.

[pthatcher2, 2017-03-23 00:41:04]

pthatcher@chromium.org changed reviewers:
+ pthatcher@chromium.org

[pthatcher2, 2017-03-23 00:41:05]

pthatcher@chromium.org changed reviewers:
+ pthatcher@chromium.org

[pthatcher2, 2017-03-23 00:41:05]

I think you should proceed with OWNERS.  It looks find to me, and I'm guessing
it will be to Justin as well.

[pthatcher2, 2017-03-23 00:41:06]

I think you should proceed with OWNERS.  It looks find to me, and I'm guessing
it will be to Justin as well.

[joachim, 2017-03-23 00:51:23]

jbauch@webrtc.org changed reviewers:
+ creis@chromium.org, grt@chromium.org, jwd@chromium.org, sergeyu@chromium.org

[joachim, 2017-03-23 00:51:24]

Thanks.

+grt for chrome/app/ and chrome/browser/
+sergeyu for content/renderer/media/webrtc/
+creis for content/browser/ and content/public/
+jwd for tools/metrics/

Ptal for OWNER review.

[Sergey Ulanov, 2017-03-23 01:04:40]

lgtm

[grt (UTC plus 2), 2017-03-23 10:59:18]

chrome/app and chrome/browser lgtm w/ the addition below

https://codereview.chromium.org/1542443002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1542443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5505: +      <message
name="IDS_FLAGS_WEBRTC_SRTP_AES_GCM_NAME" desc="Name of chrome:flags option to
enable GCM cipher suites for WebRTC">
translateable="false" here and below

[jwd, 2017-03-23 14:42:51]

histograms LGTM

[Charlie Reis, 2017-03-23 16:21:28]

content/ LGTM with nit.

https://codereview.chromium.org/1542443002/diff/60001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1542443002/diff/60001/content/public/common/c...
content/public/common/content_switches.cc:927: // Enables negotiation of GCM
cipher suites for SRTP in WebRTC.
Maybe give a link to the RFC here, to help people decode these acronyms?

[joachim, 2017-03-23 20:07:11]

All changed, thanks for your review.

https://codereview.chromium.org/1542443002/diff/60001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1542443002/diff/60001/chrome/app/generated_re...
chrome/app/generated_resources.grd:5505: +      <message
name="IDS_FLAGS_WEBRTC_SRTP_AES_GCM_NAME" desc="Name of chrome:flags option to
enable GCM cipher suites for WebRTC">
On 2017/03/23 10:59:18, grt (UTC plus 1) wrote:
> translateable="false" here and below

Done.

https://codereview.chromium.org/1542443002/diff/60001/content/public/common/c...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1542443002/diff/60001/content/public/common/c...
content/public/common/content_switches.cc:927: // Enables negotiation of GCM
cipher suites for SRTP in WebRTC.
On 2017/03/23 16:21:27, Charlie Reis (OOO soon) wrote:
> Maybe give a link to the RFC here, to help people decode these acronyms?

Done.

[joachim, 2017-03-23 20:08:15]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2017-03-23 20:08:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, jwd@chromium.org, creis@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1542443002/#ps80001
(title: "Updates based on feedback from OWNERS.")

[commit-bot: I haz the power, 2017-03-23 20:08:51]

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-03-23 20:12:30]

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

[commit-bot: I haz the power, 2017-03-23 20:12:31]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[joachim, 2017-03-23 20:24:55]

The CQ bit was checked by jbauch@webrtc.org

[joachim, 2017-03-23 20:24:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, jwd@chromium.org, creis@chromium.org, grt@chromium.org
Link to the patchset: https://codereview.chromium.org/1542443002/#ps100001
(title: "Rebased.")

[commit-bot: I haz the power, 2017-03-23 20:25:35]

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-03-23 21:57:14]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490300695888620,
"parent_rev": "bbe38f7faddf00596f655c1d66d65814784ed642", "commit_rev":
"b9782202a54cad6bc4e03610cd2ec1cadcb9a02b"}

[commit-bot: I haz the power, 2017-03-23 21:58:42]

Description was changed from

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

BUG=webrtc:5222
==========

to

==========
Add commandline flag to enable GCM cipher suites from RFC 7714 in WebRTC.

This CL adds a new commandline flag and chrome://flags option to enable
GCM cipher suites as defined in RFC 7714 in WebRTC.

BUG=webrtc:5222

Review-Url: https://codereview.chromium.org/1542443002
Cr-Commit-Position: refs/heads/master@{#459241}
Committed:
https://chromium.googlesource.com/chromium/src/+/b9782202a54cad6bc4e03610cd2e...
==========

[commit-bot: I haz the power, 2017-03-23 21:58:44]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/b9782202a54cad6bc4e03610cd2e...

[juberti, 2017-03-24 04:59:49]

lgtm
(belated)