Add ScopedSurfaceRequestManager

content/browser/android/child_process_launcher_android.cc

 // Copyright (c) 2012 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 "content/browser/android/child_process_launcher_android.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <memory>
 #include <utility>
 
 #include "base/android/context_utils.h"
 #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"
 #include "content/browser/media/android/media_web_contents_observer_android.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/child_process_host_impl.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/content_client.h"
@@ skipping 56 matching lines @@
     DVLOG(1) << "Cannot find media player for player_id " << player_id;
     return;
   }
 
   if (player != player_manager->GetFullscreenPlayer()) {
     gl::ScopedJavaSurface scoped_surface(surface);
     player->SetVideoSurface(std::move(scoped_surface));
   }
 }
 
+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);
 
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
 
   // Create the Command line String[]
   ScopedJavaLocalRef<jobjectArray> j_argv =
       ToJavaArrayOfStrings(env, cmd_line->argv());
 
@@ skipping 126 matching lines @@
   ScopedJavaGlobalRef<jobject> jsurface;
   jsurface.Reset(env, surface);
   if (jsurface.is_null())
     return;
 
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
   BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
       &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);
   Java_ChildProcessLauncher_registerViewSurface(env, surface_id, j_surface);
 }
 
 void UnregisterViewSurface(int surface_id) {
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
   Java_ChildProcessLauncher_unregisterViewSurface(env, surface_id);
@@ skipping 35 matching lines @@
 jboolean IsSingleProcess(JNIEnv* env, const JavaParamRef<jclass>& clazz) {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kSingleProcess);
 }
 
 bool RegisterChildProcessLauncher(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace content