Send enums as strings between Chromoting Native Messaging host and web-app.

Send enums as strings between Chromoting Native Messaging host and web-app.

This updates the JSON interface to transmit State and
AsyncResult values as strings instead of numbers.

BUG=232135
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203894

[Sergey Ulanov, 2013-05-30 23:14:52]

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller.h (right):

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller.h:51: static const char
kStateNotImplemented[];
These consts do not need to be in this class - it's detail of native messaging
interface, not part of DesktopController.

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/native_me...
File remoting/host/setup/native_messaging_host.cc (right):

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/native_me...
remoting/host/setup/native_messaging_host.cc:246: case
DaemonController::STATE_NOT_IMPLEMENTED:
Take a look at "remoting/protocol/name_value_map.h". It would be useful here.
Maybe move it to base and use here.

In either case all consts from DaemonController used only here, so just remove
them and put the values here.

https://codereview.chromium.org/16236008/diff/1/remoting/webapp/host_native_m...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/16236008/diff/1/remoting/webapp/host_native_m...
remoting/webapp/host_native_messaging.js:135: for (var i in
remoting.HostController.State) {
You can replace this loop with:
 if (result in remoting.ClientSession.ConnectionError) {
   return remoting.HostController.State[result];
 } else {
   // Invalid result.
 }


Same for AsyncResult.

[Lambros, 2013-05-31 01:32:56]

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/daemon_co...
File remoting/host/setup/daemon_controller.h (right):

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/daemon_co...
remoting/host/setup/daemon_controller.h:51: static const char
kStateNotImplemented[];
On 2013/05/30 23:14:52, Sergey Ulanov wrote:
> These consts do not need to be in this class - it's detail of native messaging
> interface, not part of DesktopController.

Done.

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/native_me...
File remoting/host/setup/native_messaging_host.cc (right):

https://codereview.chromium.org/16236008/diff/1/remoting/host/setup/native_me...
remoting/host/setup/native_messaging_host.cc:246: case
DaemonController::STATE_NOT_IMPLEMENTED:
On 2013/05/30 23:14:52, Sergey Ulanov wrote:
> Take a look at "remoting/protocol/name_value_map.h". It would be useful here.
> Maybe move it to base and use here.
Not worth moving, I'm only using value->name direction, and switch/case gives
compile-time checking.

> 
> In either case all consts from DaemonController used only here, so just remove
> them and put the values here.
Done.

https://codereview.chromium.org/16236008/diff/1/remoting/webapp/host_native_m...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/16236008/diff/1/remoting/webapp/host_native_m...
remoting/webapp/host_native_messaging.js:135: for (var i in
remoting.HostController.State) {
On 2013/05/30 23:14:52, Sergey Ulanov wrote:
> You can replace this loop with:
>  if (result in remoting.ClientSession.ConnectionError) {
>    return remoting.HostController.State[result];
>  } else {
>    // Invalid result.
>  }
> 
> 
> Same for AsyncResult.
Nice! Done.

[Sergey Ulanov, 2013-05-31 23:01:43]

LGTM when my comments are addressed.

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host.cc (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host.cc:39: const char
NativeMessagingHost::kStateNotImplemented[] = "NOT_IMPLEMENTED";
As I mentioned before you don't really need to define these consts - each of
them is used only in one place, so you are not gaining anything. Just put the
string values in ProcessGetDaemonState().

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:89:
EXPECT_EQ(remoting::NativeMessagingHost::kResultOk, value);
I'd rather not bother about sharing constants with the unittests. Just use "OK"
here. You are already duplicating many other strings anyway.

https://codereview.chromium.org/16236008/diff/7001/remoting/webapp/host_nativ...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/webapp/host_nativ...
remoting/webapp/host_native_messaging.js:116: return
remoting.HostController.AsyncResult[result];
nit: I would swap this block and the error case at the end. It's better when
function ends with the success case. Same for asHostState_()

[Lambros, 2013-06-03 21:28:44]

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host.cc (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host.cc:39: const char
NativeMessagingHost::kStateNotImplemented[] = "NOT_IMPLEMENTED";
On 2013/05/31 23:01:43, Sergey Ulanov wrote:
> As I mentioned before you don't really need to define these consts - each of
> them is used only in one place, so you are not gaining anything. Just put the
> string values in ProcessGetDaemonState().

Done.

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
File remoting/host/setup/native_messaging_host_unittest.cc (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/host/setup/native...
remoting/host/setup/native_messaging_host_unittest.cc:89:
EXPECT_EQ(remoting::NativeMessagingHost::kResultOk, value);
On 2013/05/31 23:01:43, Sergey Ulanov wrote:
> I'd rather not bother about sharing constants with the unittests. Just use
"OK"
> here. You are already duplicating many other strings anyway.

Done.

https://codereview.chromium.org/16236008/diff/7001/remoting/webapp/host_nativ...
File remoting/webapp/host_native_messaging.js (right):

https://codereview.chromium.org/16236008/diff/7001/remoting/webapp/host_nativ...
remoting/webapp/host_native_messaging.js:116: return
remoting.HostController.AsyncResult[result];
On 2013/05/31 23:01:43, Sergey Ulanov wrote:
> nit: I would swap this block and the error case at the end. It's better when
> function ends with the success case. Same for asHostState_()

Done.

[commit-bot: I haz the power, 2013-06-03 21:31:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/16236008/1...

[commit-bot: I haz the power, 2013-06-04 07:24:40]

Change committed as 203894