Implement HttpIceConfigRequest

Implement HttpIceConfigRequest

In the future HttpIceConfigRequest will be used to fetch STUN/TURN
configuration instead of JingleInfoRequest.

BUG=577954
Committed: https://crrev.com/ba52044ca3d89f7bc2593cb69c124365997c636c
Cr-Commit-Position: refs/heads/master@{#376600}

[Sergey Ulanov, 2016-02-18 02:19:41]

Patchset #1 (id:1) has been deleted

[Sergey Ulanov, 2016-02-18 02:20:27]

sergeyu@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[Jamie, 2016-02-18 19:37:38]

I have concerns about the error handling. If we're going to do our best to try
to connect even if the server returns bad data, I think we should at least make
that explicit in the class documentation, but in general I prefer failing with
an obvious error message in such cases.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
File remoting/protocol/http_ice_config_request.cc (right):

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:21: // Ensure they are active at
least one hour after session starts.
s/they/<whatever "they" refers to in this context>/ :)

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:32: LOG(ERROR) << "Received invalid
lifetimeDuration value: " << string;
For consistency with the other error messages, I think it would be better to log
this in the caller.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:67: return false;
Optional: This might be more readable using StringSplit, since you have either
two or three colon-separated components.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:108: on_ice_config_callback_ =
callback;
DCHECK it isn't already set (and that it is set in OnResponse)?

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:124:
base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
In case of error, you seem to be returning as much of the config as you were
able to parse, which means that the caller can't tell anything went wrong. I
think it would make more sense to return an empty config in this case, or
provide some other mechanism to detect errors.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:152: bool errors_found = false;
Why not just return early in case of error? Particularly if you take my
suggestion of returning an empty config instead of parsing as much as you can.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:179: LOG(ERROR) << "Invalid ICE
server URL: " << url_str;
errors_found = true?

[Sergey Ulanov, 2016-02-19 20:08:32]

On 2016/02/18 19:37:38, Jamie wrote:
> I have concerns about the error handling. If we're going to do our best to try
> to connect even if the server returns bad data, I think we should at least
make
> that explicit in the class documentation, but in general I prefer failing with
> an obvious error message in such cases.

Added comments and cleaned up error handling. IMO it doesn't make sense to fail
session after we've failed to fetch ICE config. It may still succeed using
direct P2P connection.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
File remoting/protocol/http_ice_config_request.cc (right):

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:21: // Ensure they are active at
least one hour after session starts.
On 2016/02/18 19:37:38, Jamie wrote:
> s/they/<whatever "they" refers to in this context>/ :)

Done.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:32: LOG(ERROR) << "Received invalid
lifetimeDuration value: " << string;
On 2016/02/18 19:37:38, Jamie wrote:
> For consistency with the other error messages, I think it would be better to
log
> this in the caller.

Done.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:67: return false;
On 2016/02/18 19:37:37, Jamie wrote:
> Optional: This might be more readable using StringSplit, since you have either
> two or three colon-separated components.

StringSplit wouldn't work for IPv6 addresses. IPv6 addresses contain colons and
are wrapped with square brackets, e.g. "turn:[2001:4860:4860::8888]:333"

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:108: on_ice_config_callback_ =
callback;
On 2016/02/18 19:37:38, Jamie wrote:
> DCHECK it isn't already set (and that it is set in OnResponse)?

Done.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:124:
base::ResetAndReturn(&on_ice_config_callback_).Run(ice_config);
On 2016/02/18 19:37:38, Jamie wrote:
> In case of error, you seem to be returning as much of the config as you were
> able to parse, which means that the caller can't tell anything went wrong. I
> think it would make more sense to return an empty config in this case, or
> provide some other mechanism to detect errors.

In this particular case we are returning a null IceConfig (IceConfig::is_null()
is true), which indicates an error.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:152: bool errors_found = false;
On 2016/02/18 19:37:38, Jamie wrote:
> Why not just return early in case of error? Particularly if you take my
> suggestion of returning an empty config instead of parsing as much as you can.

When we fail to fetch a config we will still want try to continue with the
connection, but without using STUN/TURN. If we get a config that's we could
parsed only partially then I think it's best try to use parts that we were able
to parse correctly. 

Let's say there is a new TURN version and the server starts returning urls in
the form of "turn2:host:port". AddServerToConfig() will fail to parse such urls.
But we still want to continue using stun and turn servers that were in the
config instead of ignoring the whole config.

https://codereview.chromium.org/1707223002/diff/20001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:179: LOG(ERROR) << "Invalid ICE
server URL: " << url_str;
On 2016/02/18 19:37:38, Jamie wrote:
> errors_found = true?

error_found is there only for structural errors, e.g. when there are no urls
specified for a server. In that case we want to dump the whole config for logs
to make it easier to debug. In this case, when an URL is invalid it's enough to
just display that one URL.

[Sergey Ulanov, 2016-02-19 20:08:52]

Patchset #2 (id:40001) has been deleted

[Jamie, 2016-02-19 22:25:29]

lgtm

https://codereview.chromium.org/1707223002/diff/60001/remoting/protocol/http_...
File remoting/protocol/http_ice_config_request.cc (right):

https://codereview.chromium.org/1707223002/diff/60001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:116: void
HttpIceConfigRequest::OnResponse(const UrlRequest::Result& result) {
DCHECK(!on_ice_config_callback.is_null())?

[Sergey Ulanov, 2016-02-19 22:30:32]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-02-19 22:30:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1707223002/#ps60002
(title: " ")

[commit-bot: I haz the power, 2016-02-19 22:35:04]

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

[Sergey Ulanov, 2016-02-19 22:44:26]

https://codereview.chromium.org/1707223002/diff/60001/remoting/protocol/http_...
File remoting/protocol/http_ice_config_request.cc (right):

https://codereview.chromium.org/1707223002/diff/60001/remoting/protocol/http_...
remoting/protocol/http_ice_config_request.cc:116: void
HttpIceConfigRequest::OnResponse(const UrlRequest::Result& result) {
On 2016/02/19 22:25:28, Jamie wrote:
> DCHECK(!on_ice_config_callback.is_null())?

Done.

[commit-bot: I haz the power, 2016-02-19 23:09:34]

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

[commit-bot: I haz the power, 2016-02-19 23:09:35]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sergey Ulanov, 2016-02-20 00:56:29]

The CQ bit was checked by sergeyu@chromium.org

[Sergey Ulanov, 2016-02-20 00:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamiewalch@chromium.org
Link to the patchset: https://codereview.chromium.org/1707223002/#ps90001
(title: " ")

[commit-bot: I haz the power, 2016-02-20 00:57:36]

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

[commit-bot: I haz the power, 2016-02-20 01:43:39]

Committed patchset #4 (id:90001)

[commit-bot: I haz the power, 2016-02-20 01:45:25]

Description was changed from

==========
Implement HttpIceConfigRequest

In the future HttpIceConfigRequest will be used to fetch STUN/TURN
configuration instead of JingleInfoRequest.

BUG=577954
==========

to

==========
Implement HttpIceConfigRequest

In the future HttpIceConfigRequest will be used to fetch STUN/TURN
configuration instead of JingleInfoRequest.

BUG=577954

Committed: https://crrev.com/ba52044ca3d89f7bc2593cb69c124365997c636c
Cr-Commit-Position: refs/heads/master@{#376600}
==========

[commit-bot: I haz the power, 2016-02-20 01:45:26]

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