Framework to allow Chromoting host to respect NAT traversal policy in linux.

Framework to allow Chromoting host to respect NAT traversal policy in linux.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96859

[awong, 2011-08-10 01:10:40]

Dave: please look at the policy_hack stuff
wez: please look at the scriptable object stuff.

[dmac, 2011-08-10 21:39:00]

Some basic comments... I can add more once you make decisions around some of the
stuff below.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:476: // TODO(sergeyu): Use firewall
traversal policy settings here.
get rid of the TODO

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/json_value_serializer.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/json_value_serializer.h:5: // Forked from
content/common/json_value_serializer.h
worth just grabbing json_value_serializer directly from content/common? I know
this is "bad" but it's a temporary bad instead of forking.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:15: namespace policy_hack {
is a policy_hack namespace really necessary within remoting?

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:30: static int
kFallbackReloadDelayMinutes = 15;
no need to mark static in the anonymous namespace.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:102: new
FileWatcherDelegate(weak_factory_.GetWeakPtr()))) {
refcounting the filewatcherdelegate may simplify some code here.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.h:4: 
why do we have a header for this as opposed to putting it directly in the .cc
file? That would match the rest of our creation patterns.

Same for the mac and win versions.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.h:34: ~NatPolicyLinux();
I know you get this virtual by default (having inherited from a virtual) but is
it worth marking it virtual here just to make sure other people don't freak?

[awong, 2011-08-11 01:23:28]

okay, ready for another look.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:476: // TODO(sergeyu): Use firewall
traversal policy settings here.
On 2011/08/10 21:39:00, dmac wrote:
> get rid of the TODO

Done.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/json_value_serializer.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/json_value_serializer.h:5: // Forked from
content/common/json_value_serializer.h
On 2011/08/10 21:39:00, dmac wrote:
> worth just grabbing json_value_serializer directly from content/common? I know
> this is "bad" but it's a temporary bad instead of forking.

:-/ That's so ugly...I...fine. did it.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:15: namespace policy_hack {
On 2011/08/10 21:39:00, dmac wrote:
> is a policy_hack namespace really necessary within remoting?

yes.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:30: static int
kFallbackReloadDelayMinutes = 15;
On 2011/08/10 21:39:00, dmac wrote:
> no need to mark static in the anonymous namespace.

consted instead.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:102: new
FileWatcherDelegate(weak_factory_.GetWeakPtr()))) {
On 2011/08/10 21:39:00, dmac wrote:
> refcounting the filewatcherdelegate may simplify some code here.

Went through a few iterations, and decided that infact it does not. Can talk in
person if you want to know why.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.h (right):

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.h:4: 
On 2011/08/10 21:39:00, dmac wrote:
> why do we have a header for this as opposed to putting it directly in the .cc
> file? That would match the rest of our creation patterns.
> 
> Same for the mac and win versions.

hmm...good point. collapsed.

http://codereview.chromium.org/7599017/diff/12001/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.h:34: ~NatPolicyLinux();
On 2011/08/10 21:39:00, dmac wrote:
> I know you get this virtual by default (having inherited from a virtual) but
is
> it worth marking it virtual here just to make sure other people don't freak?

Yes. Good catch. Fixed.

[Sergey Ulanov, 2011-08-11 01:32:49]

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:71: // If there is no
host_resolver_factory_, we're not sandboxes, do we
s/do/so/

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:72: // can let jingle itself do the
DNS resolution.
s/jingle/libjingle/

[awong, 2011-08-11 01:36:45]

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:71: // If there is no
host_resolver_factory_, we're not sandboxes, do we
On 2011/08/11 01:32:49, sergeyu wrote:
> s/do/so/

Done.

http://codereview.chromium.org/7599017/diff/16004/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:72: // can let jingle itself do the
DNS resolution.
On 2011/08/11 01:32:49, sergeyu wrote:
> s/jingle/libjingle/

Done.

[Jamie, 2011-08-11 15:24:31]

http://codereview.chromium.org/7599017/diff/20012/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/20012/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:71: // If there is no
host_resolver_factory_, we're not sandboxes, so we
Nit: s/sandboxes/sandboxed/ :)

Also, isn't it conventional to encode variable names in | characters?

[dmac, 2011-08-11 18:42:54]

Mostly comment nits

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:87: nat_traversal_enabled_(false),
isn't nat_traversal_enabled by default?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:539: // TODO(ajwong): Do we need to
do any weird synchronization?
synchronization of what?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:165: // Ensure this object is not
watching before deleting.
This comment confuses me a bit. Are you asking the developer to ensure this
object is not watching (what?) before deleting, or are you implying that the
scoped_ptr does that for me (I'm reading this coming as someone who doesn't know
the source).

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:169: bool policy_received_;
could we add quick comments on these (especially the closure)

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:20: // Watches the policy files
for a change to the remote access host NAT
this is not actually true for all platforms. How about just "Watches for changes
to the remote access host NAT policies".

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:19: #include
"remoting/host/plugin/policy_hack/nat_policy.h"
should this include be at the top (given that it is the "class" include?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:56: // Detach the factory
cause we handle ensure that only the message_loop_
s/cause/because/
handle ensure?
s/uses/threads/?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:64: }
NIT: standard seems to be to put {} on one line...

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:120: // Needed since
NatPolicyLinux ends up getting its refcount from
NatPolicyLinux ends up getting its refcount from NatPolicyLinux? I'm confused.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_win.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_win.cc:7: #include
"remoting/host/plugin/policy_hack/nat_policy.h"
should this include be first?

(same for other platform specializations)

[awong, 2011-08-11 19:04:07]

addressed comments.

http://codereview.chromium.org/7599017/diff/20012/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/20012/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:71: // If there is no
host_resolver_factory_, we're not sandboxes, so we
On 2011/08/11 15:24:31, Jamie wrote:
> Nit: s/sandboxes/sandboxed/ :)
> 
> Also, isn't it conventional to encode variable names in | characters?

Done.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:87: nat_traversal_enabled_(false),
On 2011/08/11 18:42:55, dmac wrote:
> isn't nat_traversal_enabled by default?

I want the policy code to own the on-off logic, so the having a neutral state of
borked seems like a good thing. :)

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:539: // TODO(ajwong): Do we need to
do any weird synchronization?
On 2011/08/11 18:42:55, dmac wrote:
> synchronization of what?

Stale comment. was wondering if we needed to signal the UI. We don't.
DisconnectInternal does that.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:165: // Ensure this object is not
watching before deleting.
On 2011/08/11 18:42:55, dmac wrote:
> This comment confuses me a bit. Are you asking the developer to ensure this
> object is not watching (what?) before deleting, or are you implying that the
> scoped_ptr does that for me (I'm reading this coming as someone who doesn't
know
> the source).

Moved comment into nat_policy.h where it belongs.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:169: bool policy_received_;
On 2011/08/11 18:42:55, dmac wrote:
> could we add quick comments on these (especially the closure)

Added (non-quick) comments.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:20: // Watches the policy files
for a change to the remote access host NAT
On 2011/08/11 18:42:55, dmac wrote:
> this is not actually true for all platforms. How about just "Watches for
changes
> to the remote access host NAT policies".

done

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:19: #include
"remoting/host/plugin/policy_hack/nat_policy.h"
On 2011/08/11 18:42:55, dmac wrote:
> should this include be at the top (given that it is the "class" include?

Done.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:56: // Detach the factory
cause we handle ensure that only the message_loop_
On 2011/08/11 18:42:55, dmac wrote:
> s/cause/because/
> handle ensure?
> s/uses/threads/?

Rewrote.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:64: }
On 2011/08/11 18:42:55, dmac wrote:
> NIT: standard seems to be to put {} on one line...

I don't think there is actually a standard on this one.  But moved cause I don't
care :)

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:120: // Needed since
NatPolicyLinux ends up getting its refcount from
On 2011/08/11 18:42:55, dmac wrote:
> NatPolicyLinux ends up getting its refcount from NatPolicyLinux? I'm confused.

Again stale.  Originally, itw as getting its refcount from NatPolicy.

But I managed to keep NatPolicyLinux unrefcounted.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_win.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_win.cc:7: #include
"remoting/host/plugin/policy_hack/nat_policy.h"
On 2011/08/11 18:42:55, dmac wrote:
> should this include be first?
> 
> (same for other platform specializations)

Done.

[Wez, 2011-08-11 20:00:27]

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:537: // When transitioning from true
to false, force a disconnect.
nit: true->enabled, false->disabled, force a disconnect -> disconnect any
existing session.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:119: const std::string& auth_service);
Can we list these methods here in the order they are invoked, and perhaps
re-name them to ConnectInternal and ConnectInternal2, to make it clearer which
one is the entry-point?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:36: // Implemented by each
platform.
Document which MessageLoop needs to be passed, and under what circumstances the
callback will be called.
Why pass the callback here, rather than to StartWatching()?

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:3: // found in the LICENSE
file.
Is this brand-new code, or cribbed from the main Chrome app?

http://codereview.chromium.org/7599017/diff/19005/remoting/DEPS
File remoting/DEPS (right):

http://codereview.chromium.org/7599017/diff/19005/remoting/DEPS#newcode2
remoting/DEPS:2: "+content/common",  # Hack for NPAPI policy support. REMOVE
ASAP.
Do we have a bug for removing the hack?

http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:73:
stun_hosts_.push_back(talk_base::SocketAddress(host, port));
This doesn't seem related to the rest of the CL?

http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp#newco...
remoting/remoting.gyp:180: '../content/common/json_value_serializer.h',
Ick.  Do these need a comment akin to the one in DEPS?

[awong, 2011-08-11 23:54:53]

PTAL

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:537: // When transitioning from true
to false, force a disconnect.
On 2011/08/11 20:00:27, Wez wrote:
> nit: true->enabled, false->disabled, force a disconnect -> disconnect any
> existing session.

Done.

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.h:119: const std::string& auth_service);
On 2011/08/11 20:00:27, Wez wrote:
> Can we list these methods here in the order they are invoked, and perhaps
> re-name them to ConnectInternal and ConnectInternal2, to make it clearer which
> one is the entry-point?

I reordered them, and renamed ConnectInternal() to FinishConnect()

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy.h (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy.h:36: // Implemented by each
platform.
On 2011/08/11 20:00:27, Wez wrote:
> Document which MessageLoop needs to be passed, and under what circumstances
the
> callback will be called.

Done.

> Why pass the callback here, rather than to StartWatching()?

No real good reason...moved to StartWatching().

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:3: // found in the LICENSE
file.
On 2011/08/11 20:00:27, Wez wrote:
> Is this brand-new code, or cribbed from the main Chrome app?

Mostly cribbed, but it's an amalgamation of a bunch of different files.

http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
File remoting/jingle_glue/jingle_info_request.cc (right):

http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
remoting/jingle_glue/jingle_info_request.cc:73:
stun_hosts_.push_back(talk_base::SocketAddress(host, port));
On 2011/08/11 20:00:27, Wez wrote:
> This doesn't seem related to the rest of the CL?

Yesish.  Without it, when NAT traversal is enabled, we segfault.

http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp
File remoting/remoting.gyp (right):

http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp#newco...
remoting/remoting.gyp:180: '../content/common/json_value_serializer.h',
On 2011/08/11 20:00:27, Wez wrote:
> Ick.  Do these need a comment akin to the one in DEPS?

Good point. added.

[dmac, 2011-08-11 23:58:37]

On 2011/08/11 23:54:53, awong wrote:
> PTAL
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
> File remoting/host/plugin/host_script_object.cc (right):
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
> remoting/host/plugin/host_script_object.cc:537: // When transitioning from
true
> to false, force a disconnect.
> On 2011/08/11 20:00:27, Wez wrote:
> > nit: true->enabled, false->disabled, force a disconnect -> disconnect any
> > existing session.
> 
> Done.
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
> File remoting/host/plugin/host_script_object.h (right):
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/host_s...
> remoting/host/plugin/host_script_object.h:119: const std::string&
auth_service);
> On 2011/08/11 20:00:27, Wez wrote:
> > Can we list these methods here in the order they are invoked, and perhaps
> > re-name them to ConnectInternal and ConnectInternal2, to make it clearer
which
> > one is the entry-point?
> 
> I reordered them, and renamed ConnectInternal() to FinishConnect()
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
> File remoting/host/plugin/policy_hack/nat_policy.h (right):
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
> remoting/host/plugin/policy_hack/nat_policy.h:36: // Implemented by each
> platform.
> On 2011/08/11 20:00:27, Wez wrote:
> > Document which MessageLoop needs to be passed, and under what circumstances
> the
> > callback will be called.
> 
> Done.
> 
> > Why pass the callback here, rather than to StartWatching()?
> 
> No real good reason...moved to StartWatching().
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
> File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):
> 
>
http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
> remoting/host/plugin/policy_hack/nat_policy_linux.cc:3: // found in the
LICENSE
> file.
> On 2011/08/11 20:00:27, Wez wrote:
> > Is this brand-new code, or cribbed from the main Chrome app?
> 
> Mostly cribbed, but it's an amalgamation of a bunch of different files.
> 
>
http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
> File remoting/jingle_glue/jingle_info_request.cc (right):
> 
>
http://codereview.chromium.org/7599017/diff/19005/remoting/jingle_glue/jingle...
> remoting/jingle_glue/jingle_info_request.cc:73:
> stun_hosts_.push_back(talk_base::SocketAddress(host, port));
> On 2011/08/11 20:00:27, Wez wrote:
> > This doesn't seem related to the rest of the CL?
> 
> Yesish.  Without it, when NAT traversal is enabled, we segfault.
> 
> http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp
> File remoting/remoting.gyp (right):
> 
>
http://codereview.chromium.org/7599017/diff/19005/remoting/remoting.gyp#newco...
> remoting/remoting.gyp:180: '../content/common/json_value_serializer.h',
> On 2011/08/11 20:00:27, Wez wrote:
> > Ick.  Do these need a comment akin to the one in DEPS?
> 
> Good point. added.

LGTM

[Wez, 2011-08-12 00:54:41]

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:3: // found in the LICENSE
file.
On 2011/08/11 23:54:54, awong wrote:
> On 2011/08/11 20:00:27, Wez wrote:
> > Is this brand-new code, or cribbed from the main Chrome app?
> 
> Mostly cribbed, but it's an amalgamation of a bunch of different files.

Perhaps add a comment indicating where it's cribbed from, assuming it's not too
many, for future reference?

[awong, 2011-08-12 00:57:03]

PTAL

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
File remoting/host/plugin/policy_hack/nat_policy_linux.cc (right):

http://codereview.chromium.org/7599017/diff/21003/remoting/host/plugin/policy...
remoting/host/plugin/policy_hack/nat_policy_linux.cc:3: // found in the LICENSE
file.
On 2011/08/12 00:54:42, Wez wrote:
> On 2011/08/11 23:54:54, awong wrote:
> > On 2011/08/11 20:00:27, Wez wrote:
> > > Is this brand-new code, or cribbed from the main Chrome app?
> > 
> > Mostly cribbed, but it's an amalgamation of a bunch of different files.
> 
> Perhaps add a comment indicating where it's cribbed from, assuming it's not
too
> many, for future reference?

Done.

[Wez, 2011-08-12 00:58:59]

LGTM!

[commit-bot: I haz the power, 2011-08-15 19:12:19]

Commit queue patch verification failed without an error message.
Something went wrong, probably a crash, a hickup or simply
the monkeys went out for diner.
Ping the relevant dude on a on-needed basis.

[M-A Ruel, 2011-08-15 20:33:22]

Found the bug, checked the commit bit again.