Parse termination reason and propagate the error to the Session interface.

Parse termination reason and propagate the error to the Session interface.

BUG=91402
TEST=None

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

[Sergey Ulanov, 2011-09-26 21:30:44]

http://codereview.chromium.org/8046018/diff/4005/remoting/protocol/jingle_ses...
File remoting/protocol/jingle_session_manager.cc (right):

http://codereview.chromium.org/8046018/diff/4005/remoting/protocol/jingle_ses...
remoting/protocol/jingle_session_manager.cc:230: cricket_session->Terminate(
Reject() doesn't include termination reason to the message, so need to call
Terminate() here.

[simonmorris, 2011-09-26 23:33:44]

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:133: void set_error(Session::Error error) {
error_ = error; }
Any reason not to have error() in this file, if
set_error() is here?

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.cc (right):

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.cc:281: }
Factor out the map from Reason to string, so it doesn't
have to be coded identically here and in lines 161-170.

[Sergey Ulanov, 2011-09-27 01:09:45]

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:133: void set_error(Session::Error error) {
error_ = error; }
On 2011/09/26 23:33:44, simonmorris wrote:
> Any reason not to have error() in this file, if
> set_error() is here?

error() is virtual, and there is clang rule that reject inline definitions for
virtual methods.

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.cc (right):

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.cc:281: }
On 2011/09/26 23:33:44, simonmorris wrote:
> Factor out the map from Reason to string, so it doesn't
> have to be coded identically here and in lines 161-170.
Done. Also refactored ActionType->string mapping.

[simonmorris, 2011-09-27 01:26:10]

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
File remoting/protocol/fake_session.h (right):

http://codereview.chromium.org/8046018/diff/6002/remoting/protocol/fake_sessi...
remoting/protocol/fake_session.h:133: void set_error(Session::Error error) {
error_ = error; }
On 2011/09/27 01:09:46, sergeyu wrote:
> On 2011/09/26 23:33:44, simonmorris wrote:
> > Any reason not to have error() in this file, if
> > set_error() is here?
> 
> error() is virtual, and there is clang rule that reject inline definitions for
> virtual methods.

OK.

http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.cc (right):

http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.cc:52: if (map[i].name == name)
Is this doing a pointer comparison instead of a string
comparison? A unit test would make sure.

[Sergey Ulanov, 2011-09-27 03:16:18]

http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.cc (right):

http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.cc:52: if (map[i].name == name)
On 2011/09/27 01:26:10, simonmorris wrote:
> Is this doing a pointer comparison instead of a string
> comparison? A unit test would make sure.
No, |name| is a std::string, so it compares string content. There are unittests
for this file and they would fail if this function wasn't working correctly.

[simonmorris, 2011-09-27 15:30:02]

On 2011/09/27 03:16:18, sergeyu wrote:
>
http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
> File remoting/protocol/jingle_messages.cc (right):
> 
>
http://codereview.chromium.org/8046018/diff/8002/remoting/protocol/jingle_mes...
> remoting/protocol/jingle_messages.cc:52: if (map[i].name == name)
> On 2011/09/27 01:26:10, simonmorris wrote:
> > Is this doing a pointer comparison instead of a string
> > comparison? A unit test would make sure.
> No, |name| is a std::string, so it compares string content. There are
unittests
> for this file and they would fail if this function wasn't working correctly.

You're right. You could add a check on JingleMessage.reason
to one of the unit tests, but it's not essential.

LGTM