Add command line flag --disable-webrtc-encryption for dev and canary channels

Add command line flag --disable-webrtc-encryption for dev and canary channels.

It is also be available for "unknown" channel, e.g. unbranded/dev builds. The flag is ignored for beta and stable channels.

The purpose of the flag is for app developer to easier debug RTP media related problems in WebRTC.

https://code.google.com/p/webrtc/issues/detail?id=491

This CL is based on https://codereview.chromium.org/27131004/ by perkj@.

TEST= 1) Start Chrome with the flag --disable-webrtc-encryption. 2) Setup a call using apprtc.appspot.com. 3) Open the JS console and make sure the SDP offer don't contain 'a=crypto:'.
BUG=304024
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232855

[Henrik Grunell, 2013-10-23 15:30:32]

Please review this.

Joi, as owner of content/public and please also
chrome/browser/chrome_content_browser_client.cc.

Justin, confirm that enabling for dev and beta and not for "unknown" (e.g.
non-branded) channel is OK. If checking the channel fails for beta or stable it
will return unknown, so there's no way of telling the difference between a
failed check and a Chromium or dev build. Also, distros of Chromium will afaik
return unknown.

[Henrik Grunell, 2013-10-23 15:32:47]

Oh and another thing, this depends on a libjingle roll and the change in
content/renderer/media/rtc_peer_connection_handler.cc will be done differently.
No need to review that file.

[Ami GONE FROM CHROMIUM, 2013-10-23 15:55:51]

<drive-by>
Supporting this flag for dev builds seems useful to me, so I'd lobby for
replacing the dev||canary check with a !stable&&!beta check.  WDYT?

Also, exposing this in about:flags would give a good place to doco that it's
unavailable on stable/beta, and also make it easier to enable on android, for
testing.

[Henrik Grunell, 2013-10-23 16:03:06]

On 2013/10/23 15:55:51, Ami Fischman wrote:
> <drive-by>
> Supporting this flag for dev builds seems useful to me, so I'd lobby for
> replacing the dev||canary check with a !stable&&!beta check.  WDYT?

Agree it can be useful for dev builds. It was like so in patch set 1 but I
changed it for the reasons in my first message. Maybe allow it for that case if
a debug build?

> 
> Also, exposing this in about:flags would give a good place to doco that it's
> unavailable on stable/beta, and also make it easier to enable on android, for
> testing.

[Ami GONE FROM WEBRTC_CHROMIUM, 2013-10-23 16:44:44]

(still a drive-by)
I missed that your comment turned on the possibility of GetChannel
"failing" in a branded build.  I don't understand what that would mean and
I don't see in the code how that can happen.  If the doco for GetChannel()
didn't leave open the possibility of failure, would you revert to PS#1?  If
so, might be worth reaching out to VersionInfo's OWNERS and seeing what's
up.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Henrik Grunell, 2013-10-23 18:36:41]

On 2013/10/23 16:44:44, fischman1 wrote:
> (still a drive-by)
> I missed that your comment turned on the possibility of GetChannel
> "failing" in a branded build.  I don't understand what that would mean and
> I don't see in the code how that can happen.  If the doco for GetChannel()
> didn't leave open the possibility of failure, would you revert to PS#1?  If
> so, might be worth reaching out to VersionInfo's OWNERS and seeing what's
> up.

Right, fail is wrong wording. Header file says: "In unbranded builds, or in
branded builds when the channel cannot be determined, this will be
CHANNEL_UNKNOWN." So I referred to "channel cannot be determined". If that can't
happen given you're in beta or stable, then fine. I'll check the code tomorrow.

Still though, I'm told that there are other distros/brandings of Chromium that
Chrome, where - in a stable release - this would then return CHANNEL_UNKNOWN. I
haven't verified this, you or Joi might know more about this?

[Ami GONE FROM WEBRTC_CHROMIUM, 2013-10-23 18:40:01]

Chromium source in an unbranded build will always return UNKNOWN.  Of
course once you're in an unbranded build all bets are off - distros are as
likely to patch chromium code as they are to build with arbitrary settings.
 I'm not sure it's worth worrying about that here.

-a


On Wed, Oct 23, 2013 at 11:36 AM, <grunell@chromium.org> wrote:

> On 2013/10/23 16:44:44, fischman1 wrote:
>
>> (still a drive-by)
>> I missed that your comment turned on the possibility of GetChannel
>> "failing" in a branded build.  I don't understand what that would mean and
>> I don't see in the code how that can happen.  If the doco for GetChannel()
>> didn't leave open the possibility of failure, would you revert to PS#1?
>>  If
>> so, might be worth reaching out to VersionInfo's OWNERS and seeing what's
>> up.
>>
>
> Right, fail is wrong wording. Header file says: "In unbranded builds, or in
> branded builds when the channel cannot be determined, this will be
> CHANNEL_UNKNOWN." So I referred to "channel cannot be determined". If that
> can't
> happen given you're in beta or stable, then fine. I'll check the code
> tomorrow.
>
> Still though, I'm told that there are other distros/brandings of Chromium
> that
> Chrome, where - in a stable release - this would then return
> CHANNEL_UNKNOWN. I
> haven't verified this, you or Joi might know more about this?
>
>
https://chromiumcodereview.**appspot.com/37253002/<https://chromiumcodereview...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Jói, 2013-10-23 21:57:17]

LGTM for my bits. Looked at the whole change and tend to agree with Ami that you
could just disable copying of the flag when the channel is beta or stable, and
allow it in all other cases. After all, it's not that easy to set a flag
accidentally, and the warning infobar is also being shown for this one.

Cheers,
Jói

https://codereview.chromium.org/37253002/diff/30001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/37253002/diff/30001/content/public/common/con...
content/public/common/content_switches.cc:926: // Disables encryption of RTP
Media for WebRTC.
The comments here are usually the canonical description of the flags, so should
mention that the switch is ignored on channels other than dev and canary.
Something like "When Chrome embeds Content, it ignores this switch except on its
dev and canary channels."

[Henrik Grunell, 2013-10-24 09:13:04]

I took a look at the code for posix. VersionInfo::GetChannel() will return
CHANNEL_UNKNOWN if VersionInfo::GetVersionStringModifier() returns something
other than empty string, "beta", "dev" or "canary". The return string is
determined from CHROME_VERSION_EXTRA environment variable. I tested to set that
to "foo" for the Chrome process and version shows as "30.0.1599.114 (Official
Build 229842) unknown". On Windows the registry is read, I suppose the same
thing can be done there. However, it seems very unlikely that would "fail"
knowing it's stable or beta by bad luck or machine issues.

OK, I'm buying your and Joi's arguments about distros.

I'll revert to checking !stable && !beta.

Justin, please confirm you're fine with this.

On 2013/10/23 18:40:01, fischman1 wrote:
> Chromium source in an unbranded build will always return UNKNOWN.  Of
> course once you're in an unbranded build all bets are off - distros are as
> likely to patch chromium code as they are to build with arbitrary settings.
>  I'm not sure it's worth worrying about that here.
> 
> -a
> 
> 
> On Wed, Oct 23, 2013 at 11:36 AM, <mailto:grunell@chromium.org> wrote:
> 
> > On 2013/10/23 16:44:44, fischman1 wrote:
> >
> >> (still a drive-by)
> >> I missed that your comment turned on the possibility of GetChannel
> >> "failing" in a branded build.  I don't understand what that would mean and
> >> I don't see in the code how that can happen.  If the doco for GetChannel()
> >> didn't leave open the possibility of failure, would you revert to PS#1?
> >>  If
> >> so, might be worth reaching out to VersionInfo's OWNERS and seeing what's
> >> up.
> >>
> >
> > Right, fail is wrong wording. Header file says: "In unbranded builds, or in
> > branded builds when the channel cannot be determined, this will be
> > CHANNEL_UNKNOWN." So I referred to "channel cannot be determined". If that
> > can't
> > happen given you're in beta or stable, then fine. I'll check the code
> > tomorrow.
> >
> > Still though, I'm told that there are other distros/brandings of Chromium
> > that
> > Chrome, where - in a stable release - this would then return
> > CHANNEL_UNKNOWN. I
> > haven't verified this, you or Joi might know more about this?
> >
> >
>
https://chromiumcodereview.**appspot.com/37253002/%3Chttps://chromiumcoderevi...>
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Henrik Grunell, 2013-10-24 10:01:12]

Adding sky@ as owner of chrome/browser/ui, please review that single file.

New patch that allows the flag on "unknown" channel.

https://codereview.chromium.org/37253002/diff/30001/content/public/common/con...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/37253002/diff/30001/content/public/common/con...
content/public/common/content_switches.cc:926: // Disables encryption of RTP
Media for WebRTC.
On 2013/10/23 21:57:18, Jói wrote:
> The comments here are usually the canonical description of the flags, so
should
> mention that the switch is ignored on channels other than dev and canary.
> Something like "When Chrome embeds Content, it ignores this switch except on
its
> dev and canary channels."

Done.

[Henrik Grunell, 2013-10-24 11:38:02]

Patch set 4 is updates based on pending CL
https://codereview.chromium.org/30983002/ (changes how the info is passed to
PeerConnection) which in turn depends on a libjingle roll.

Ami, can you please as owner review content/renderer/media/*.

[juberti, 2013-10-24 15:35:56]

Checking !stable && !beta SGTM.


On Thu, Oct 24, 2013 at 2:13 AM, <grunell@chromium.org> wrote:

> I took a look at the code for posix. VersionInfo::GetChannel() will return
> CHANNEL_UNKNOWN if VersionInfo::**GetVersionStringModifier() returns
> something
> other than empty string, "beta", "dev" or "canary". The return string is
> determined from CHROME_VERSION_EXTRA environment variable. I tested to set
> that
> to "foo" for the Chrome process and version shows as "30.0.1599.114
> (Official
> Build 229842) unknown". On Windows the registry is read, I suppose the same
> thing can be done there. However, it seems very unlikely that would "fail"
> knowing it's stable or beta by bad luck or machine issues.
>
> OK, I'm buying your and Joi's arguments about distros.
>
> I'll revert to checking !stable && !beta.
>
> Justin, please confirm you're fine with this.
>
>
> On 2013/10/23 18:40:01, fischman1 wrote:
>
>> Chromium source in an unbranded build will always return UNKNOWN.  Of
>> course once you're in an unbranded build all bets are off - distros are as
>> likely to patch chromium code as they are to build with arbitrary
>> settings.
>>   I'm not sure it's worth worrying about that here.
>>
>
>  -a
>>
>
>
>  On Wed, Oct 23, 2013 at 11:36 AM, <mailto:grunell@chromium.org> wrote:
>>
>
>  > On 2013/10/23 16:44:44, fischman1 wrote:
>> >
>> >> (still a drive-by)
>> >> I missed that your comment turned on the possibility of GetChannel
>> >> "failing" in a branded build.  I don't understand what that would mean
>> and
>> >> I don't see in the code how that can happen.  If the doco for
>> GetChannel()
>> >> didn't leave open the possibility of failure, would you revert to PS#1?
>> >>  If
>> >> so, might be worth reaching out to VersionInfo's OWNERS and seeing
>> what's
>> >> up.
>> >>
>> >
>> > Right, fail is wrong wording. Header file says: "In unbranded builds,
>> or in
>> > branded builds when the channel cannot be determined, this will be
>> > CHANNEL_UNKNOWN." So I referred to "channel cannot be determined". If
>> that
>> > can't
>> > happen given you're in beta or stable, then fine. I'll check the code
>> > tomorrow.
>> >
>> > Still though, I'm told that there are other distros/brandings of
>> Chromium
>> > that
>> > Chrome, where - in a stable release - this would then return
>> > CHANNEL_UNKNOWN. I
>> > haven't verified this, you or Joi might know more about this?
>> >
>> >
>>
>
> https://chromiumcodereview.**a**ppspot.com/37253002/%3Chttps:/**
>
/chromiumcodereview.appspot.**com/37253002/<http://appspot.com/37253002/%3Chttps://chromiumcodereview.appspot.com/37253002/>
> >
>
>> >
>>
>
>  To unsubscribe from this group and stop receiving emails from it, send an
>>
> email
>
>> to
mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org>
>> .
>>
>
>
>
https://codereview.chromium.**org/37253002/<https://codereview.chromium.org/3...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ami GONE FROM CHROMIUM, 2013-10-24 16:37:44]

LGTM

https://codereview.chromium.org/37253002/diff/180001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/37253002/diff/180001/content/public/common/co...
content/public/common/content_switches.cc:923: const char
kDisableWebRtcEncryption[]      = "disable-webrtc-encryption";
I wonder if the flag should be more explicit about the channels it applies to,
but I can't think of an obviously better name.

[sky, 2013-10-28 14:10:21]

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1437:
command_line->CopySwitchesFrom(browser_command_line,
Can you simplify to AppendSwitch?

[Henrik Grunell, 2013-10-28 15:06:57]

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1437:
command_line->CopySwitchesFrom(browser_command_line,
On 2013/10/28 14:10:22, sky wrote:
> Can you simplify to AppendSwitch?

Yes after checking with browser_command_line->HasSwitch(). However that won't
transfer a possible value and although the code checking for this flag currently
doesn't bother about the value I think I would prefer being fully transparent.
Wdyt?

https://codereview.chromium.org/37253002/diff/180001/content/public/common/co...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/37253002/diff/180001/content/public/common/co...
content/public/common/content_switches.cc:923: const char
kDisableWebRtcEncryption[]      = "disable-webrtc-encryption";
On 2013/10/24 16:37:44, Ami Fischman wrote:
> I wonder if the flag should be more explicit about the channels it applies to,
> but I can't think of an obviously better name.

Yes, that would be nice, but the name's going to be long to be clear enough.
Keeping this unless someone objects.

[sky, 2013-10-28 19:49:14]

LGTM

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/37253002/diff/180001/chrome/browser/chrome_co...
chrome/browser/chrome_content_browser_client.cc:1437:
command_line->CopySwitchesFrom(browser_command_line,
On 2013/10/28 15:06:57, Henrik Grunell wrote:
> On 2013/10/28 14:10:22, sky wrote:
> > Can you simplify to AppendSwitch?
> 
> Yes after checking with browser_command_line->HasSwitch(). However that won't
> transfer a possible value and although the code checking for this flag
currently
> doesn't bother about the value I think I would prefer being fully transparent.
> Wdyt?

You're right, this is fine.

[juberti, 2013-10-28 21:20:10]

lgtm regarding the policy used to enable/disable based on channels.

[commit-bot: I haz the power, 2013-11-04 15:21:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/37253002/180001

[commit-bot: I haz the power, 2013-11-04 17:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/37253002/180001

[commit-bot: I haz the power, 2013-11-05 00:53:25]

Change committed as 232855