Add ScopedSurfaceRequestManager

content/browser/android/child_process_launcher_android.cc

Index: content/browser/android/child_process_launcher_android.cc
diff --git a/content/browser/android/child_process_launcher_android.cc b/content/browser/android/child_process_launcher_android.cc
index d3e8ee589ef18186b95e8061c41e73e07cb11a59..c736bcce0ce7533026835cd178cd76ca4c00dd32 100644
--- a/content/browser/android/child_process_launcher_android.cc
+++ b/content/browser/android/child_process_launcher_android.cc
@@ -14,6 +14,7 @@
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/logging.h"
+#include "content/browser/android/scoped_surface_request_manager.h"
 #include "content/browser/file_descriptor_info_impl.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/media/android/browser_media_player_manager.h"
@@ -90,6 +91,19 @@ static void SetSurfacePeer(
   }
 }
 
+static void GetAndCompleteScopedSurfaceRequest(
+    const base::android::JavaRef<jobject>& surface,

[liberato (no reviews please), 2016/08/30 17:58:35]

perhaps switch |surface| and |request_id|.  they're ordered the other way
everywhere else.

[tguilbert, 2016/08/30 22:53:09]

Deleted this method.

+    uint64_t request_id) {
+  ScopedSurfaceRequestManager::ScopedSurfaceRequestCB surface_request =
+      ScopedSurfaceRequestManager::GetInstance()
+          ->GetAndUnregisterScopedSurfaceRequest(request_id);
+
+  if (!surface_request.is_null()) {
+    gl::ScopedJavaSurface scoped_surface(surface);
+    surface_request.Run(std::move(scoped_surface));
+  }

[watk, 2016/08/29 19:42:25]

Two things:
1) why not call FulfillRequest?
2) doesn't this have the race condition you were worried about in fulfill?

[tguilbert, 2016/08/30 22:53:09]

1) Updated to call Fulfill.
2) I have added DCHECKS to make sure we are on the browser UI thread. I think
this solves the race conditions I mentioned.

+}
+
 void LaunchDownloadProcess(base::CommandLine* cmd_line) {
   std::unique_ptr<base::CommandLine> cmd_line_deleter(cmd_line);
 
@@ -236,6 +250,21 @@ void EstablishSurfacePeer(JNIEnv* env,
       &SetSurfacePeer, jsurface, pid, primary_id, secondary_id));
 }
 
+void CompleteScopedSurfaceRequest(JNIEnv* env,

[liberato (no reviews please), 2016/08/30 17:58:35]

/Complete/Fulfill ?

[tguilbert, 2016/08/30 22:53:09]

I don't know if it is easier to follow the sequence of calls if the function
names are all identical, or not. In any case, I have updated this method's
content, and the naming is now slightly different (and clearer IMO), so leaving
complete here, but I am open to re-visiting this in the new patch.

+                                  const JavaParamRef<jclass>& clazz,
+                                  jlong request_id,
+                                  const JavaParamRef<jobject>& surface) {
+  ScopedJavaGlobalRef<jobject> jsurface;
+  jsurface.Reset(env, surface);
+  if (jsurface.is_null())
+    return;
+
+  DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,

[watk, 2016/08/29 19:42:26]

Do we have guarantees about which thread MPR runs on? If we do, maybe we should
remove locking from SSRM and add DCHECKS instead? Seems easier to reason about
if we don't need locking in SSRM.

i.e., you would make Fulfill post to itself on the UI thread if it were not
called on the UI thread. And Register and Unregister would only be allowed to be
called on the UI thread.

[liberato (no reviews please), 2016/08/30 17:58:35]

conversely, why post here if there will not be any guarantees from the
"register" side, once it is added?

[tguilbert, 2016/08/30 22:53:09]

I think I have addressed this in this new patch, but let me know if I missed
something.

+      base::Bind(&GetAndCompleteScopedSurfaceRequest, jsurface, request_id));
+}
+
 void RegisterViewSurface(int surface_id, jobject j_surface) {
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);