Chromoting Android app mouse/keyboard bugfixes and enhancements

remoting/client/jni/chromoting_jni_instance.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/client/jni/chromoting_jni_instance.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "remoting/client/audio_player.h"
 #include "remoting/client/jni/android_keymap.h"
 #include "remoting/client/jni/chromoting_jni_runtime.h"
 #include "remoting/protocol/libjingle_transport_factory.h"
 
 // TODO(solb) Move into location shared with client plugin.
 const char* const CHAT_SERVER = "talk.google.com";

[Sergey Ulanov, 2013/07/30 21:55:18]

This should be kChatServer (though it might be better to refer to it as XMPP
server). Same for the constants below.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Consta...

It's more common to use char arrays for const strings, e.g.:
const char kChatServer[] = "talk.google.com";

Also please put these constants in an anonymous namespace:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

[solb, 2013/07/30 23:03:38]

Wez and I have discussed this very same issue (see the first comment on
https://codereview.chromium.org/19297003/diff2/6001:11001/remoting/client/jni...),
but decided these shouldn't be namespaced because C++ treats const as static
const in this context. I am renaming them though.

 const int CHAT_PORT = 5222;
 const bool CHAT_USE_TLS = true;
 
 namespace remoting {
 
 ChromotingJniInstance::ChromotingJniInstance(ChromotingJniRuntime* jni_runtime,
                                              const char* username,
                                              const char* auth_token,
                                              const char* host_jid,
                                              const char* host_id,
@@ skipping 73 matching lines @@
   protocol::MouseEvent action;
   action.set_x(x);
   action.set_y(y);
   action.set_button(button);
   if (button != protocol::MouseEvent::BUTTON_UNDEFINED)
     action.set_button_down(buttonDown);
 
   connection_->input_stub()->InjectMouseEvent(action);
 }
 
 void ChromotingJniInstance::PerformKeyboardAction(int keyCode, bool keyDown) {

[Sergey Ulanov, 2013/07/30 21:55:18]

Here and in other functions: we don't use camel casing for variables in C++. So
these should be key_code, key_down. Maybe worth having a separate CL to fix it.

[solb, 2013/07/30 23:03:38]

I just fixed it in this CL since it's already backed up behind another one.

   if (!jni_runtime_->network_task_runner()->BelongsToCurrentThread()) {
     jni_runtime_->network_task_runner()->PostTask(
         FROM_HERE,
         base::Bind(&ChromotingJniInstance::PerformKeyboardAction,
                    this,
                    keyCode,
                    keyDown));
     return;
   }
 
   uint32 usbCode = AndroidKeycodeToUsbKeycode(keyCode);
   if (usbCode) {
     protocol::KeyEvent action;
     action.set_usb_keycode(usbCode);
     action.set_pressed(keyDown);
     connection_->input_stub()->InjectKeyEvent(action);
   }
-  else
+  else {
     LOG(WARNING) << "Ignoring unknown keycode: " << keyCode;
+  }
 }
 
 void ChromotingJniInstance::OnConnectionState(
     protocol::ConnectionToHost::State state,
     protocol::ErrorCode error) {
   if (!jni_runtime_->ui_task_runner()->BelongsToCurrentThread()) {
     jni_runtime_->ui_task_runner()->PostTask(
             FROM_HERE,
             base::Bind(&ChromotingJniInstance::OnConnectionState,
                        this,
@@ skipping 133 matching lines @@
                    pairable,
                    callback));
     return;
   }
 
   pin_callback_ = callback;
   jni_runtime_->DisplayAuthenticationPrompt();
 }
 
 }  // namespace remoting