Integrate Surface requests with MediaPlayerRenderer

content/renderer/media/android/media_player_renderer_client.cc

Index: content/renderer/media/android/media_player_renderer_client.cc
diff --git a/content/renderer/media/android/media_player_renderer_client.cc b/content/renderer/media/android/media_player_renderer_client.cc
index 20a5871c83ec1a88b38e80af2b8b351058b1d327..b035bff3ec65619c4b9f0ff973192841c515576e 100644
--- a/content/renderer/media/android/media_player_renderer_client.cc
+++ b/content/renderer/media/android/media_player_renderer_client.cc
@@ -58,13 +58,31 @@ void MediaPlayerRendererClient::InitializeRemoteRenderer(
                  weak_factory_.GetWeakPtr()));
 }
 
+void MediaPlayerRendererClient::OnScopedSurfaceRequested(
+    const base::Optional<base::UnguessableToken>& request_token) {
+  if (!request_token) {

[dcheng, 2016/10/03 21:11:27]

After thinking about this over the weekend... I'm still have trouble wrapping my
head around this reply callback.

The mojom declaration says this can return nullopt on error. At the same time,
this has a NOTREACHED().

1) Is there a reason why we'd want to call this on the renderer if the
MojoRendererService wasn't constructed with a non-null callback? The
NOTREACHED() seems to indicate this should never happen.

2) If it only for error cases, does a NOTREACHED() still make sense? What are
the errors that could cause this?

[tguilbert, 2016/10/03 22:26:19]

1) There is no situation where we would want to call OnScopedSurfaceRequest if
the media::Renderer* that was given to the MojoRendererService (MRS) at creation
time is not a content::MediaPlayerRenderer*. There is no way for the
MediaPlayerRendererClient to poll the MRS and to make sure it properly
constructed.
Currently, the MRS registered at the frame host level will be the only MRS to be
constructed with a MediaPlayerRenderer. 
If we receive an empty nullopt token in OnScopedSurfaceRequested, this is an
initialization/configuration error from the developer, rather than an unforseen
runtime error. It would therefore be more appropriate to change the 
comment in the mojom to "configuration error".

NOTE: I still haven't sent the CL that actually registers the MRS.

2) There should be no "runtime error" that could cause this.

Would it make make sense to clarify the comments in the .mojom and to leave the
NOTREACHED() here there?

[dcheng, 2016/10/04 06:28:39]

OK, thanks for the explanation. Let's update the mojom to mention this.

Given this, I personally prefer to DCHECK that request_token is non-null. To me,
when the if () branch is written out explicitly, it indicates that something can
happen, whereas DCHECK() is an explicit indication of precondition that should
never be violated.

[tguilbert, 2016/10/05 01:30:11]

Done. (Although, the request_token is no longer a base::Optional)

+    // No request was registered in the browser.
+    // The MojoRendererService was not constructed with an
+    // InitiateSurfaceRequestCB. This means that the renderer in the browser
+    // process likely isn't a MediaPlayerRenderer.
+    NOTREACHED();
+    return;
+  }
+
+  stream_texture_wrapper_->ForwardStreamTextureForSurfaceRequest(
+      request_token.value());
+}
+
 void MediaPlayerRendererClient::CompleteInitialization(
     media::PipelineStatus status) {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
   DCHECK(!init_cb_.is_null());
 
-  // TODO(tguilbert): Register |stream_texture_wrapper_|'s surface and send it
-  // to MediaPlayerRenderer via |mojo_renderer_|. See crbug.com/627658.
+  // TODO(tguilbert): Measure and smooth out the initialization's ordering to
+  // have the lowest total initialization time.
+  mojo_renderer_->InitiateScopedSurfaceRequest(
+      base::Bind(&MediaPlayerRendererClient::OnScopedSurfaceRequested,
+                 weak_factory_.GetWeakPtr()));
 
   base::ResetAndReturn(&init_cb_).Run(status);
 }