Support LCS signaling address in the Host.

Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
Committed: https://crrev.com/d05ed1f6f63b4e2612e8ac0b638da1ae7ff30ee6
Cr-Commit-Position: refs/heads/master@{#394828}

[kelvinp, 2016-05-17 21:01:12]

Description was changed from

==========
Host changes for lcs signaling addressing.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
==========

to

==========
Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
==========

[kelvinp, 2016-05-17 21:01:12]

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

[kelvinp, 2016-05-17 21:01:44]

PTAL

[Sergey Ulanov, 2016-05-18 07:00:42]

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.cc (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:188: : jid(""), endpoint_id(""),
channel(Address::XMPP) {}
don't need to specify jid and endpoint_id explicitly here.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:208: if (endpoint_id.empty()) {
I think it's better to branch here by channel and verify that endpoint_id is
either set for LCS or not set for XMPP.

Or this can be something like this:
  bool is_lcs = (channel == Address::LCS);
  if (is_lcs == endpoint_id.empty()) {
    boom
  }

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:214: } else if (jid.empty()) {
I think we always expect to have a valid jid, so this shouldn't be an else
statement. Move it right after we extract JID?

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:215: *error = "|endpoint-id| is not empty
but the |jid| is empty.";
indentation. 'git cl format' please

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:230: LOG(FATAL) << "Invalid channel value "
<< channel;
Don't really need this. This case is already marked as NOTREACHED()  in
ValueToName()

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:234: if (endpoint_id != jid) {
Replace this condition with (channel == Address::LCS)? It's easier to understand
and is faster.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:475: 
remove extra empty line

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.h (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:21: struct Address {
I think this belongs to remoting/signaling. Potentially we could update
SignalStrategy interface to use this instead of jid for addressing. Maybe keep
it here for now, but add a TODO.
Also it should be called SignalingAddress. Address is a too generic name.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:22: enum Channel {
enum class please, here and for enums below

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:27: enum Role {
this enum is confusing. I suggest removing it and replace ParseXml and ToXml
with GetTo(), GetFrom(), SetTo() and SetFrom().

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:46: // Represents the identifier of an
endpoint. In XMPP, this is the JID.  In
It feels wrong that we duplicate JID for XMPP. Why not store these values the
same values the way they are present in XML. I.e. set endpoint_id only when
channel = LCS and keep it empty otherwise.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:56: bool ParseXml(const buzz::XmlElement*
stanza, Role role, std::string* error);
I don't think these two functions need to be class members. You can replace them
with static helpers in jingle_messages.cc

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:60: void toXml(buzz::XmlElement* iqElement,
Role role) const;
I don't think toXml() is a good name as it doesn't return XML.
I suggest replacing with SetToField() and SetFromField(), without the role
parameter.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:62: bool empty() const;
is_empty().
inline this method?

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.h:119: Address peer_;
peer_address_

[kelvinp, 2016-05-18 23:43:29]

PTAL.

I also tested my change with the webapp and it is able to connect.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.cc (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:188: : jid(""), endpoint_id(""),
channel(Address::XMPP) {}
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> don't need to specify jid and endpoint_id explicitly here.

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:208: if (endpoint_id.empty()) {
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> I think it's better to branch here by channel and verify that endpoint_id is
> either set for LCS or not set for XMPP.
> 
> Or this can be something like this:
>   bool is_lcs = (channel == Address::LCS);
>   if (is_lcs == endpoint_id.empty()) {
>     boom
>   }

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:214: } else if (jid.empty()) {
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> I think we always expect to have a valid jid, so this shouldn't be an else
> statement. Move it right after we extract JID?

Acknowledged.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:215: *error = "|endpoint-id| is not empty
but the |jid| is empty.";
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> indentation. 'git cl format' please

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:230: LOG(FATAL) << "Invalid channel value "
<< channel;
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> Don't really need this. This case is already marked as NOTREACHED()  in
> ValueToName()

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:234: if (endpoint_id != jid) {
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> Replace this condition with (channel == Address::LCS)? It's easier to
understand
> and is faster.

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.cc:475: 
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> remove extra empty line

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
File remoting/protocol/jingle_messages.h (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:21: struct Address {
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> I think this belongs to remoting/signaling. Potentially we could update
> SignalStrategy interface to use this instead of jid for addressing. Maybe keep
> it here for now, but add a TODO.
> Also it should be called SignalingAddress. Address is a too generic name. 

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:22: enum Channel {
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> enum class please, here and for enums below

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:27: enum Role {
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> this enum is confusing. I suggest removing it and replace ParseXml and ToXml
> with GetTo(), GetFrom(), SetTo() and SetFrom().

Removed.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:46: // Represents the identifier of an
endpoint. In XMPP, this is the JID.  In
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> It feels wrong that we duplicate JID for XMPP. Why not store these values the
> same values the way they are present in XML. I.e. set endpoint_id only when
> channel = LCS and keep it empty otherwise.

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:56: bool ParseXml(const buzz::XmlElement*
stanza, Role role, std::string* error);
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> I don't think these two functions need to be class members. You can replace
them
> with static helpers in jingle_messages.cc

Removed.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:60: void toXml(buzz::XmlElement* iqElement,
Role role) const;
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> I don't think toXml() is a good name as it doesn't return XML.
> I suggest replacing with SetToField() and SetFromField(), without the role
> parameter.

Removed.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_me...
remoting/protocol/jingle_messages.h:62: bool empty() const;
On 2016/05/18 07:00:41, Sergey Ulanov wrote:
> is_empty().
> inline this method?

Done.

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_se...
File remoting/protocol/jingle_session.h (right):

https://codereview.chromium.org/1984383003/diff/1/remoting/protocol/jingle_se...
remoting/protocol/jingle_session.h:119: Address peer_;
On 2016/05/18 07:00:42, Sergey Ulanov wrote:
> peer_address_

Done.

[kelvinp, 2016-05-19 00:41:33]

The CQ bit was checked by kelvinp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-05-19 00:42:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1984383003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1984383003/20001

[commit-bot: I haz the power, 2016-05-19 02:24:21]

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

[commit-bot: I haz the power, 2016-05-19 02:24:22]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-05-19 17:39:59]

lgtm

[kelvinp, 2016-05-19 18:04:39]

The CQ bit was checked by kelvinp@chromium.org

[kelvinp, 2016-05-19 18:04:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/1984383003/#ps40001
(title: "Add DCHECK")

[commit-bot: I haz the power, 2016-05-19 18:05:16]

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

[commit-bot: I haz the power, 2016-05-19 19:20:46]

Description was changed from

==========
Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
==========

to

==========
Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
==========

[commit-bot: I haz the power, 2016-05-19 19:20:47]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-19 19:23:47]

Description was changed from

==========
Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588
==========

to

==========
Support LCS signaling address in the Host.

This CL introduces a new struct protocol::Address that encapsulates the
routing address of the chromoting endpoints.

This will allow us to route signaling messages using both XMPP and LCS.

BUG=612588

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

[commit-bot: I haz the power, 2016-05-19 19:23:48]

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