Change WebRTC IP handling policy from multiple booleans to an enum.

Change WebRTC IP handling policy from multiple booleans to an enum.

Previously in M42 we have mutliple routes as a boolean. In M47 dev, we added one more called enable_non_proxied_udp. It becomes clear that we're going to add more modes and some of the combination of booleans are not supported.

We're replacing this by an enum (string in preference). To make sure we don't break the existing mutliple_routes options, a check has been put to honor that setting but multiple_routes will become readonly now and will be ignored if the new preference is set.

BUG=542765
Committed: https://crrev.com/c537ba208c24c14ca0259e0492041b3f2e6a6eb9
Cr-Commit-Position: refs/heads/master@{#358402}

[guoweis_left_chromium, 2015-10-20 22:19:24]

guoweis@chromium.org changed reviewers:
+ juberti@chromium.org, pthatcher@chromium.org

[guoweis_left_chromium, 2015-10-20 23:03:34]

Patchset #2 (id:20001) has been deleted

[pthatcher1, 2015-10-21 00:06:52]

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

[pthatcher1, 2015-10-21 00:06:53]

https://codereview.chromium.org/1413393003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_api.cc (left):

https://codereview.chromium.org/1413393003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_api.cc:124:
{"webRTCNonProxiedUdpEnabled", prefs::kWebRTCNonProxiedUdpEnabled,
Is this going to break the existing Network Limiter extension?  How are we going
to transition the extension to the new API?  Can the extension do version
checks?

https://codereview.chromium.org/1413393003/diff/40001/chrome/common/pref_name...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/1413393003/diff/40001/chrome/common/pref_name...
chrome/common/pref_names.cc:1201: // set, it defaults to "default".
We should probably define what "default" means.  I assume it means something
like "do what Chrome does with no preference set".

https://codereview.chromium.org/1413393003/diff/40001/content/public/common/r...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/1413393003/diff/40001/content/public/common/r...
content/public/common/renderer_preferences.cc:34:
report_frame_name_changes(false),
Do we not need to set the value to "default"?

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

https://codereview.chromium.org/1413393003/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:68: enum
WebRTCIPPermissionMode {
It was called IPHandling in one place and IPPermission in another.  And policy
in one place and mode in the other.  I prefer "permission" and "policy" for a
combination of IPPermissionPolicy

https://codereview.chromium.org/1413393003/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:483:
port_config.enable_nonproxied_udp = (mode != DISABLE_NON_PROXIED_UDP);
I think this might be more clear as:

switch(mode) {
  case DEFAULT_PUBLIC_INTERFACE_ONLY:
    port_config.enable_multiple_routes = false;
    port_config.enable_nonproxied_udp = true;
    break;
  case DISABLE_NON_PROXIED_UDP:
    port_config.enable_multiple_routes = false;
    port_config.enable_nonproxied_udp = false;
  default:
    port_config.enable_multiple_routes = true;
    port_config.enable_nonproxied_udp = true;
    break;
}

[juberti2, 2015-10-21 00:58:40]

https://codereview.chromium.org/1413393003/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/preference/preference_api.cc (left):

https://codereview.chromium.org/1413393003/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/preference/preference_api.cc:124:
{"webRTCNonProxiedUdpEnabled", prefs::kWebRTCNonProxiedUdpEnabled,
On 2015/10/21 00:06:53, pthatcher1 wrote:
> Is this going to break the existing Network Limiter extension?  How are we
going
> to transition the extension to the new API?  Can the extension do version
> checks?

Agree with this concern. I think we need to handle the case where the IP
handling policy is set, or, if not set, check for the old multiple routes
preference. Otherwise, users who set the pref manually and old extension users
will regress when the new version of Chrome rolls out.

[guoweis_left_chromium, 2015-10-23 16:39:14]

Patchset #3 (id:60001) has been deleted

[guoweis_left_chromium, 2015-10-23 16:39:25]

Patchset #3 (id:80001) has been deleted

[guoweis_left_chromium, 2015-10-23 16:40:49]

PTAL.

https://codereview.chromium.org/1413393003/diff/40001/content/public/common/r...
File content/public/common/renderer_preferences.cc (right):

https://codereview.chromium.org/1413393003/diff/40001/content/public/common/r...
content/public/common/renderer_preferences.cc:34:
report_frame_name_changes(false),
On 2015/10/21 00:06:53, pthatcher1 wrote:
> Do we not need to set the value to "default"?

No. browser_ui_prefs.cc has the default already.

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

https://codereview.chromium.org/1413393003/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:68: enum
WebRTCIPPermissionMode {
On 2015/10/21 00:06:53, pthatcher1 wrote:
> It was called IPHandling in one place and IPPermission in another.  And policy
> in one place and mode in the other.  I prefer "permission" and "policy" for a
> combination of IPPermissionPolicy

Renamed it to be WebRTCIPHandlingPolicy to be consistent.

https://codereview.chromium.org/1413393003/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:483:
port_config.enable_nonproxied_udp = (mode != DISABLE_NON_PROXIED_UDP);
On 2015/10/21 00:06:53, pthatcher1 wrote:
> I think this might be more clear as:
> 
> switch(mode) {
>   case DEFAULT_PUBLIC_INTERFACE_ONLY:
>     port_config.enable_multiple_routes = false;
>     port_config.enable_nonproxied_udp = true;
>     break;
>   case DISABLE_NON_PROXIED_UDP:
>     port_config.enable_multiple_routes = false;
>     port_config.enable_nonproxied_udp = false;
>   default:
>     port_config.enable_multiple_routes = true;
>     port_config.enable_nonproxied_udp = true;
>     break;
> }

Done.

[pthatcher1, 2015-10-23 17:27:29]

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:51:
pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled);
What if someone had webRTCNonProxiedUdpEnabled set before?  Do we just throw
that knowledge away?

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:51:
pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled);
This might be more clear as:

bool enable_multiple_routes = 
  !pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy) &&
  (!pref_service->HasPrefPath(prefs::kWebRTCMultipleRoutesEnabled) ||
   pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled));

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
File content/public/renderer/webrtc_ip_handling_policy.h (right):

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
content/public/renderer/webrtc_ip_handling_policy.h:17: // address discovered.
There will be no host address discovered => This doesn't expose any local
addresses.

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
content/public/renderer/webrtc_ip_handling_policy.h:21: // proxy server supports
UDP. This doesn't expose any local address either.
TCP protocol => TCP

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
content/public/renderer/webrtc_ip_handling_policy.h:21: // proxy server supports
UDP. This doesn't expose any local address either.
local address => local addresses

[guoweis_left_chromium, 2015-10-23 18:24:58]

guoweis@chromium.org changed reviewers:
+ wfh@chromium.org

[guoweis_left_chromium, 2015-10-23 18:24:59]

wfh@chromium.org: Please review changes in view_message.h

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:51:
pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled);
On 2015/10/23 17:27:29, pthatcher1 wrote:
> What if someone had webRTCNonProxiedUdpEnabled set before?  Do we just throw
> that knowledge away?

webRTCNonProxiedUdpEnabled is still in beta. I'm trying to get this in M47 to
prevent people from using that.

https://codereview.chromium.org/1413393003/diff/100001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:51:
pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled);
On 2015/10/23 17:27:29, pthatcher1 wrote:
> This might be more clear as:
> 
> bool enable_multiple_routes = 
>   !pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy) &&
>   (!pref_service->HasPrefPath(prefs::kWebRTCMultipleRoutesEnabled) ||
>    pref_service->GetBoolean(prefs::kWebRTCMultipleRoutesEnabled));

I don't feel this is clear. We could chat if you feel strongly.

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
File content/public/renderer/webrtc_ip_handling_policy.h (right):

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
content/public/renderer/webrtc_ip_handling_policy.h:17: // address discovered.
On 2015/10/23 17:27:29, pthatcher1 wrote:
> There will be no host address discovered => This doesn't expose any local
> addresses.

Done.

https://codereview.chromium.org/1413393003/diff/100001/content/public/rendere...
content/public/renderer/webrtc_ip_handling_policy.h:21: // proxy server supports
UDP. This doesn't expose any local address either.
On 2015/10/23 17:27:29, pthatcher1 wrote:
> local address => local addresses

Done.

[guoweis_left_chromium, 2015-10-23 18:25:45]

guoweis@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org

[guoweis_left_chromium, 2015-10-23 18:25:45]

jochen:
  chrome/browser/DEPS
  chrome/browser/extensions/api/DEPS
  chrome/browser/extensions/api/preference/preference_api.cc
  chrome/browser/extensions/api/preference/preference_apitest.cc
  chrome/browser/renderer_preferences_util.cc
  chrome/browser/ui/DEPS
  chrome/browser/ui/browser_ui_prefs.cc
  chrome/browser/ui/prefs/prefs_tab_helper.cc
  chrome/common/extensions/api/privacy.json
  content/public/common/renderer_preferences.cc
  content/public/common/renderer_preferences.h
  content/public/renderer/webrtc_ip_handling_policy.cc
  content/public/renderer/webrtc_ip_handling_policy.h
  content/renderer/media/webrtc/peer_connection_dependency_factory.cc
  content/shell/browser/shell.cc
nasko:
  content/common/view_messages.h
  content/public/common/renderer_preferences.cc
  content/public/common/renderer_preferences.h
  content/public/renderer/webrtc_ip_handling_policy.cc
  content/public/renderer/webrtc_ip_handling_policy.h
  content/renderer/media/webrtc/peer_connection_dependency_factory.cc
  content/shell/browser/shell.cc

[guoweis_left_chromium, 2015-10-23 18:42:54]

Patchset #5 (id:140001) has been deleted

[guoweis_left_chromium, 2015-10-23 20:16:49]

Description was changed from

==========
Change WebRTC IP handling policy from multiple booleans to an enum.

BUG=542765
==========

to

==========
Change WebRTC IP handling policy from multiple booleans to an enum.

Previously in M42 we have mutliple routes as a boolean. In M47 dev, we added one
more called enable_non_proxied_udp. It becomes clear that we're going to add
more modes and some of the combination of booleans are not supported.

We're replacing this by an enum (string in preference). To make sure we don't
break the existing mutliple_routes options, a check has been put to honor that
setting but multiple_routes will become readonly now and will be ignored if the
new preference is set.

BUG=542765
==========

[pthatcher2, 2015-10-23 21:07:30]

lgtm

[juberti2, 2015-10-24 00:45:13]

lgtm

[jochen (gone - plz use gerrit), 2015-10-26 15:43:56]

https://codereview.chromium.org/1413393003/diff/120001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1413393003/diff/120001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:122: "+content/public/renderer",
browser can't depend on renderer. i'd propose to move your header to
public/common

[nasko, 2015-10-26 15:49:49]

Few nits.

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

https://codereview.chromium.org/1413393003/diff/120001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:76: if
(preference == content::kWebRTCIPHandlingDefaultPublicInterfaceOnly)
This anonymous namespace should go inside content. Then the content:: prefix can
be dropped, as this code does reside in the content/ module.

https://codereview.chromium.org/1413393003/diff/120001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:488:
default:
Why not have a case for DEFAULT return value and have the default: handler be
NOTREACHED()? It will make sure all possible values of the enum are properly
handled.

https://codereview.chromium.org/1413393003/diff/120001/content/shell/browser/...
File content/shell/browser/shell.cc (right):

https://codereview.chromium.org/1413393003/diff/120001/content/shell/browser/...
content/shell/browser/shell.cc:125:
content::kWebRTCIPHandlingDefaultPublicInterfaceOnly;
This code is already in content/. No need for content:: prefix.

[Will Harris, 2015-10-26 16:30:40]

wfh@chromium.org changed reviewers:
- wfh@chromium.org

[Will Harris, 2015-10-26 16:31:20]

nasko can you security review view_messages.h when you do content/ review?
Removing myself.

[guoweis_left_chromium, 2015-10-26 18:13:17]

On 2015/10/26 16:31:20, Will Harris wrote:
> nasko can you security review view_messages.h when you do content/ review?
> Removing myself.

PTAL.

However, my change in chrome/common/extensions/api/privacy.json still causes
warning.
WARNING:root:Could not resolve $ref privacy.webRTCIPHandlingPolicy in namespace
privacy on extensions/privacy.


Does anyone know how to fix that?

[jochen (gone - plz use gerrit), 2015-10-27 12:19:49]

jochen@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[jochen (gone - plz use gerrit), 2015-10-27 12:19:49]

+devlin for extensions

[Devlin, 2015-10-27 16:13:32]

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_api.cc (left):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:124:
{"webRTCNonProxiedUdpEnabled", prefs::kWebRTCNonProxiedUdpEnabled,
Does this one not need to be left for now?

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:123: //
webRTCIPHandlingPolicy. Leaving it to allow reading from it.
Add a TODO to clean this up after a couple milestones (maybe M50?).

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:125:
APIPermission::kPrivacy, APIPermission::kInvalid},
We should make this writable, still.  Otherwise extensions that used to work
won't the second this is released.  We should give extensions a few milestones
to update.

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_apitest.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_apitest.cc:129:
prefs->SetString(prefs::kWebRTCIPHandlingPolicy,
I'm a little worried that it looks like all tests for the old prefs are removed,
but we'll still need the functionality there... Why not leave the old tests in,
and add new ones?

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:48: bool ignore_multiroutes_option =
This is just my ignorance, but why do we ignore the option if it's set to true?

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:94: expect({
'value':
nit: no need for quotes around keys.

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:107:
chrome.test.callbackPass());
What does this actually test?  It's not verifying the result...

[guoweis_left_chromium, 2015-10-27 20:18:19]

Patchset #6 (id:180001) has been deleted

[guoweis_left_chromium, 2015-10-27 20:32:00]

Devlin@, PTAL.

Could you also help me with the warning I mentioned?
WARNING:root:Could not resolve $ref privacy.webRTCIPHandlingPolicy in namespace
privacy on extensions/privacy.

https://codereview.chromium.org/1413393003/diff/120001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1413393003/diff/120001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:122: "+content/public/renderer",
On 2015/10/26 15:43:56, jochen wrote:
> browser can't depend on renderer. i'd propose to move your header to
> public/common

Done.

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

https://codereview.chromium.org/1413393003/diff/120001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:76: if
(preference == content::kWebRTCIPHandlingDefaultPublicInterfaceOnly)
On 2015/10/26 15:49:49, nasko (slow to review) wrote:
> This anonymous namespace should go inside content. Then the content:: prefix
can
> be dropped, as this code does reside in the content/ module.

Done.

https://codereview.chromium.org/1413393003/diff/120001/content/renderer/media...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:488:
default:
On 2015/10/26 15:49:49, nasko (slow to review) wrote:
> Why not have a case for DEFAULT return value and have the default: handler be
> NOTREACHED()? It will make sure all possible values of the enum are properly
> handled.

Done.

https://codereview.chromium.org/1413393003/diff/120001/content/shell/browser/...
File content/shell/browser/shell.cc (right):

https://codereview.chromium.org/1413393003/diff/120001/content/shell/browser/...
content/shell/browser/shell.cc:125:
content::kWebRTCIPHandlingDefaultPublicInterfaceOnly;
On 2015/10/26 15:49:49, nasko (slow to review) wrote:
> This code is already in content/. No need for content:: prefix.

Done.

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:123: //
webRTCIPHandlingPolicy. Leaving it to allow reading from it.
On 2015/10/27 16:13:31, Devlin wrote:
> Add a TODO to clean this up after a couple milestones (maybe M50?).

Done.

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:125:
APIPermission::kPrivacy, APIPermission::kInvalid},
On 2015/10/27 16:13:31, Devlin wrote:
> We should make this writable, still.  Otherwise extensions that used to work
> won't the second this is released.  We should give extensions a few milestones
> to update.

Done.

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_apitest.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_apitest.cc:129:
prefs->SetString(prefs::kWebRTCIPHandlingPolicy,
On 2015/10/27 16:13:31, Devlin wrote:
> I'm a little worried that it looks like all tests for the old prefs are
removed,
> but we'll still need the functionality there... Why not leave the old tests
in,
> and add new ones?

Done.

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:48: bool ignore_multiroutes_option =
On 2015/10/27 16:13:31, Devlin wrote:
> This is just my ignorance, but why do we ignore the option if it's set to
true?

If it's true, it's the default, hence we could use the default of the new policy
as well since it's not changed.

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:94: expect({
'value':
On 2015/10/27 16:13:31, Devlin wrote:
> nit: no need for quotes around keys.

Done.

https://codereview.chromium.org/1413393003/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:107:
chrome.test.callbackPass());
On 2015/10/27 16:13:31, Devlin wrote:
> What does this actually test?  It's not verifying the result...

Done.

[Devlin, 2015-10-27 20:45:44]

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/DEPS (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/DEPS:6: "+content/public/common",
This is already included in chrome/DEPS - do we really need it here?  (Plenty of
extension apis already depend on content/public/common, and we haven't needed
this before - what changed?)

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:123: // obsoleted and
replaced by webRTCIPHandlingPolicy. Leaving it to allow
s/become obsoleted and/been/

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:124: // reading from
it. TODO(guoweis): Remove this in M50.
But not just reading now, right?  So this should probably just be "Leaving it
for backwards compatibility."

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:49: if
(!pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy)) {
I think this is clearer when using positive if-clauses, e.g.,
if (pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy)) {
  prefs->webrtc_ip_handling_policy =
      pref_service->GetString(prefs::kWebRTCIPHandlingPolicy);
}

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/privacy.json (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:31: "deprecated": "Please use
$(ref:privacy.webRTCIPHandlingPolicy). This remains read-only for backward
compatibility in this release and will be removed in the future.",
webRTCIPHandlingPolicy isn't defined on privacy, it's defined on
privacy.network.  You could try $(ref:privacy.network.webRTCIPHandlingPolicy),
which would hopefully work.

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:37: "deprecated": "Please use
$(ref:privacy.webRTCIPHandlingPolicy). This remains read-only for backward
compatibility in this release and will be removed in the future.",
It's not read-only, right?

[guoweis_left_chromium, 2015-10-28 16:39:51]

also added one more mode 

DEFAULT_PUBLIC_AND_PRIVATE_INTERFACES

This will expose one public and private default address.

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/DEPS (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/DEPS:6: "+content/public/common",
On 2015/10/27 20:45:44, Devlin wrote:
> This is already included in chrome/DEPS - do we really need it here?  (Plenty
of
> extension apis already depend on content/public/common, and we haven't needed
> this before - what changed?)

Done.

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
File chrome/browser/extensions/api/preference/preference_api.cc (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:123: // obsoleted and
replaced by webRTCIPHandlingPolicy. Leaving it to allow
On 2015/10/27 20:45:44, Devlin wrote:
> s/become obsoleted and/been/

Done.

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/extensi...
chrome/browser/extensions/api/preference/preference_api.cc:124: // reading from
it. TODO(guoweis): Remove this in M50.
On 2015/10/27 20:45:44, Devlin wrote:
> But not just reading now, right?  So this should probably just be "Leaving it
> for backwards compatibility."

Done.

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/rendere...
File chrome/browser/renderer_preferences_util.cc (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/browser/rendere...
chrome/browser/renderer_preferences_util.cc:49: if
(!pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy)) {
On 2015/10/27 20:45:44, Devlin wrote:
> I think this is clearer when using positive if-clauses, e.g.,
> if (pref_service->HasPrefPath(prefs::kWebRTCIPHandlingPolicy)) {
>   prefs->webrtc_ip_handling_policy =
>       pref_service->GetString(prefs::kWebRTCIPHandlingPolicy);
> }

agree. But when both webRTCNonProxiedUdpEnabled and webrtcMultipleRoutesEnabled
are true and there is NO webrtcIPHandlingPolicy , I still want to fall back to
the default of webrtcIPHandlingPolicy. I can hard code it here but since I have
specified in the other place, I don't want to repeat here.

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/privacy.json (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:31: "deprecated": "Please use
$(ref:privacy.webRTCIPHandlingPolicy). This remains read-only for backward
compatibility in this release and will be removed in the future.",
On 2015/10/27 20:45:44, Devlin wrote:
> webRTCIPHandlingPolicy isn't defined on privacy, it's defined on
> privacy.network.  You could try $(ref:privacy.network.webRTCIPHandlingPolicy),
> which would hopefully work.

That was the first thing I ever tried. No, it didn't do the trick... Still
warning.

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:37: "deprecated": "Please use
$(ref:privacy.webRTCIPHandlingPolicy). This remains read-only for backward
compatibility in this release and will be removed in the future.",
On 2015/10/27 20:45:44, Devlin wrote:
> It's not read-only, right?

Done.

[Devlin, 2015-10-28 22:39:37]

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
File chrome/common/extensions/api/privacy.json (right):

https://codereview.chromium.org/1413393003/diff/200001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:31: "deprecated": "Please use
$(ref:privacy.webRTCIPHandlingPolicy). This remains read-only for backward
compatibility in this release and will be removed in the future.",
On 2015/10/28 16:39:51, guoweis_chromium wrote:
> On 2015/10/27 20:45:44, Devlin wrote:
> > webRTCIPHandlingPolicy isn't defined on privacy, it's defined on
> > privacy.network.  You could try
$(ref:privacy.network.webRTCIPHandlingPolicy),
> > which would hopefully work.
> 
> That was the first thing I ever tried. No, it didn't do the trick... Still
> warning.

Dang.  I guess we don't parse properties for cross-references.  In lieu of that,
pick one between just typing it out or linking to the type (IPHandlingPolicy).

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:98: expect({
value:
no spaces between brackets and keys/values.  Also, please re-wrap this so it
makes sense.

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:100:
'levelOfControl': "controllable_by_this_extension" },
no quotes around keys.

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:100:
'levelOfControl': "controllable_by_this_extension" },
prefer single quotes in JS

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:111: expect({
value:
Umm... this shouldn't pass.  The callback doesn't return any value.  If it
*does* pass, we need to fix the rest of these tests (but I'm hoping it doesn't).

[guoweis_webrtc, 2015-10-29 21:34:53]

guoweis@webrtc.org changed reviewers:
+ guoweis@webrtc.org

[guoweis_webrtc, 2015-10-29 21:34:54]

PTAL.

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:98: expect({
value:
On 2015/10/28 22:39:37, Devlin wrote:
> no spaces between brackets and keys/values.  Also, please re-wrap this so it
> makes sense.

Done.

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:100:
'levelOfControl': "controllable_by_this_extension" },
On 2015/10/28 22:39:37, Devlin wrote:
> no quotes around keys.

Done.

https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:111: expect({
value:
On 2015/10/28 22:39:37, Devlin wrote:
> Umm... this shouldn't pass.  The callback doesn't return any value.  If it
> *does* pass, we need to fix the rest of these tests (but I'm hoping it
doesn't).

My bad. This test doesn't pass. I switched the order of set and get so we set a
value and using get to verity that it succeeded.

[guoweis_webrtc, 2015-10-30 16:37:29]

On 2015/10/29 21:34:54, guoweis wrote:
> PTAL.
> 
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> File chrome/test/data/extensions/api_test/preference/standard/test.js (right):
> 
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/preference/standard/test.js:98: expect({
> value:
> On 2015/10/28 22:39:37, Devlin wrote:
> > no spaces between brackets and keys/values.  Also, please re-wrap this so it
> > makes sense.
> 
> Done.
> 
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/preference/standard/test.js:100:
> 'levelOfControl': "controllable_by_this_extension" },
> On 2015/10/28 22:39:37, Devlin wrote:
> > no quotes around keys.
> 
> Done.
> 
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/preference/standard/test.js:111: expect({
> value:
> On 2015/10/28 22:39:37, Devlin wrote:
> > Umm... this shouldn't pass.  The callback doesn't return any value.  If it
> > *does* pass, we need to fix the rest of these tests (but I'm hoping it
> doesn't).
> 
> My bad. This test doesn't pass. I switched the order of set and get so we set
a
> value and using get to verity that it succeeded.

Ping, jochen@, nasko@, devlin@, PTAL.

[guoweis_webrtc, 2015-11-02 17:30:31]

On 2015/10/30 16:37:29, guoweis wrote:
> On 2015/10/29 21:34:54, guoweis wrote:
> > PTAL.
> > 
> >
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> > File chrome/test/data/extensions/api_test/preference/standard/test.js
(right):
> > 
> >
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> > chrome/test/data/extensions/api_test/preference/standard/test.js:98:
expect({
> > value:
> > On 2015/10/28 22:39:37, Devlin wrote:
> > > no spaces between brackets and keys/values.  Also, please re-wrap this so
it
> > > makes sense.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> > chrome/test/data/extensions/api_test/preference/standard/test.js:100:
> > 'levelOfControl': "controllable_by_this_extension" },
> > On 2015/10/28 22:39:37, Devlin wrote:
> > > no quotes around keys.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1413393003/diff/220001/chrome/test/data/exten...
> > chrome/test/data/extensions/api_test/preference/standard/test.js:111:
expect({
> > value:
> > On 2015/10/28 22:39:37, Devlin wrote:
> > > Umm... this shouldn't pass.  The callback doesn't return any value.  If it
> > > *does* pass, we need to fix the rest of these tests (but I'm hoping it
> > doesn't).
> > 
> > My bad. This test doesn't pass. I switched the order of set and get so we
set
> a
> > value and using get to verity that it succeeded.
> 
> Ping, jochen@, nasko@, devlin@, PTAL.

Ping, jochen@, nasko@, devlin@, PTAL.

[Devlin, 2015-11-02 18:00:17]

https://codereview.chromium.org/1413393003/diff/240001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1413393003/diff/240001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:122: "+content/public/common",
I'm still confused on why you need this when it's specified in chrome/DEPS.

https://codereview.chromium.org/1413393003/diff/240001/chrome/common/extensio...
File chrome/common/extensions/api/privacy.json (right):

https://codereview.chromium.org/1413393003/diff/240001/chrome/common/extensio...
chrome/common/extensions/api/privacy.json:43: "description": "Allow users to
specify the media performance/privacy tradeoffs which impacts how WebRTC traffic
will be routed and how much local address information is exposed. This
preference's value is of type IPHandlingPolicy, defaulting to
<code>default</code>."
indentation off.

https://codereview.chromium.org/1413393003/diff/240001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:101: function
getWebRTCIPHandlingPolicy() {
Tests shouldn't be inter-dependent.

[guoweis_webrtc, 2015-11-02 18:27:48]

Patchset #9 (id:260001) has been deleted

[guoweis_webrtc, 2015-11-02 18:28:48]

On 2015/11/02 18:00:17, Devlin wrote:
> https://codereview.chromium.org/1413393003/diff/240001/chrome/browser/DEPS
> File chrome/browser/DEPS (right):
> 
>
https://codereview.chromium.org/1413393003/diff/240001/chrome/browser/DEPS#ne...
> chrome/browser/DEPS:122: "+content/public/common",
> I'm still confused on why you need this when it's specified in chrome/DEPS.
> 
>
https://codereview.chromium.org/1413393003/diff/240001/chrome/common/extensio...
> File chrome/common/extensions/api/privacy.json (right):
> 
>
https://codereview.chromium.org/1413393003/diff/240001/chrome/common/extensio...
> chrome/common/extensions/api/privacy.json:43: "description": "Allow users to
> specify the media performance/privacy tradeoffs which impacts how WebRTC
traffic
> will be routed and how much local address information is exposed. This
> preference's value is of type IPHandlingPolicy, defaulting to
> <code>default</code>."
> indentation off.
> 
>
https://codereview.chromium.org/1413393003/diff/240001/chrome/test/data/exten...
> File chrome/test/data/extensions/api_test/preference/standard/test.js (right):
> 
>
https://codereview.chromium.org/1413393003/diff/240001/chrome/test/data/exten...
> chrome/test/data/extensions/api_test/preference/standard/test.js:101: function
> getWebRTCIPHandlingPolicy() {
> Tests shouldn't be inter-dependent.

PTAL.

[Devlin, 2015-11-02 19:25:46]

extensions lgtm if the bots are happy.

https://codereview.chromium.org/1413393003/diff/280001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/preference/standard/test.js (right):

https://codereview.chromium.org/1413393003/diff/280001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/preference/standard/test.js:102: 'should
receive default_public_interface_only.'));
nit: indentation off.

[nasko, 2015-11-03 18:54:01]

Few comments.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/renderer_preferences.h:97: // of the strings defined in
privacy.json. Default is in brower_ui_prefs.cc.
nit: The comment should probably list the values and not refer to files outside
of content/. It is not allowed for content/ to depend on chrome/, so we should
strive for this to be the case even with comments.

The valid flags seem to be described in webrtc_ip_handling_policy.h, so you
could reference that.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/webrtc_ip_handling_policy.cc (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
No (c).

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/webrtc_ip_handling_policy.h (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
No (c)

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.h:19:
kWebRTCIPHandlingDefaultPublicAndPrivateInterfaces[];
Does the "Default" part add meaning to the flag? Is
kWebRTCIPHandlingPublicAndPrivateInterfaces not sufficient enough? The same
question about the flag below.

[guoweis_webrtc, 2015-11-03 23:19:22]

PTAL.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/renderer_preferences.h (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/renderer_preferences.h:97: // of the strings defined in
privacy.json. Default is in brower_ui_prefs.cc.
On 2015/11/03 18:54:01, nasko (slow to review) wrote:
> nit: The comment should probably list the values and not refer to files
outside
> of content/. It is not allowed for content/ to depend on chrome/, so we should
> strive for this to be the case even with comments.
> 
> The valid flags seem to be described in webrtc_ip_handling_policy.h, so you
> could reference that.

Done.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/webrtc_ip_handling_policy.cc (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.cc:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/03 18:54:01, nasko (slow to review) wrote:
> No (c).

Done.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
File content/public/common/webrtc_ip_handling_policy.h (right):

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.h:1: // Copyright (c) 2015 The
Chromium Authors. All rights reserved.
On 2015/11/03 18:54:01, nasko (slow to review) wrote:
> No (c)

Done.

https://codereview.chromium.org/1413393003/diff/340001/content/public/common/...
content/public/common/webrtc_ip_handling_policy.h:19:
kWebRTCIPHandlingDefaultPublicAndPrivateInterfaces[];
On 2015/11/03 18:54:01, nasko (slow to review) wrote:
> Does the "Default" part add meaning to the flag? Is
> kWebRTCIPHandlingPublicAndPrivateInterfaces not sufficient enough? The same
> question about the flag below.

Yes, default here is the one interface chosen by OS for http traffic. There
could be multiple interfaces all with internet access on a multi-homed machine.

[nasko, 2015-11-04 17:34:12]

Looks good, but I'm not sure I understand the gypi change. Can you explain why
it is needed?

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
content/content_common.gypi:827: 'sources!': [
What is the goal of this change?

[guoweis_webrtc, 2015-11-04 17:38:07]

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
content/content_common.gypi:827: 'sources!': [
On 2015/11/04 17:34:12, nasko (slow to review) wrote:
> What is the goal of this change?

If there is no webrtc, there is no need to include these 2 files. Probably don't
do much but just for cleanness.

I tried to do public_common_sources! but it doesn't seem to work.

[nasko, 2015-11-04 17:50:58]

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1413393003/diff/360001/content/content_common...
content/content_common.gypi:827: 'sources!': [
On 2015/11/04 17:38:07, guoweis wrote:
> On 2015/11/04 17:34:12, nasko (slow to review) wrote:
> > What is the goal of this change?
> 
> If there is no webrtc, there is no need to include these 2 files. Probably
don't
> do much but just for cleanness.
> 
> I tried to do public_common_sources! but it doesn't seem to work.

But you have already included it up in the public_common_sources and here. There
is no need for both of these to exist.

Shouldn't your sources string here drop the "!"?

[nasko, 2015-11-04 18:33:33]

The GN change seems fine, but I'm not certain. I'd defer to someone with better
knowledge of GN.

https://codereview.chromium.org/1413393003/diff/380001/content/content_common...
File content/content_common.gypi (right):

https://codereview.chromium.org/1413393003/diff/380001/content/content_common...
content/content_common.gypi:821: 'sources': [
Sources should go after dependencies.

[guoweis_webrtc, 2015-11-04 18:34:52]

guoweis@webrtc.org changed reviewers:
+ brettw@chromium.org

[guoweis_webrtc, 2015-11-04 18:34:53]

brettw@, can you review the content/public/common/BUILD.gn?

[jochen (gone - plz use gerrit), 2015-11-05 01:30:36]

lgtm

[Will Harris, 2015-11-06 18:35:04]

content/common/view_messages.h lgtm

[guoweis_webrtc, 2015-11-06 18:35:42]

The CQ bit was checked by guoweis@webrtc.org

[guoweis_webrtc, 2015-11-06 18:35:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@chromium.org, juberti@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1413393003/#ps400001
(title: "update GYP file with right order.")

[commit-bot: I haz the power, 2015-11-06 18:36:26]

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

[commit-bot: I haz the power, 2015-11-06 21:20:38]

Committed patchset #15 (id:400001)

[commit-bot: I haz the power, 2015-11-06 21:21:22]

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