Send correct termination reason when session is terminated by the host.

Send correct termination reason when session is terminated by the host.

BUG=None
TEST=None

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

[Wez, 2011-11-11 23:10:09]

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:42: GENERAL_ERROR,
How does a "general error" differ from an "unknown reason"?

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:43: INCOMPATIBLE_PARAMETERS,
These are all macro-style and don't have very consistent names. :(

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.cc:112: case CHANNEL_CONNECTION_ERROR:
Would it make more sense to have a default: case for setting
STR_TERMINATE_ERROR, or are you specifically wanting to make sure the compiler
warns if we add new Error codes and don't handle them here?

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_session.h
File remoting/protocol/jingle_session.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.h:84: // Close all the channels and terminate
the session.
Explain what |result| and |error| are used for?

http://codereview.chromium.org/8537020/diff/3001/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/3001/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.h:42: GENERAL_ERROR,
Where does this actually get used?

[Sergey Ulanov, 2011-11-12 00:02:52]

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:42: GENERAL_ERROR,
On 2011/11/11 23:10:09, Wez wrote:
> How does a "general error" differ from an "unknown reason"?
It's basically the same. Jingle defines it as "The action is related to a
non-specific application error."

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:43: INCOMPATIBLE_PARAMETERS,
On 2011/11/11 23:10:09, Wez wrote:
> These are all macro-style 
That's required by style guide.

>and don't have very consistent names. :(
they correspond to names defined by jingle spec.

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
File remoting/protocol/jingle_session.cc (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.cc:112: case CHANNEL_CONNECTION_ERROR:
On 2011/11/11 23:10:09, Wez wrote:
> Would it make more sense to have a default: case for setting
> STR_TERMINATE_ERROR, or are you specifically wanting to make sure the compiler
> warns if we add new Error codes and don't handle them here?

Done.

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_session.h
File remoting/protocol/jingle_session.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_sessio...
remoting/protocol/jingle_session.h:84: // Close all the channels and terminate
the session.
On 2011/11/11 23:10:09, Wez wrote:
> Explain what |result| and |error| are used for?

Done.

http://codereview.chromium.org/8537020/diff/3001/remoting/protocol/jingle_mes...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/3001/remoting/protocol/jingle_mes...
remoting/protocol/jingle_messages.h:42: GENERAL_ERROR,
On 2011/11/11 23:10:09, Wez wrote:
> Where does this actually get used?
It is not used right now, but it can be used in the future. E.g. in the previous
CL http://codereview.chromium.org/8536032/ we should set error to
CHANNEL_CONNECTION_ERROR only when the reason in the terminate message is set to
GENERAL_ERROR, otherwise it's just regular terminate. Older hosts don't set the
error properly so we have to interpret any error as a CHANNEL_CONNECTION_ERROR.

[Wez, 2011-11-15 02:30:05]

lgtm

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:43: INCOMPATIBLE_PARAMETERS,
On 2011/11/12 00:02:52, sergeyu wrote:
> On 2011/11/11 23:10:09, Wez wrote:
> > These are all macro-style 
> That's required by style guide.

Style guide allows both forms and prefers non-macro-style unless it's for
consistency with existing code, though.

> >and don't have very consistent names. :(
> they correspond to names defined by jingle spec.

[Sergey Ulanov, 2011-11-15 02:37:21]

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
File remoting/protocol/jingle_messages.h (right):

http://codereview.chromium.org/8537020/diff/1/remoting/protocol/jingle_messag...
remoting/protocol/jingle_messages.h:43: INCOMPATIBLE_PARAMETERS,
On 2011/11/15 02:30:05, Wez wrote:
> On 2011/11/12 00:02:52, sergeyu wrote:
> > On 2011/11/11 23:10:09, Wez wrote:
> > > These are all macro-style 
> > That's required by style guide.
> 
> Style guide allows both forms and prefers non-macro-style unless it's for
> consistency with existing code, though.

Chromium style guide (http://www.chromium.org/developers/coding-style) says:
  Though the style guide says to use kConstantNaming for enums now, we still use
MACRO_STYLE naming for enums for consistency with existing code.

It is worded ambiguously, but it think the right way to interpret is to always
use MACRO_STYLE for enums.

> 
> > >and don't have very consistent names. :(
> > they correspond to names defined by jingle spec.
>

[commit-bot: I haz the power, 2011-11-15 02:37:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8537020/3006

[commit-bot: I haz the power, 2011-11-15 03:45:47]

Change committed as 110036