Media Remoting: Draw remoting interstitial on poster image.

content/renderer/image_downloader/single_image_downloader.cc

+// Copyright 2015 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/renderer/image_downloader/single_image_downloader.h"
+
+#include "base/bind.h"
+#include "base/logging.h"
+
+namespace content {
+
+// Static
+void SingleImageDownloader::DownloadImage(
+    const base::WeakPtr<RenderFrame>& render_frame,
+    const GURL& url,
+    const DownloadImageCallback& cb) {
+  if (cb.is_null())

[miu, 2016/12/03 01:05:31]

Should be DCHECK().

[xjz, 2016/12/06 19:50:55]

Done.

+    return;
+
+  if (!render_frame)
+    cb.Run(SkBitmap());

[miu, 2016/12/03 01:05:31]

ditto: Should be DCHECK().

[xjz, 2016/12/06 19:50:55]

Done.

+
+  // Will request to download the image and run |cb| when done. This will be
+  // self-destructed after done or when |this| is destructed.
+  new SingleImageDownloader(render_frame.get(), url, false, false, cb);
+}
+
+SingleImageDownloader::SingleImageDownloader(
+    RenderFrame* render_frame,
+    const GURL& url,
+    bool is_favicon,
+    bool bypass_cache,
+    const DownloadImageCallback& callback)
+    : RenderFrameObserver(render_frame), image_downloader_(render_frame) {
+  DCHECK(render_frame);
+  DCHECK(callback);
+  image_downloader_.DownloadImage(
+      url, is_favicon, bypass_cache,
+      base::Bind(&SingleImageDownloader::DidDownloadImage,
+                 base::Unretained(this), callback));
+}
+
+SingleImageDownloader::~SingleImageDownloader() {}
+
+void SingleImageDownloader::DidDownloadImage(
+    const DownloadImageCallback& callback,
+    int http_status_code,
+    const std::vector<SkBitmap>& images) {
+  DCHECK(callback);
+  callback.Run(images[0]);

[miu, 2016/12/03 01:05:31]

Can the |images| vector ever be empty? How about:

  callback.Run(images.empty() ? SkBitmap() : images[0]);

[xjz, 2016/12/06 19:50:55]

Done.

+  delete this;
+}
+
+void SingleImageDownloader::OnDestruct() {
+  delete this;

[miu, 2016/12/03 01:05:30]

This second path-to-delete is dangerous and could result in duplicate deletes.

Instead, we should make sure the Core class is guaranteed to ALWAYS run the
DownloadCallback (which calls DidDownloadCallback()). So, if the RenderFrame is
destroyed during a download, the Core class would run the callback with an empty
SkBitmap() to indicate the download was aborted.

Note: If you make this change, then you no longer need this class to inherit
from RenderFrameObserver. After that, there's no reason to instantiate this
class at all: Your static public function becomes:

// static
void SingleImageDownloader::DownloadImageAt(RenderFrame* render_frame,
                                            const GURL& url,
                                            const base::Callback<void(const
SkBitmap&)& callback) {
  ...DCHECKs here...

  std::unique_ptr<ImageDowloaderCore> core(new ...);
  ImageDownloaderCore* core_ptr = core.get();
  core_ptr->DownloadImage(..., base::Bind(DidDownloadImage, base::Passed(&core),
callback));
}

and then the private DidDownloadImage() becomes static:

// static
void DidDownloadImage(std::unique_ptr<ImageDownloaderCore> to_be_deleted, ...) {
  callback.Run(...);
}

[xjz, 2016/12/06 19:50:55]

I don't think this will cause duplicated delete. There might be two conditions
to delete "this": 1. DidDownloadImage() is called first, "this" is deleted and
stopped observing RenderFrame, so OnDestruct() will not be called. 2.
RenderFrame is gone, OnDestruct() is called first, "this" is deleted, so
|image_downloader_| (core) is deleted, which owns all fetchers in
|image_fetchers_|, which are also deleted, and the callback is never be run. .
I didn't do this way because:
1. When RenderFrame is gone, the ImageDownloaderBase should be destroyed to
prevent further calling FetchImage(). If instead call the callback anyway, then
each subclass is required to override OnDestruct() or "delete this" in the
callback. I think it makes sense to let the ImageDownloaderBase itself handle
this.
2. ImageDonwloaderBase then needs to maintain a map with all image fetchers and
the corresponding callbacks just for this purpose.

[xjz, 2016/12/06 23:58:17]

Now after making ImageDownloaderBase run callbacks when RenderFrame is
destructed, changed SingleImageDownloader as suggested in the original comment.

+}
+
+}  // namespace content