[Chromoting] Hook up host talkgadget policy checks.

[Chromoting] Hook up host talkgadget policy checks.

This updates the host to ping the specified talkgadget and block startup
if the talkgadget is not reachable. This permits admins to DNS block the
talkgadget to disable hosts from sharing out from their network.

Also, update the policy watcher to watch the correct location for Chromium builds.

BUG=132345, 132681
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=154583

[garykac, 2012-08-23 22:08:49]

http://codereview.chromium.org/10873050/diff/1/remoting/host/remoting_me2me_h...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10873050/diff/1/remoting/host/remoting_me2me_h...
remoting/host/remoting_me2me_host.cc:586: ResetHost();
Note that calling ResetHost here resets the SignalingConnector as well. This is
needed to reset the connection_attempts counter (or else updating the policy can
take up to 10 min before it will try to connect again).

[Sergey Ulanov, 2012-08-24 19:43:33]

I think it's better to add a new class that will take care of verifying that we
are not blocked.

Also do we want the same policy to apply for It2Me host?

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...
remoting/host/constants.h:15: extern const char kDefaultHostTalkGadgetPrefix[];
We should avoid adding more values to this file (ideally this file shouldn't
exist at all). 
kDefaultHostTalkGadgetPrefix is used only in policy_watcher.cc, so it might be
better to move it there.
kTalkGadgetURL is also used in only one file.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...
remoting/host/constants.h:18: extern const char kTalkGadgetURL[];
This should be kTalkGadgetUrl. From chromium code style guide:
  Prefer to capitalize well-known abbreviations as if they were words: HttpUrl
instead of HTTPURL.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/policy_hack/p...
File remoting/host/policy_hack/policy_watcher_linux.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/policy_hack/p...
remoting/host/policy_hack/policy_watcher_linux.cc:39:
FILE_PATH_LITERAL("/etc/chromium/policies/managed");
Was it intentional change?
We always want to look at the Chrome's policies, even in chromium builds.
Otherwise it would be really easy to workaround chrome policies by running
chromium build. That's also the way Mac and Windows policy watchers work.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:476: if (!signal_strategy_.get()) {
replace this with DCHECK(!signal_strategy_.get())?

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:483: signal_strategy_.get(),
context_.get(), talkgadget_prefix_,
I think you need to update simple_host as it uses SignalingConnector too.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:95: // net::URLFetcherDelegate interface.
I don't think we need to duplicate this comment from the header. E.g. in this
particular case it would be more useful to comment that this is called in
response to talk-gadget HTTP request initiated in TryReconnect().

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:106: ScheduleTryReconnect();
If we know we are blocked then there isn't point in retrying the connection.
Should we shutdown the host in that case?

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.h:36: public net::URLFetcherDelegate,
Can you please move the new connectivity checking logic to a separate class
(e.g. call it ConnectionBlockChecker or something like that)?
This class is already complicated as is (it inherits from 6 classes already!). 
Also moving this logic outside of this class will allow to turn the new policy
off and on very easily by supplying different implementations of
ConnectionBlockChecker.

[garykac, 2012-08-27 22:34:56]

ptal

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...
remoting/host/constants.h:15: extern const char kDefaultHostTalkGadgetPrefix[];
On 2012/08/24 19:43:33, sergeyu wrote:
> We should avoid adding more values to this file (ideally this file shouldn't
> exist at all). 

I would rather merge constants_mac.cc and constants_win.cc into this file.

> kDefaultHostTalkGadgetPrefix is used only in policy_watcher.cc, so it might be
> better to move it there.
> kTalkGadgetURL is also used in only one file.

The _unittest file uses it as well.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...
remoting/host/constants.h:18: extern const char kTalkGadgetURL[];
On 2012/08/24 19:43:33, sergeyu wrote:
> This should be kTalkGadgetUrl. From chromium code style guide:
>   Prefer to capitalize well-known abbreviations as if they were words: HttpUrl
> instead of HTTPURL.

Done.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/policy_hack/p...
File remoting/host/policy_hack/policy_watcher_linux.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/policy_hack/p...
remoting/host/policy_hack/policy_watcher_linux.cc:39:
FILE_PATH_LITERAL("/etc/chromium/policies/managed");
On 2012/08/24 19:43:33, sergeyu wrote:
> Was it intentional change?
> We always want to look at the Chrome's policies, even in chromium builds.
> Otherwise it would be really easy to workaround chrome policies by running
> chromium build. That's also the way Mac and Windows policy watchers work.

Ah. I didn't realize that (and it's easier to edit my chromium settings). I've
added a comment.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:476: if (!signal_strategy_.get()) {
On 2012/08/24 19:43:33, sergeyu wrote:
> replace this with DCHECK(!signal_strategy_.get())?

Done.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/remoting_me2m...
remoting/host/remoting_me2me_host.cc:483: signal_strategy_.get(),
context_.get(), talkgadget_prefix_,
On 2012/08/24 19:43:33, sergeyu wrote:
> I think you need to update simple_host as it uses SignalingConnector too.

Done.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:95: // net::URLFetcherDelegate interface.
On 2012/08/24 19:43:33, sergeyu wrote:
> I don't think we need to duplicate this comment from the header. E.g. in this
> particular case it would be more useful to comment that this is called in
> response to talk-gadget HTTP request initiated in TryReconnect().

Done.

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:106: ScheduleTryReconnect();
On 2012/08/24 19:43:33, sergeyu wrote:
> If we know we are blocked then there isn't point in retrying the connection.
> Should we shutdown the host in that case?

Not being able to reach the talkgadget could be for a number of reasons so we
need to retry.

Did you mean look specifically for a 404 response and kill the host?

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/signaling_con...
remoting/host/signaling_connector.h:36: public net::URLFetcherDelegate,
On 2012/08/24 19:43:33, sergeyu wrote:
> Can you please move the new connectivity checking logic to a separate class
> (e.g. call it ConnectionBlockChecker or something like that)?
> This class is already complicated as is (it inherits from 6 classes already!).

> Also moving this logic outside of this class will allow to turn the new policy
> off and on very easily by supplying different implementations of
> ConnectionBlockChecker.

Done.

[Sergey Ulanov, 2012-08-28 00:33:41]

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...
remoting/host/constants.h:15: extern const char kDefaultHostTalkGadgetPrefix[];
On 2012/08/27 22:34:57, garykac wrote:
> On 2012/08/24 19:43:33, sergeyu wrote:
> > We should avoid adding more values to this file (ideally this file shouldn't
> > exist at all). 
> 
> I would rather merge constants_mac.cc and constants_win.cc into this file.

This has been discussed some time ago:
https://chromiumcodereview.appspot.com/10579002/
It's best to avoid dumping all shared constants in one file.

> 
> > kDefaultHostTalkGadgetPrefix is used only in policy_watcher.cc, so it might
be
> > better to move it there.
> > kTalkGadgetURL is also used in only one file.
> 
> The _unittest file uses it as well.

maybe put them in connection_block_checker.h ?

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
File remoting/host/connection_block_checker.cc (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.cc:37: if (response == 404 ||
reconnect_attempts_ > kMaxConnectionBlockChecks) {
We may get 404 if there is a misbehaving proxy, so I'm not sure we can rely on
it.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.cc:47: ConnectionBlockChecker::Status
ConnectionBlockChecker::GetStatus() {
Here we check connection status only once when this code is called for the first
time. Problem is that if chromoting gets blocked later we will not know about
it. It's better to check before every single connection. It's even a bigger
problem if first kMaxConnectionBlockChecks connections fail just because of a
network failure. We will never be able to connect again, even after network
starts working. I suggest you make this method asynchronous and make a new URL
request every time it's called, e.g.
void GetBlockingState(const base::Callback<State>& callback);

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
File remoting/host/connection_block_checker.h (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:19: DENY,
UNKNOWN/DENY distinction is not reliable, so I think it's better to have only
two states - ALLOW and DENY.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:24: virtual ~ConnectionBlockChecker()
{}
move destructor implementation to the .cc file. clang bots will not like it
here.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:51: };
nit: incorrect indentation

http://codereview.chromium.org/10873050/diff/2015/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:181: return;
This will keep the host running even though we know that we will not be able to
connect. It would be better if we could shutdown the host if we know that it's
being blocked. Problem is that we cannot distinguish between it being blocked
and general network failure :(
So it doesn't look right to distinguish between DENY and UNKNOWN as they are not
reliable anyway. Probably it's better call ScheduleTryReconnect() either way -
that will also handle the case when access gets reenabled by the network
administrator.

The case that I'm worried about is that if user starts the host and everything
will look like the host is started, but it won't be possible to connect anyway.
Maybe we should add a check in the webapp (or the plugin) so that it would
verify that we are not blocked before enabling the host? What do you think? It
doesn't have to be part of this CL.

[Wez, 2012-08-28 01:24:56]

Superhypersonicdrive-by-review-nit: Can we not call it ConnectionBlockChecker?
For historical reasons, I parsed that name completely wrongly when I read it...
say TalkReachableChecker?

[Sergey Ulanov, 2012-08-28 01:35:32]

On 2012/08/28 01:24:56, Wez wrote:
> Superhypersonicdrive-by-review-nit: Can we not call it ConnectionBlockChecker?
> For historical reasons, I parsed that name completely wrongly when I read
it...
> say TalkReachableChecker?

Problem with TalkReacheableChecker is that it implies specific blocking scheme,
whereas we may want implement a different approach later.

[Wez, 2012-08-28 01:37:06]

On 2012/08/28 01:35:32, sergeyu wrote:
> On 2012/08/28 01:24:56, Wez wrote:
> > Superhypersonicdrive-by-review-nit: Can we not call it
ConnectionBlockChecker?
> > For historical reasons, I parsed that name completely wrongly when I read
> it...
> > say TalkReachableChecker?
> 
> Problem with TalkReacheableChecker is that it implies specific blocking
scheme,
> whereas we may want implement a different approach later.

OK; how about NetworkBlockChecker, since this class tests whether the host
network blocks Chromoting, or EnterpriseBlockChecker, since it's really for
enterprise environments?

[Sergey Ulanov, 2012-08-28 01:39:01]

On 2012/08/28 01:37:06, Wez wrote:
> On 2012/08/28 01:35:32, sergeyu wrote:
> > On 2012/08/28 01:24:56, Wez wrote:
> > > Superhypersonicdrive-by-review-nit: Can we not call it
> ConnectionBlockChecker?
> > > For historical reasons, I parsed that name completely wrongly when I read
> > it...
> > > say TalkReachableChecker?
> > 
> > Problem with TalkReacheableChecker is that it implies specific blocking
> scheme,
> > whereas we may want implement a different approach later.
> 
> OK; how about NetworkBlockChecker, since this class tests whether the host
> network blocks Chromoting, or EnterpriseBlockChecker, since it's really for
> enterprise environments?

I'm fine with either of your suggestions.

[garykac, 2012-08-29 00:07:15]

I still need to talk to wez about renaming the connection_block_checker, but
ptal at the rest.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
File remoting/host/connection_block_checker.cc (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.cc:37: if (response == 404 ||
reconnect_attempts_ > kMaxConnectionBlockChecks) {
On 2012/08/28 00:33:42, sergeyu wrote:
> We may get 404 if there is a misbehaving proxy, so I'm not sure we can rely on
> it.

Done.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.cc:47: ConnectionBlockChecker::Status
ConnectionBlockChecker::GetStatus() {
On 2012/08/28 00:33:42, sergeyu wrote:
> Here we check connection status only once when this code is called for the
first
> time. Problem is that if chromoting gets blocked later we will not know about
> it. It's better to check before every single connection. It's even a bigger
> problem if first kMaxConnectionBlockChecks connections fail just because of a
> network failure. We will never be able to connect again, even after network
> starts working. I suggest you make this method asynchronous and make a new URL
> request every time it's called, e.g.
> void GetBlockingState(const base::Callback<State>& callback);

Done.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
File remoting/host/connection_block_checker.h (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:19: DENY,
On 2012/08/28 00:33:42, sergeyu wrote:
> UNKNOWN/DENY distinction is not reliable, so I think it's better to have only
> two states - ALLOW and DENY.

Removed.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:24: virtual ~ConnectionBlockChecker()
{}
On 2012/08/28 00:33:42, sergeyu wrote:
> move destructor implementation to the .cc file. clang bots will not like it
> here.

Done.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/connection_bl...
remoting/host/connection_block_checker.h:51: };
On 2012/08/28 00:33:42, sergeyu wrote:
> nit: incorrect indentation

Done.

http://codereview.chromium.org/10873050/diff/2015/remoting/host/signaling_con...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/2015/remoting/host/signaling_con...
remoting/host/signaling_connector.cc:181: return;
On 2012/08/28 00:33:42, sergeyu wrote:
> This will keep the host running even though we know that we will not be able
to
> connect. It would be better if we could shutdown the host if we know that it's
> being blocked. Problem is that we cannot distinguish between it being blocked
> and general network failure :(
> So it doesn't look right to distinguish between DENY and UNKNOWN as they are
not
> reliable anyway. Probably it's better call ScheduleTryReconnect() either way -
> that will also handle the case when access gets reenabled by the network
> administrator.

Done.

> The case that I'm worried about is that if user starts the host and everything
> will look like the host is started, but it won't be possible to connect
anyway.
> Maybe we should add a check in the webapp (or the plugin) so that it would
> verify that we are not blocked before enabling the host? What do you think? It
> doesn't have to be part of this CL.

Yes, it would certainly be nice if the webapp could display this. It doesn't
have direct access to the policy values. But we could do it in the plugin or
expose the talkgadget url to the webapp.

[Sergey Ulanov, 2012-08-29 20:10:29]

Looks mostly good, I just have some nits about interface provided by
ConnectionBlockChecker

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
File remoting/host/connection_block_checker.cc (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.cc:59: LOG(INFO) << "Pending connection
check";
I think it's better to DCHECK(!url_fetcher_.get()) in the beginning of this
method.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
File remoting/host/connection_block_checker.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:20: const base::Callback<void(bool)>&
callback);
it would be cleaner to pass callback to CheckStatus() method - that will allow
to use different callbacks for each check (e.g. in case CheckStatus() caller
needs to pass some additional info to the callback).

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:24: // talkgadget. Calls |callback_|
with the result.
nit: Calls the the |callback| specified in the constructor. 
callback_ is private so it's better not to mention it when describing public
interface.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:29: virtual void
OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
interface implementation must be public.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/constants.h#...
remoting/host/constants.h:18: extern const char kTalkGadgetUrl[];
please see my comment about this const here:
http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:178: // host talkgadget.
OnConnectionBlockCheckerDone will be called with the
You would need this comment if callback was passed directly to CheckStatus() :)

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
remoting/host/signaling_connector.h:59: ChromotingHostContext* context,
Maybe pass ConnectionBlockChecker instance? (that requires that we pass callback
to CheckStatus instead of the constructor)

[garykac, 2012-08-29 22:30:51]

ptal

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
File remoting/host/connection_block_checker.cc (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.cc:59: LOG(INFO) << "Pending connection
check";
On 2012/08/29 20:10:29, sergeyu wrote:
> I think it's better to DCHECK(!url_fetcher_.get()) in the beginning of this
> method.

A DCHECK for this is wrong since CheckStatus can possibly be called before the
previous check finishes.  We just don't want to fire off another check in that
case.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
File remoting/host/connection_block_checker.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:20: const base::Callback<void(bool)>&
callback);
On 2012/08/29 20:10:29, sergeyu wrote:
> it would be cleaner to pass callback to CheckStatus() method - that will allow
> to use different callbacks for each check (e.g. in case CheckStatus() caller
> needs to pass some additional info to the callback).

Done.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:24: // talkgadget. Calls |callback_|
with the result.
On 2012/08/29 20:10:29, sergeyu wrote:
> nit: Calls the the |callback| specified in the constructor. 
> callback_ is private so it's better not to mention it when describing public
> interface.

Done.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/connection_b...
remoting/host/connection_block_checker.h:29: virtual void
OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
On 2012/08/29 20:10:29, sergeyu wrote:
> interface implementation must be public.

Done.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/constants.h#...
remoting/host/constants.h:18: extern const char kTalkGadgetUrl[];
On 2012/08/29 20:10:29, sergeyu wrote:
> please see my comment about this const here:
>
http://codereview.chromium.org/10873050/diff/2001/remoting/host/constants.h#n...

Done.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:178: // host talkgadget.
OnConnectionBlockCheckerDone will be called with the
On 2012/08/29 20:10:29, sergeyu wrote:
> You would need this comment if callback was passed directly to CheckStatus()
:)

Done.

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/14001/remoting/host/signaling_co...
remoting/host/signaling_connector.h:59: ChromotingHostContext* context,
On 2012/08/29 20:10:29, sergeyu wrote:
> Maybe pass ConnectionBlockChecker instance? (that requires that we pass
callback
> to CheckStatus instead of the constructor)

Done.

[Sergey Ulanov, 2012-08-30 18:05:01]

http://codereview.chromium.org/10873050/diff/13013/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/constants.h#...
remoting/host/constants.h:15: extern const char kDefaultHostTalkGadgetPrefix[];
can we move it to dns_blackhole_checker.h?

Then you can also remove constants.cc

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
File remoting/host/dns_blackhole_checker.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
remoting/host/dns_blackhole_checker.h:16: class DnsBlackholeChecker : public
net::URLFetcherDelegate {
I'm not fan for of this name for the same reason I didn't like
TalkReachableChecker that Wez suggested previously. But it's a minor concern -
feel free to keep it as is.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
remoting/host/dns_blackhole_checker.h:27: void CheckForDnsBlackhole(const
base::Callback<void(bool)>& callback);
Please add comment saying that if this method is called again before the
callback is called, then the pending check is cancelled.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:650:
scoped_ptr<base::files::FilePathWatcher> config_watcher_;
Please add an empty line above this one to separate policy-related members from
config watcher.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:657: scoped_ptr<DnsBlackholeChecker>
dns_blackhole_checker_;
We don't really use this thing here. Just pass ownership to SignalingConnector?

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (left):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:150: int delay_s = std::min(1 <<
(reconnect_attempts_ * 2),
Why remove *2 here. Do we still have the problem with the first check always
failing?

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:172: DCHECK(dns_blackhole_checker_);
maybe put this DCHECK in constructor?

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.h:60: DnsBlackholeChecker*
dns_blackhole_checker,
I think SignalingConnector should accept ownership of dns_blackhole_checker.

[Wez, 2012-08-30 20:43:38]

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
File remoting/host/dns_blackhole_checker.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
remoting/host/dns_blackhole_checker.h:16: class DnsBlackholeChecker : public
net::URLFetcherDelegate {
On 2012/08/30 18:05:01, sergeyu wrote:
> I'm not fan for of this name for the same reason I didn't like
> TalkReachableChecker that Wez suggested previously. But it's a minor concern -
> feel free to keep it as is. 

Given that we have no better ideas for naming, and that we have only a single
implementation that checks for a DNS blackhole, I'd prefer that we re-name this
if/when we need to, but I agree with you in principle!

[garykac, 2012-08-31 18:28:23]

ptal

http://codereview.chromium.org/10873050/diff/13013/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/constants.h#...
remoting/host/constants.h:15: extern const char kDefaultHostTalkGadgetPrefix[];
On 2012/08/30 18:05:01, sergeyu wrote:
> can we move it to dns_blackhole_checker.h?
> 
> Then you can also remove constants.cc

Done.
This is used by policy_watcher and dns_blackhole_checker. It  doens't make sense
to have policy_watcher include blackhole_checker.h just to get this default
value.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
File remoting/host/dns_blackhole_checker.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/dns_blackhol...
remoting/host/dns_blackhole_checker.h:27: void CheckForDnsBlackhole(const
base::Callback<void(bool)>& callback);
On 2012/08/30 18:05:01, sergeyu wrote:
> Please add comment saying that if this method is called again before the
> callback is called, then the pending check is cancelled.

Done.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:650:
scoped_ptr<base::files::FilePathWatcher> config_watcher_;
On 2012/08/30 18:05:01, sergeyu wrote:
> Please add an empty line above this one to separate policy-related members
from
> config watcher.

Done.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:657: scoped_ptr<DnsBlackholeChecker>
dns_blackhole_checker_;
On 2012/08/30 18:05:01, sergeyu wrote:
> We don't really use this thing here. Just pass ownership to
SignalingConnector?

Done.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (left):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:150: int delay_s = std::min(1 <<
(reconnect_attempts_ * 2),
On 2012/08/30 18:05:01, sergeyu wrote:
> Why remove *2 here. Do we still have the problem with the first check always
> failing?

Yes. I haven't been able to find any reason why this happens though.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:172: DCHECK(dns_blackhole_checker_);
On 2012/08/30 18:05:01, sergeyu wrote:
> maybe put this DCHECK in constructor?

Done.

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.h (right):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.h:60: DnsBlackholeChecker*
dns_blackhole_checker,
On 2012/08/30 18:05:01, sergeyu wrote:
> I think SignalingConnector should accept ownership of dns_blackhole_checker.

Done.

[Sergey Ulanov, 2012-08-31 23:40:16]

LGTM, but see my last two nits

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (left):

http://codereview.chromium.org/10873050/diff/13013/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:150: int delay_s = std::min(1 <<
(reconnect_attempts_ * 2),
On 2012/08/31 18:28:23, garykac wrote:
> On 2012/08/30 18:05:01, sergeyu wrote:
> > Why remove *2 here. Do we still have the problem with the first check always
> > failing?
> 
> Yes. I haven't been able to find any reason why this happens though. 

I actually see the same issue fetching OAuth token - the first attempt always
fails. We need to figure out why, but it's not related to this CL, so please
keep *2 here.

http://codereview.chromium.org/10873050/diff/29001/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/29001/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:190: LOG(INFO) << "Scheduling reconnect.
Attempt " << reconnect_attempts_;
I think it's better to log a message saying that we failed to connect to talk.

[garykac, 2012-08-31 23:51:13]

http://codereview.chromium.org/10873050/diff/29001/remoting/host/signaling_co...
File remoting/host/signaling_connector.cc (right):

http://codereview.chromium.org/10873050/diff/29001/remoting/host/signaling_co...
remoting/host/signaling_connector.cc:190: LOG(INFO) << "Scheduling reconnect.
Attempt " << reconnect_attempts_;
On 2012/08/31 23:40:16, sergeyu wrote:
> I think it's better to log a message saying that we failed to connect to talk.


Done.

[commit-bot: I haz the power, 2012-08-31 23:51:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/garykac@chromium.org/10873050/32001

[commit-bot: I haz the power, 2012-09-01 00:29:19]

Try job failure for 10873050-32001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-01 00:38:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/garykac@chromium.org/10873050/25003

[commit-bot: I haz the power, 2012-09-01 03:05:23]

Change committed as 154583