Media Remoting: Draw remoting interstitial on poster image.

media/remoting/remoting_renderer_controller.cc

Index: media/remoting/remoting_renderer_controller.cc
diff --git a/media/remoting/remoting_renderer_controller.cc b/media/remoting/remoting_renderer_controller.cc
index 5f0d37eacdbc7a5939d49d5b0941e7077d07da8f..90c39cc429081fcdd5786a002b030582c34b5d56 100644
--- a/media/remoting/remoting_renderer_controller.cc
+++ b/media/remoting/remoting_renderer_controller.cc
@@ -73,6 +73,15 @@ void RemotingRendererController::OnSetCdm(CdmContext* cdm_context) {
   UpdateAndMaybeSwitch();
 }
 
+void RemotingRendererController::OnSetPoster(const GURL& poster_url) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (poster_url != poster_url_) {
+    poster_url_ = poster_url;
+    UpdateAndMaybeShowInterstitial();

[miu, 2016/12/20 00:48:06]

Instead of calling UpdateInterstitial() here, I would suggest you just run the
download callback here, and then let PosterImageDownloaded() decide whether to
call UpdateInterstitial() later.

[xjz, 2016/12/20 21:46:25]

Done. The poster image will be downloaded here only when url changes and
interstitial callback is set (after merging with another CL). After merge, it
will be also downloaded when the interstitial callback is set (if the url was
set before).

+  }
+}
+
 void RemotingRendererController::SetSwitchRendererCallback(
     const base::Closure& cb) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -243,4 +252,35 @@ void RemotingRendererController::UpdateAndMaybeSwitch() {
   }
 }
 
+void RemotingRendererController::SetDownloadPosterCallback(
+    const DownloadPosterCallback& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(download_poster_cb_.is_null());
+  download_poster_cb_ = cb;
+}
+
+void RemotingRendererController::UpdateAndMaybeShowInterstitial() {

[miu, 2016/12/20 00:48:06]

naming nit: It's fine to just call this UpdateInterstitial() (This'll be a merge
task after your other CL lands.)

[xjz, 2016/12/20 21:46:25]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (pipeline_metadata_.natural_size.IsEmpty())
+    return;
+
+  DCHECK(remote_rendering_started_);
+  if (poster_url_.is_empty() || download_poster_cb_.is_null()) {
+    // TODO(xjz): Draw interstitial on a black image and show it.
+    // https://codereview.chromium.org/2566223005/
+  } else {
+    download_poster_cb_.Run(
+        poster_url_,
+        base::Bind(&RemotingRendererController::PosterImageDownloaded,
+                   weak_factory_.GetWeakPtr()));

[miu, 2016/12/20 00:48:06]

Note: Recommend you pass |poster_url_| as an argument here. (See comment below
for why.)

[xjz, 2016/12/20 21:46:25]

Done.

+  }
+}
+
+void RemotingRendererController::PosterImageDownloaded(const SkBitmap& image) {

[miu, 2016/12/20 00:48:06]

naming nit: OnPosterImageDownloaded().

[miu, 2016/12/20 00:48:06]

Recommend you double-check that the Poster URL has not changed in the meantime.
If it has, just discard the result. Something like:

void RemotingRendererController::PosterImageDownloaded(const GURL& download_url,
const SkBitmap& image) {
  DCHECK(thread_checker_.CalledOnValidThread());
  if (download_url != poster_url_) {
    // The poster image URL has changed during the download.
    return;
  }
  poster_image_ = image;
  UpdateInterstitial();
}

[xjz, 2016/12/20 21:46:25]

Done.

[xjz, 2016/12/20 21:46:25]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(remote_rendering_started_);

[miu, 2016/12/20 00:48:06]

Since the downloading can take an unbounded amount of time, it's easily possible
that remote rendering was stopped during the download. That shouldn't crash the
process, so I suggest removing this DCHECK(). Just let the call to
UpdateInterstitial() decide what to do.

[xjz, 2016/12/20 21:46:25]

Done.

+  // TODO(xjz): Draw interstitial on |image| and show it.
+  // https://codereview.chromium.org/2566223005/
+}
+
 }  // namespace media