Implement SpeechRecognizerImplAndroid

content/browser/speech/speech_recognizer_impl_android.h

+// Copyright (c) 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.
+
+#ifndef CONTENT_BROWSER_SPEECH_SPEECH_RECOGNIZER_IMPL_ANDROID_H_
+#define CONTENT_BROWSER_SPEECH_SPEECH_RECOGNIZER_IMPL_ANDROID_H_
+
+#include <jni.h>
+
+#include "base/android/scoped_java_ref.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Nit: Is this newline needed here? I can't find anything in either code style.

[janx, 2013/06/05 13:51:00]

I think new lines are needed when include order matters and you have to break
alphabetical order. It is not needed here, so I'll remove it.

+#include "content/browser/speech/speech_recognizer.h"
+#include "content/public/browser/speech_recognition_session_config.h"
+#include "content/public/common/speech_recognition_error.h"
+#include "content/public/common/speech_recognition_result.h"
+
+namespace content {
+
+class SpeechRecognitionEventListener;
+
+class CONTENT_EXPORT SpeechRecognizerImplAndroid : public SpeechRecognizer {
+ public:
+  static void Init(JNIEnv* env);
+
+  virtual ~SpeechRecognizerImplAndroid();

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Since SpeechRecognizer (the base class) is refcounted, I guess it would be safer
for the dtor to be protected, in order to catch unwanted/unexpected explicit
frees.

[janx, 2013/06/05 13:51:00]

I'll make it private, thanks for spotting this!

+
+  SpeechRecognizerImplAndroid(SpeechRecognitionEventListener* listener,
+                              const SpeechRecognitionSessionConfig& config,
+                              int session_id);
+
+  void StartRecognition();

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Shouldn't this be virtual void StartRecognition() OVERRIDE?
(same for the 4 lines below)

[janx, 2013/06/05 13:51:00]

Indeed, that's a leftover from before SpeechRecognizer could be derived.

+  void AbortRecognition();
+  void StopAudioCapture();
+  bool IsActive() const;
+  bool IsCapturingAudio() const;
+
+  void StartRecognitionJNI(bool continuous, bool partial_results);

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Shouldn't these three methods be private?
Also, can you add a comment like this?
// Wrappers for JNI methods.

[janx, 2013/06/05 13:51:00]

Will do.

+  void CancelRecognitionJNI();
+  void StopRecognitionJNI();
+

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Add a comment: // Called from Java via JNI.

[janx, 2013/06/05 13:51:00]

Done.

+  void OnAudioStartJNI(JNIEnv* env, jobject obj);
+  void OnSoundStartJNI(JNIEnv* env, jobject obj);
+  void OnSoundEndJNI(JNIEnv* env, jobject obj);
+  void OnRecognitionResultsJNI(JNIEnv* env, jobject obj, jobjectArray strings,
+                               jfloatArray floats, jboolean partial);
+  void OnRecognitionErrorJNI(JNIEnv* env, jobject obj, jint error);
+  void OnAudioLevelsChangeJNI(JNIEnv* env, jobject obj, jfloat rms);

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Looks like here you need an OnRecognitionEndJNI event coming from Java.
This event should be "solemn" promise that no other callback will be called from
the JNI object.
Concretely, it will be the method in which you should call the
listener_->OnRecognitionEnd() (C++) method, which has the meaning "This is the
last event, feel free to destroy the object".
(See the corresponding comment in:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...)

[janx, 2013/06/05 13:51:00]

I'll implement an OnRecognitionEndJNI(e,o) method with a wrapper calling
listener_->OnRecognitionEnd(), and make sure that no events are thrown
afterwards.

+
+  void OnAudioStart();

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Similar to previous comments: I guess these 6 methods, which are wrapped by the
equivalent ...JNI ones, should be private.

[janx, 2013/06/05 13:51:00]

Moving to private.

+  void OnSoundStart();
+  void OnSoundEnd();
+  void OnRecognitionResults(std::vector<string16> options,
+                            std::vector<float> scores, bool partial);
+  void OnRecognitionError(SpeechRecognitionError error);
+  void OnAudioLevelsChange(float rms);
+
+ private:
+  base::android::ScopedJavaGlobalRef<jobject> j_recognition_;
+

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

Nit: remove blank line.

+  SpeechRecognitionSessionConfig config_;

[Primiano Tucci (use gerrit), 2013/06/03 16:24:11]

See my previous comment: since it seems you're using config_ only in
StartRecognition, it would be cleaner just using calling GetSessionConfig (in
StartRecognition) and avoiding this field at all.

[janx, 2013/06/05 13:51:00]

Great suggestion, I'll use
SpeechRecognitionManagerImpl::GetInstance()->GetSessionConfig() instead of the
config_ attribute (thanks for the tip).

+
+  DISALLOW_COPY_AND_ASSIGN(SpeechRecognizerImplAndroid);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_SPEECH_SPEECH_RECOGNIZER_IMPL_ANDROID_H_