Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2026123002:
[Chromoting] Use google:remoting namespace to export remoting specific error codes (Closed)
Description[Chromoting]Use google:remoting namespace to export remoting specific error code.
Jingle does not provide enough error codes for chromoting. So this change is to
add an error-code tag under 'google:remoting' namespace in JingleMessage, and
export chromoting specific error code under this node and namespace.
BUG=616156
Committed: https://crrev.com/d1ee82f8fe9fd81b8ecb6b668f0acf0fbaa262c2
Cr-Commit-Position: refs/heads/master@{#398387}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve review comments #
Total comments: 28
Patch Set 3 : Resolve review comments #
Total comments: 12
Patch Set 4 : Resolve review comments #
Total comments: 12
Patch Set 5 : Resolve review comments #
Messages
Total messages: 26 (10 generated)
Description was changed from ========== Add test cases for JingleMessage BUG= ========== to ========== Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ==========
zijiehe@chromium.org changed reviewers: + kelvinp@chromium.org, sergeyu@chromium.org
Patchset #1 (id:1) has been deleted
Please update CL description to follow the format suggested here: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:25: const char kRemotingNamespace[] = "google:remoting"; We already have kChromotingXmlNamespace in remoting/base/constants.h https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:61: const NameMapElement<JingleMessage::ErrorCode> kErrorCodes[] = { We already have ErrorCodeToString() in errors.cc. It will need to be rewritten to use NameToValue() and ValueToName() https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.h:74: enum struct ErrorCode { I don't think we want to add another enum to represent errors. Just use remoting::protocol::ErrorCode https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:497: switch (message.error_code) { I think we should use the <error-code> for any reason, not only for <decline>. <reason> matter only for compatibility with older versions that weren't setting <error-code>
Description was changed from ========== Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ========== to ========== Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ==========
https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:25: const char kRemotingNamespace[] = "google:remoting"; On 2016/06/01 08:52:47, Sergey Ulanov wrote: > We already have kChromotingXmlNamespace in remoting/base/constants.h Done. https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:61: const NameMapElement<JingleMessage::ErrorCode> kErrorCodes[] = { On 2016/06/01 08:52:47, Sergey Ulanov wrote: > We already have ErrorCodeToString() in errors.cc. It will need to be rewritten > to use NameToValue() and ValueToName() Done. https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.h (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_messages.h:74: enum struct ErrorCode { On 2016/06/01 08:52:47, Sergey Ulanov wrote: > I don't think we want to add another enum to represent errors. Just use > remoting::protocol::ErrorCode Done. https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/20001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:497: switch (message.error_code) { On 2016/06/01 08:52:47, Sergey Ulanov wrote: > I think we should use the <error-code> for any reason, not only for <decline>. > <reason> matter only for compatibility with older versions that weren't setting > <error-code> Done.
https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:12: #define MAP_STRING_LITERAL(x) \ We don't use define like this for NameMapElement definitions elsewhere, so I'd prefer to avoid it here as well. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:14: const NameMapElement<ErrorCode> kErrorCodes[] = { nit: kErrorCodeNames https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:30: #define RETURN_STRING_LITERAL(x) \ This define can be removed. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/errors.h File remoting/protocol/errors.h (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.h:35: extern const NameMapElement<ErrorCode> kErrorCodes[ERROR_CODE_MAX + 1]; Please don't expose this here. Instead add ParseErrorCode() function. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:351: error_code = UNKNOWN_ERROR; log a warning that the error code wasn't recognized. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: if (!error_code_string) { replace with DCHECK(error_code_string) https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:450: error_code_tag->AddElement(new XmlElement( I don't think we want to add the error as element. Insert it as text. I.e. it should be <error-code>PEER_IS_OFFLINE</error-code> instead of <error-code><PEER_IS_OFFLINE/></error-code> https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:576: EXPECT_EQ(message.ToXml()->Str(), You don't want to compare generate XML as string. Tests like this are unstable because they are easily broken with small changes in the XML formatter. Tests above use VerifyXml() to compare generated XML with expected value. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:592: message.error_code = parsed.error_code = UNKNOWN_ERROR; this double assignment is not readable. Please split it into two lines. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:243: ErrorCode error_code = UNKNOWN_ERROR; I think we want to set error_code for _all_ errors. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:278: message.error_code = error_code; message.error_code = error; and remove error_code variable. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:488: switch (message.error_code) { You don't need this switch. Just replace it with error_ = message.error_code; https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:501: switch (message.reason) { switch inside a switch is hard for humans to parse
lgtm pending sergeyu's comment. Once this change is landed. I will make a similar change in chromoting.com to parse the host errors.
Description was changed from ========== Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ========== to ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ==========
https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:12: #define MAP_STRING_LITERAL(x) \ On 2016/06/02 09:26:03, Sergey Ulanov wrote: > We don't use define like this for NameMapElement definitions elsewhere, so I'd > prefer to avoid it here as well. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:14: const NameMapElement<ErrorCode> kErrorCodes[] = { On 2016/06/02 09:26:03, Sergey Ulanov wrote: > nit: kErrorCodeNames Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.cc:30: #define RETURN_STRING_LITERAL(x) \ On 2016/06/02 09:26:03, Sergey Ulanov wrote: > This define can be removed. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/errors.h File remoting/protocol/errors.h (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/error... remoting/protocol/errors.h:35: extern const NameMapElement<ErrorCode> kErrorCodes[ERROR_CODE_MAX + 1]; On 2016/06/02 09:26:03, Sergey Ulanov wrote: > Please don't expose this here. Instead add ParseErrorCode() function. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:351: error_code = UNKNOWN_ERROR; On 2016/06/02 09:26:03, Sergey Ulanov wrote: > log a warning that the error code wasn't recognized. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: if (!error_code_string) { On 2016/06/02 09:26:04, Sergey Ulanov wrote: > replace with DCHECK(error_code_string) Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:450: error_code_tag->AddElement(new XmlElement( On 2016/06/02 09:26:03, Sergey Ulanov wrote: > I don't think we want to add the error as element. Insert it as text. > I.e. it should be <error-code>PEER_IS_OFFLINE</error-code> instead of > <error-code><PEER_IS_OFFLINE/></error-code> I used to implement as a text, but I found it's not a good idea. Do we treat <error-code> PEER_IS_OFFLINE</error-code> and <error-code> PEER_IS_OFFLINE </error-code> the same as <error-code>PEER_IS_OFFLINE</error-code>. I did not see libjingle implements whiteSpace=collapsed scenario, and there is also no whiteSpace=collapsed defined in our Xml output, which makes us need to do trim and collapse ourselves. And current solution is also consistent with other jingle errors. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:576: EXPECT_EQ(message.ToXml()->Str(), On 2016/06/02 09:26:04, Sergey Ulanov wrote: > You don't want to compare generate XML as string. Tests like this are unstable > because they are easily broken with small changes in the XML formatter. Tests > above use VerifyXml() to compare generated XML with expected value. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:592: message.error_code = parsed.error_code = UNKNOWN_ERROR; On 2016/06/02 09:26:04, Sergey Ulanov wrote: > this double assignment is not readable. Please split it into two lines. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:243: ErrorCode error_code = UNKNOWN_ERROR; On 2016/06/02 09:26:04, Sergey Ulanov wrote: > I think we want to set error_code for _all_ errors. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:278: message.error_code = error_code; On 2016/06/02 09:26:04, Sergey Ulanov wrote: > message.error_code = error; and remove error_code variable. Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:488: switch (message.error_code) { On 2016/06/02 09:26:04, Sergey Ulanov wrote: > You don't need this switch. > Just replace it with error_ = message.error_code; Done. https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:501: switch (message.reason) { On 2016/06/02 09:26:04, Sergey Ulanov wrote: > switch inside a switch is hard for humans to parse Done.
https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:450: error_code_tag->AddElement(new XmlElement( On 2016/06/02 22:00:18, Hzj_jie wrote: > On 2016/06/02 09:26:03, Sergey Ulanov wrote: > > I don't think we want to add the error as element. Insert it as text. > > I.e. it should be <error-code>PEER_IS_OFFLINE</error-code> instead of > > <error-code><PEER_IS_OFFLINE/></error-code> > > I used to implement as a text, but I found it's not a good idea. Do we treat > <error-code> PEER_IS_OFFLINE</error-code> and > <error-code> > PEER_IS_OFFLINE > </error-code> > the same as <error-code>PEER_IS_OFFLINE</error-code>. > I did not see libjingle implements whiteSpace=collapsed scenario, and there is > also no whiteSpace=collapsed defined in our Xml output, which makes us need to > do trim and collapse ourselves. > > And current solution is also consistent with other jingle errors. I don't see how that can be a problem. When we generate <error-code>FOO</error-code> we should get <error-code>FOO</error-code> on the other end. If whitespace gets inserted there somehow it's a bug. There are other places where we depend on whitespace not being inserted in random places. Beside that it's easy to trim the whitepsace if we necessary. And if we really wanted to be consistent with the Jingle's reason codes, then it would need to be <rem:foo-error> instead of <rem:error-code>FOO_ERROR</rem:error-code> https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.cc:12: #define MAP_STRING_LITERAL(x) \ remove this define https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/errors.h File remoting/protocol/errors.h (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.h:10: #include "remoting/protocol/name_value_map.h" Don't need this include https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.h:18: // Be sure to update these locations if you make any changes to the ordering. Please update this comment to mention that error.cc needs to be updated too.
https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:577: ParseFormatAndCompare(message.ToXml()->Str().c_str(), &parsed); This tests formats message, parses it and then formats again. I'm not sure what's the point. Can this test works the same way as all other tests: Parse static XML blob and then format it again? https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:587: EXPECT_EQ(UNKNOWN_ERROR, parsed.error_code); Add this condition in the SessionTerminate test above and then you wouldn't need to test this case here. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:607: TEST(JingleMessageTest, AuthenticationFailed) { Maybe instead of testing these two errors in two separate tests add a single tests that iterates over all possible error codes and verifies that each of them gets handled properly (ie. for each error code create JingleMessage, format it, parse it, check that you get the right result on the output).
https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/40001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:450: error_code_tag->AddElement(new XmlElement( On 2016/06/03 08:43:22, Sergey Ulanov wrote: > On 2016/06/02 22:00:18, Hzj_jie wrote: > > On 2016/06/02 09:26:03, Sergey Ulanov wrote: > > > I don't think we want to add the error as element. Insert it as text. > > > I.e. it should be <error-code>PEER_IS_OFFLINE</error-code> instead of > > > <error-code><PEER_IS_OFFLINE/></error-code> > > > > I used to implement as a text, but I found it's not a good idea. Do we treat > > <error-code> PEER_IS_OFFLINE</error-code> and > > <error-code> > > PEER_IS_OFFLINE > > </error-code> > > the same as <error-code>PEER_IS_OFFLINE</error-code>. > > I did not see libjingle implements whiteSpace=collapsed scenario, and there is > > also no whiteSpace=collapsed defined in our Xml output, which makes us need to > > do trim and collapse ourselves. > > > > And current solution is also consistent with other jingle errors. > > I don't see how that can be a problem. When we generate > <error-code>FOO</error-code> we should get <error-code>FOO</error-code> on the > other end. If whitespace gets inserted there somehow it's a bug. There are other > places where we depend on whitespace not being inserted in random places. Beside > that it's easy to trim the whitepsace if we necessary. And if we really wanted > to be consistent with the Jingle's reason codes, then it would need to be > <rem:foo-error> instead of <rem:error-code>FOO_ERROR</rem:error-code> Done. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.cc:12: #define MAP_STRING_LITERAL(x) \ On 2016/06/03 08:43:22, Sergey Ulanov wrote: > remove this define Done. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/errors.h File remoting/protocol/errors.h (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.h:10: #include "remoting/protocol/name_value_map.h" On 2016/06/03 08:43:22, Sergey Ulanov wrote: > Don't need this include Done. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/error... remoting/protocol/errors.h:18: // Be sure to update these locations if you make any changes to the ordering. On 2016/06/03 08:43:22, Sergey Ulanov wrote: > Please update this comment to mention that error.cc needs to be updated too. Done. And other locations are not accurate, I have updated them together. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... File remoting/protocol/jingle_messages_unittest.cc (right): https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:577: ParseFormatAndCompare(message.ToXml()->Str().c_str(), &parsed); On 2016/06/03 08:53:49, Sergey Ulanov wrote: > This tests formats message, parses it and then formats again. I'm not sure > what's the point. Can this test works the same way as all other tests: Parse > static XML blob and then format it again? This test was for debugging purpose, I should remove it. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:587: EXPECT_EQ(UNKNOWN_ERROR, parsed.error_code); On 2016/06/03 08:53:49, Sergey Ulanov wrote: > Add this condition in the SessionTerminate test above and then you wouldn't need > to test this case here. Acknowledged. https://codereview.chromium.org/2026123002/diff/60001/remoting/protocol/jingl... remoting/protocol/jingle_messages_unittest.cc:607: TEST(JingleMessageTest, AuthenticationFailed) { On 2016/06/03 08:53:49, Sergey Ulanov wrote: > Maybe instead of testing these two errors in two separate tests add a single > tests that iterates over all possible error codes and verifies that each of them > gets handled properly (ie. for each error code create JingleMessage, format it, > parse it, check that you get the right result on the output). Done.
LGTM with some style nits. Thanks for getting this done! https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/error... remoting/protocol/errors.cc:31: } // namespace https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: DCHECK(error_code_string); nit: don't need this. There is already NOTREACHED() in ValueToName() which would fail for unknown error_code values. And then you can remove error_code_string variable, and move ErrorCodeToString() inline. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:253: reason = JingleMessage::DECLINE; This is the same as the case above, so you can just add INVALID_ACCOUNT after AUTHENTICATION_FAILED, so the same code handles both cases. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:486: // For legacy hosts, which do not provide message.error_code. This code is not specific to client, so needs to be reworded. E.g.: // Get error code from message.reason for compatibility with // older versions that do not add <error-code>. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:519: // TODO(zijiehe): Handle SESSION_REJECTED error in WebApp. Please file a bug for this.
https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/error... File remoting/protocol/errors.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/error... remoting/protocol/errors.cc:31: } On 2016/06/07 16:22:23, Sergey Ulanov wrote: > // namespace Done. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: DCHECK(error_code_string); On 2016/06/07 16:22:23, Sergey Ulanov wrote: > nit: don't need this. There is already NOTREACHED() in ValueToName() which would > fail for unknown error_code values. And then you can remove error_code_string > variable, and move ErrorCodeToString() inline. I copied the similar logic from reason node above. I would prefer to change them both. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_session.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:253: reason = JingleMessage::DECLINE; On 2016/06/07 16:22:23, Sergey Ulanov wrote: > This is the same as the case above, so you can just add INVALID_ACCOUNT after > AUTHENTICATION_FAILED, so the same code handles both cases. Done. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:486: // For legacy hosts, which do not provide message.error_code. On 2016/06/07 16:22:23, Sergey Ulanov wrote: > This code is not specific to client, so needs to be reworded. E.g.: > // Get error code from message.reason for compatibility with > // older versions that do not add <error-code>. Done. https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_session.cc:519: // TODO(zijiehe): Handle SESSION_REJECTED error in WebApp. On 2016/06/07 16:22:23, Sergey Ulanov wrote: > Please file a bug for this. Done.
https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: DCHECK(error_code_string); On 2016/06/07 19:29:57, Hzj_jie wrote: > On 2016/06/07 16:22:23, Sergey Ulanov wrote: > > nit: don't need this. There is already NOTREACHED() in ValueToName() which > would > > fail for unknown error_code values. And then you can remove error_code_string > > variable, and move ErrorCodeToString() inline. > > I copied the similar logic from reason node above. I would prefer to change them > both. I think that check for reason_string was added before ValueToName() had that NOTREACHED() statement. See https://codereview.chromium.org/8046018/diff/9013/remoting/protocol/jingle_me... - that's where it was added. But it doesn't make sense there anymore.
https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... File remoting/protocol/jingle_messages.cc (right): https://codereview.chromium.org/2026123002/diff/80001/remoting/protocol/jingl... remoting/protocol/jingle_messages.cc:447: DCHECK(error_code_string); On 2016/06/07 20:17:21, Sergey Ulanov wrote: > On 2016/06/07 19:29:57, Hzj_jie wrote: > > On 2016/06/07 16:22:23, Sergey Ulanov wrote: > > > nit: don't need this. There is already NOTREACHED() in ValueToName() which > > would > > > fail for unknown error_code values. And then you can remove > error_code_string > > > variable, and move ErrorCodeToString() inline. > > > > I copied the similar logic from reason node above. I would prefer to change > them > > both. > > I think that check for reason_string was added before ValueToName() had that > NOTREACHED() statement. See > https://codereview.chromium.org/8046018/diff/9013/remoting/protocol/jingle_me... > - that's where it was added. But it doesn't make sense there anymore. Yes, both have been removed in the latest iteration.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org, sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2026123002/#ps100001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2026123002/100001
Message was sent while issue was closed.
Description was changed from ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ========== to ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 ========== to ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 Committed: https://crrev.com/d1ee82f8fe9fd81b8ecb6b668f0acf0fbaa262c2 Cr-Commit-Position: refs/heads/master@{#398387} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d1ee82f8fe9fd81b8ecb6b668f0acf0fbaa262c2 Cr-Commit-Position: refs/heads/master@{#398387}
Message was sent while issue was closed.
Description was changed from ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 Committed: https://crrev.com/d1ee82f8fe9fd81b8ecb6b668f0acf0fbaa262c2 Cr-Commit-Position: refs/heads/master@{#398387} ========== to ========== [Chromoting]Use google:remoting namespace to export remoting specific error code. Jingle does not provide enough error codes for chromoting. So this change is to add an error-code tag under 'google:remoting' namespace in JingleMessage, and export chromoting specific error code under this node and namespace. BUG=616156 Committed: https://crrev.com/d1ee82f8fe9fd81b8ecb6b668f0acf0fbaa262c2 Cr-Commit-Position: refs/heads/master@{#398387} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
