Add base/android/activity_status.cc

base/android/activity_status_unittest.cc

Index: base/android/activity_status_unittest.cc
diff --git a/base/android/activity_status_unittest.cc b/base/android/activity_status_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c3c40422a11fee8dde540fb119313a599436f355
--- /dev/null
+++ b/base/android/activity_status_unittest.cc
@@ -0,0 +1,101 @@
+// 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 "base/android/activity_status.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/run_loop.h"
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {

[gavinp, 2013/04/26 07:37:53]

Blank line here.

[digit1, 2013/04/26 12:32:40]

Are you sure about that? I'm asking because _all_ the other Android-specific
code I've seen tacks the lines together.

[gavinp, 2013/04/26 12:38:48]

I just checked. It's not required. It is allowed.

I think you should leave this consistant with the other android code. Thanks for
educating me.

+namespace android {
+
+// This is equivalent to what the JNI generator would output if
+// ActivityStatus.onStateChange() was tagged with @CalledByNative.
+// because this is only useful for unit-testing, there is no point in
+// making this permanent for production code.
+using base::android::ScopedJavaLocalRef;
+
+namespace {

[gavinp, 2013/04/26 07:37:53]

Blank line here.

[digit1, 2013/04/26 12:32:40]

Done.

+const char kActivityStatusClassPath[] = "org/chromium/base/ActivityStatus";
+jclass g_ActivityStatus_clazz = NULL;
+
+base::subtle::AtomicWord g_ActivityStatus_onStateChange = 0;
+
+void Java_ActivityStatus_onStateChange(JNIEnv* env, jobject activity,
+    jint newState) {
+  // Ensure g_ActivityStatus_clazz is initialized
+  if (g_ActivityStatus_clazz == NULL) {
+    g_ActivityStatus_clazz = reinterpret_cast<jclass>(env->NewGlobalRef(
+        base::android::GetClass(env, kActivityStatusClassPath).obj()));
+    DCHECK(g_ActivityStatus_clazz);
+  }
+  jmethodID method_id =
+      base::android::MethodID::LazyGet<
+      base::android::MethodID::TYPE_STATIC>(
+      env, g_ActivityStatus_clazz,
+      "onStateChange",
+"("
+"Landroid/app/Activity;"
+"I"
+")"
+"V",
+      &g_ActivityStatus_onStateChange);
+
+  env->CallStaticVoidMethod(g_ActivityStatus_clazz,
+      method_id, activity, newState);
+  base::android::CheckException(env);
+}

[gavinp, 2013/04/26 07:37:53]

Blank line above this, and comment: // namespace

[digit1, 2013/04/26 12:32:40]

I think you're mistaken, this is the end of a function (it was copy-pasted from
auto-generated output, but I've reformatted it in a later patch).

The line you're looking for is lower :-)

+
+void ForceActivityStateTo(ActivityState state) {
+  JNIEnv* env = base::android::AttachCurrentThread();

[gavinp, 2013/04/26 07:37:53]

#include <jni.h> ?

[digit1, 2013/04/26 12:32:40]

Added #include "base/android/jni_android.h"

+  DCHECK(env != NULL);

[gavinp, 2013/04/26 07:37:53]

Don't DCHECK in unit tests. Either ASSERT or EXPECT. This looks like an ASSERT.
It's best to ASSERT from the top level (due to gtest annoyingness), so perhaps
this function could return a bool, which could be ASSERT_TRUE in the caller?

[digit1, 2013/04/26 12:32:40]

Replaced with an ASSERT_TRUE(env). Thanks.

[gavinp, 2013/04/26 12:38:48]

Since the ASSERT is inside a function, I think it won't work, so the test will
still crash. Not ideal. :(

[digit1, 2013/04/26 13:25:55]

Yes, I believe that's why I wanted to DCHECK() in the first place. I've replaced
this with EXPECT_() calls and returning success/false flags from some functions.

+  RunLoop run_loop;
+  Java_ActivityStatus_onStateChange(env, NULL, static_cast<int>(state));
+  run_loop.RunUntilIdle();
+}
+
+// Helper class that writes the new state at a dedicated location.
+class StoringStateListener : public ActivityStatus::StateListener {
+ public:
+  // When creating an instance, pass the address of an ActivityState
+  // variable in |result_ptr|
+  StoringStateListener(ActivityState* result_ptr)
+    : result_ptr_(result_ptr) {}
+
+  void OnActivityStateChange(ActivityState new_state) OVERRIDE {
+    // Simply write the result to the correct address.
+    *result_ptr_ = new_state;
+  }
+ private:
+  ActivityState* result_ptr_;
+};
+
+typedef scoped_ptr<StoringStateListener> ScopedStoringStateListener;

[gavinp, 2013/04/26 07:37:53]

This is never used. I prefer the naked scoped_ptr definition, anyway, so I
suggest dropping this.

[digit1, 2013/04/26 12:32:40]

Done.

+
+// An invalid ActivityState value.
+const ActivityState kInvalidActivityState = static_cast<ActivityState>(100);
+
+}  // namespace
+
+TEST(ActivityStatusTest, SingleThread) {
+  MessageLoop message_loop;
+
+  ActivityState result = kInvalidActivityState;
+
+  scoped_ptr<StoringStateListener> listener(new StoringStateListener(&result));
+  ASSERT_TRUE(listener.get() != NULL);

[gavinp, 2013/04/26 07:37:53]

ASSERT_TRUE(listener);

Looks cleaner to me, but that may just be taste.

[digit1, 2013/04/26 12:32:40]

Done.

+
+  EXPECT_EQ(kInvalidActivityState, result);
+
+  ForceActivityStateTo(ACTIVITY_STATE_CREATED);
+  EXPECT_EQ(ACTIVITY_STATE_CREATED, result);
+
+  ForceActivityStateTo(ACTIVITY_STATE_DESTROYED);
+  EXPECT_EQ(ACTIVITY_STATE_DESTROYED, result);
+}
+
+}  // namespace android

[gavinp, 2013/04/26 07:37:53]

Blank line after this.

+}  // namespace base