Android: Remove Surface cruft

content/common/android/surface.cc

Index: content/common/android/surface.cc
diff --git a/content/common/android/surface.cc b/content/common/android/surface.cc
new file mode 100644
index 0000000000000000000000000000000000000000..916706a5de0edf0b52eabefe420d14024f694c0c
--- /dev/null
+++ b/content/common/android/surface.cc
@@ -0,0 +1,42 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.

[bulach, 2013/03/04 10:44:51]

nit: 2013 :)

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/common/android/surface.h"
+
+#include "jni/Surface_jni.h"
+
+namespace {
+
+bool g_jni_initialized = false;
+
+void RegisterNativesIfNeeded(JNIEnv* env) {
+  if (!g_jni_initialized) {
+    JNI_Surface::RegisterNativesImpl(env);
+    g_jni_initialized = true;
+  }
+}
+
+}  // anonymous namespace
+
+namespace content {
+
+Surface::Surface(const base::android::JavaRef<jobject>& surface_texture) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  RegisterNativesIfNeeded(env);
+
+  ScopedJavaLocalRef<jobject> tmp(
+      JNI_Surface::Java_Surface_Constructor(env, surface_texture.obj()));
+  DCHECK(!tmp.is_null());
+  j_surface_.Reset(tmp);
+}
+
+Surface::~Surface() {
+}
+
+void Surface::Release() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  JNI_Surface::Java_Surface_release(env, j_surface_.obj());

[bulach, 2013/03/04 10:44:51]

I see the naming clash here on Release vs Reset... :-/
since it's not really exposing anything from Surface but it rather looks like a
ScopedJavaRef with an extra call on destruction, I think this method should be
Reset, and called on the dtor "if (j_surface_.obj())"

and also, I think j_surface_.Reset() should be called here as well, right?

[no sievers, 2013/03/04 18:09:52]

Actually, this method (in the java class) is named a bit ambiguously. What it
does is release the graphics memory referenced from the native side of the class
instance (to avoid having to rely on GC for this to happen). I think it might be
better to leave it as an explicit method to deal with this (one has to be a bit
careful when to do this), but I'll add a comment to the method.

I have another patch I'm working on where I need to call Surface.release(), but
there I create the Surface object in Java, so I'd either need to expose a static
Release(jobject) method after all, or have a constructor (from a jobject - I
guess I could expose three static methods for construction: Create(),
CreateFromSurface(jobject), CreateFromSurfaceTexture(jobject)). What do you
think?

[bulach, 2013/03/04 18:48:52]

I agree it's going to be hard to fix the API naming mismatch :)
let's try to hide that then!!

I'm not sure about "CreateFromSurface" though, since it won't really create a
surface, it'd just wrap an existing one, right?

how about this:
- name this "ScopedJavaSurface", so that it's clear it will NOT expose the whole
java Surface API ever.
- make the constructor take no arguments.
- make the dtor call a private release, or even better, just call these two
lines here in the dtor..
- have two methods: "ResetFromSurface(jobject)" and
"ResetFromSurfaceTexture(jobject)".. document that the first will just ensure
that Surface::Release will be called upon going out of scope, and the latter
will create a a new Surface for the given SurfaceTexture, and ensure it'll be
Release once it goes out of scope..

overall goals being:
- it hides badly named APIs ;)
- it mimics the existing |ScopedJava*| API and semantics with Reset and
releasing refs on dtor
- by being "ScopedJavaSurface" rather than just "Surface" we ensure this will
not grow out of proportions, and it also clarifies its only purpose is to manage
a Surface.java lifetime..

wdyt?

+}
+
+}  // namespace content