ImageLoaderImageFamilyCallback: Take ImageFamily value, not reference.

The ImageLoader now std::moves the ImageFamily into the callback, rather
than holding onto a reference throughout the duration of the callback.

Fixes a data race in ScheduleCreatePlatformShortcut where the
ImageFamily could not be safely handed off to the other thread (because
the ImageLoader was still holding onto a reference).

Bug: 749342
Change-Id: I1452e47c892660e5fd06e27e3a0582240a5cbcec
Reviewed-on: https://chromium-review.googlesource.com/590338Reviewed-by: Ben Wells <benwells@chromium.org>
Commit-Queue: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#490708}

chrome/browser/extensions/bookmark_app_helper.cc

@@ -145,7 +145,7 @@ class GeneratedIconImageSource : public gfx::CanvasImageSource {
 void OnIconsLoaded(
     WebApplicationInfo web_app_info,
     const base::Callback<void(const WebApplicationInfo&)> callback,
-    const gfx::ImageFamily& image_family) {
+    gfx::ImageFamily image_family) {
   for (gfx::ImageFamily::const_iterator it = image_family.begin();
        it != image_family.end();
        ++it) {

chrome/browser/web_applications/web_app.cc

@@ -100,7 +100,7 @@ void UpdateAllShortcutsForShortcutInfo(
 
 void OnImageLoaded(std::unique_ptr<web_app::ShortcutInfo> shortcut_info,
                    web_app::ShortcutInfoCallback callback,
-                   const gfx::ImageFamily& image_family) {
+                   gfx::ImageFamily image_family) {
   // If the image failed to load (e.g. if the resource being loaded was empty)
   // use the standard application icon.
   if (image_family.empty()) {
@@ -116,12 +116,7 @@ void OnImageLoaded(std::unique_ptr<web_app::ShortcutInfo> shortcut_info,
     image_skia.MakeThreadSafe();
     shortcut_info->favicon.Add(gfx::Image(image_skia));
   } else {
-    // TODO(mgiuca): Move |image_family| instead of copying it. This copy
-    // results in a refcount of 2 for the duration of the callback, leading to a
-    // data race decrementing the refcount. This requires a change to the
-    // ImageLoaderImageFamilyCallback interface (to pass by value).
-    // https://crbug.com/749342.
-    shortcut_info->favicon = image_family.Clone();
+    shortcut_info->favicon = std::move(image_family);
   }
 
   callback.Run(std::move(shortcut_info));

extensions/browser/image_loader.cc

@@ -338,7 +338,7 @@ void ImageLoader::ReplyBackWithImageFamily(
     image_family.Add(it->second);
   }
 
-  callback.Run(image_family);
+  callback.Run(std::move(image_family));
 }
 
 }  // namespace extensions

extensions/browser/image_loader.h

@@ -29,8 +29,7 @@ namespace extensions {
 class Extension;
 
 typedef base::Callback<void(const gfx::Image&)> ImageLoaderImageCallback;
-typedef base::Callback<void(const gfx::ImageFamily&)>
-    ImageLoaderImageFamilyCallback;
+typedef base::Callback<void(gfx::ImageFamily)> ImageLoaderImageFamilyCallback;
 
 // This class is responsible for asynchronously loading extension images and
 // calling a callback when an image is loaded.

extensions/browser/image_loader_unittest.cc

@@ -47,11 +47,11 @@ class ImageLoaderTest : public ExtensionsTest {
     image_ = image;
   }
 
-  void OnImageFamilyLoaded(const gfx::ImageFamily& image_family) {
+  void OnImageFamilyLoaded(gfx::ImageFamily image_family) {
     image_loaded_count_++;
     if (quit_in_image_loaded_)
       base::RunLoop::QuitCurrentWhenIdleDeprecated();
-    image_family_ = image_family.Clone();
+    image_family_ = std::move(image_family);
   }
 
   void WaitForImageLoad() {