Support WebP images in the IOSImageDecoderImpl

ios/chrome/browser/suggestions/ios_image_decoder_impl.mm

Index: ios/chrome/browser/suggestions/ios_image_decoder_impl.mm
diff --git a/ios/chrome/browser/suggestions/ios_image_decoder_impl.mm b/ios/chrome/browser/suggestions/ios_image_decoder_impl.mm
index f28d382f1fb0e538bbe54b28161a81e245d5415b..96d5b96d72cf6efe0780ee3942c2b93fd50516fb 100644
--- a/ios/chrome/browser/suggestions/ios_image_decoder_impl.mm
+++ b/ios/chrome/browser/suggestions/ios_image_decoder_impl.mm
@@ -7,29 +7,143 @@
 #include <UIKit/UIKit.h>
 
 #include "base/callback.h"
+#include "base/mac/scoped_nsobject.h"
+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "ios/chrome/browser/webp_transcode/webp_decoder.h"
+#include "ios/web/public/web_thread.h"
 #include "ui/gfx/image/image.h"
 
+namespace {
+
+class WebpDecoderDelegate : public webp_transcode::WebpDecoder::Delegate {
+ public:
+  NSData* data() const { return decoded_image_; }
+
+  // WebpDecoder::Delegate methods
+  void OnFinishedDecoding(bool success) override {
+    if (!success)
+      decoded_image_.reset();
+  }
+
+  void SetImageFeatures(
+      size_t total_size,
+      webp_transcode::WebpDecoder::DecodedImageFormat format) override {
+    decoded_image_.reset([[NSMutableData alloc] initWithCapacity:total_size]);
+  }
+
+  void OnDataDecoded(NSData* data) override {
+    DCHECK(decoded_image_);
+    [decoded_image_ appendData:data];
+  }
+
+ private:
+  ~WebpDecoderDelegate() override {}
+  base::scoped_nsobject<NSMutableData> decoded_image_;
+};

[sdefresne, 2016/10/05 09:15:28]

DISALLOW_COPY_AND_ASSIGN(WebpDecoderDelegate);

[vitaliii, 2016/10/06 06:35:08]

Done.

+
+// Returns an NSData object containing the decoded image data of the given
+// webp_image. Returns nil in case of failure.
+base::scoped_nsobject<NSData> DecodeWebpImage(
+    const base::scoped_nsobject<NSData>& webp_image) {
+  scoped_refptr<WebpDecoderDelegate> delegate(new WebpDecoderDelegate);
+  scoped_refptr<webp_transcode::WebpDecoder> decoder(
+      new webp_transcode::WebpDecoder(delegate.get()));
+  decoder->OnDataReceived(webp_image);
+  DLOG_IF(ERROR, !delegate->data()) << "WebP image decoding failed.";
+  return base::scoped_nsobject<NSData>([delegate->data() retain]);
+}
+
+// Returns true if the given image_data is a WebP image.
+//
+// Every WebP file contains a 12 byte file header in the beginning of the file.
+// A WebP file header starts with the four ASCII characters "RIFF". The next
+// four bytes contain the image size and the last four header bytes contain the
+// four ASCII characters "WEBP".
+//
+// WebP file header:
+//                                  1 1
+// Byte Nr.     0 1 2 3 4 5 6 7 8 9 0 1
+// Byte value [ R I F F ? ? ? ? W E B P  ]
+//
+// For more information see:
+// https://developers.google.com/speed/webp/docs/riff_container#webp_file_header
+bool IsWebpImage(const std::string& image_data) {
+  if (image_data.length() < 12)
+    return false;
+  return image_data.compare(0, 4, "RIFF") == 0 &&
+         image_data.compare(8, 4, "WEBP") == 0;
+}
+
+}  // namespace
+
 namespace suggestions {
 
-IOSImageDecoderImpl::IOSImageDecoderImpl() {}
+class IOSImageDecoderImpl : public image_fetcher::ImageDecoder {
+ public:
+  explicit IOSImageDecoderImpl(scoped_refptr<base::TaskRunner> task_runner);

[sdefresne, 2016/10/05 09:15:28]

const scoped_refptr<base::TaskRunner>& task_runner

[vitaliii, 2016/10/06 06:35:09]

Done.

However, I was advised against |const &| and the argument was 

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

Is this different for iOS?
Or is this because there is no |const &| in |CreateIOSImageDecoder| and so the
argument above is applied there?

[sdefresne, 2016/10/06 15:53:06]

Since the implementation does not use std::move() [line 108], then it is better
to pass the scoped_refptr<> per const reference. If you want to pass it by copy,
then you should have used std::move() in the implementation.

So, I think we should either use "const scoped_refptr<>&" in IOSImageDecoderImpl
constructor and CreateIOSImageDecoder or neither. Given that
CreateIOSImageDecoder does not use refefence, maybe we should not here either
and instead do the following:

IOSImageDecoderImpl::IOSImageDecoderImpl(scoped_refptr<base::TaskRunner>
task_runner)
    : task_runner_(std::move(task_runner)), weak_factory_(this) {}

std::unique_ptr<image_fetcher::ImageDecoder> createIOSImageDecoder(
    scoped_refptr<base::TaskRunner> task_runner) {
  return base::MakeUnique<image_fetcher::ImageDecoder>(
      IOSImageDecoderImpl(std::move(task_runner));
}

Sorry for the churn, I was just trying to be consistent in use a const-reference
in the file (I should instead have followed the recommendation you pointed to).

[vitaliii, 2016/10/07 09:50:31]

No worries :)
That is a very good point about std::move in the implementation, which I missed
previously. I choose your second approach (adding std::move to the
implementation), which agrees with the link above. Thanks for the explanation!

+  ~IOSImageDecoderImpl() override;
+
+  void DecodeImage(
+      const std::string& image_data,
+      const image_fetcher::ImageDecodedCallback& callback) override;
+
+ private:
+  void CreateUIImageAndRunCallback(
+      const image_fetcher::ImageDecodedCallback& callback,
+      base::scoped_nsobject<NSData> image_data);
+
+  // The task runner used to decode images if necessary.
+  const scoped_refptr<base::TaskRunner> task_runner_;
+
+  // The WeakPtrFactory is used to cancel callbacks if ImageFetcher is
+  // destroyed during WebP decoding.
+  base::WeakPtrFactory<IOSImageDecoderImpl> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(IOSImageDecoderImpl);
+};
+
+IOSImageDecoderImpl::IOSImageDecoderImpl(
+    scoped_refptr<base::TaskRunner> task_runner)
+    : task_runner_(task_runner), weak_factory_(this) {
+  DCHECK(task_runner_.get());
+}
 
 IOSImageDecoderImpl::~IOSImageDecoderImpl() {}
 
 void IOSImageDecoderImpl::DecodeImage(
     const std::string& image_data,
     const image_fetcher::ImageDecodedCallback& callback) {
-  // Convert the |image_data| std::string to a NSData buffer.
-  NSData* data =
-      [NSData dataWithBytesNoCopy:const_cast<char*>(image_data.c_str())
-                           length:image_data.length()
-                     freeWhenDone:NO];
-
-  // Decode the Image using UIImage.
-  if (data) {
-    // Most likely always returns 1x images.
-    UIImage* ui_image = [UIImage imageWithData:data scale:1];
+  // Convert the |image_data| std::string to an NSData buffer.
+  base::scoped_nsobject<NSData> data(
+      [[NSData dataWithBytesNoCopy:const_cast<char*>(image_data.c_str())
+                            length:image_data.length()
+                      freeWhenDone:NO] retain]);
+
+  // The WebP image format is not supported by iOS natively. Therefore WebP
+  // images need to be decoded explicitly,
+  if (IsWebpImage(image_data)) {
+    base::PostTaskAndReplyWithResult(
+        task_runner_.get(), FROM_HERE, base::Bind(&DecodeWebpImage, data),
+        base::Bind(&IOSImageDecoderImpl::CreateUIImageAndRunCallback,
+                   weak_factory_.GetWeakPtr(), callback));
+  } else {
+    CreateUIImageAndRunCallback(callback, data);
+  }
+}
+
+void IOSImageDecoderImpl::CreateUIImageAndRunCallback(
+    const image_fetcher::ImageDecodedCallback& callback,
+    base::scoped_nsobject<NSData> image_data) {

[sdefresne, 2016/10/05 09:15:28]

const base::scoped_nsobject<NSData>& image_data

[vitaliii, 2016/10/06 06:35:09]

Done.

+  // Decode the image data using UIImage.
+  if (image_data) {
+    // "Most likely" always returns 1x images.
+    UIImage* ui_image = [UIImage imageWithData:image_data scale:1];
     if (ui_image) {
-      gfx::Image gfx_image(ui_image);
+      // This constructor does not retain the image, but expects to take the
+      // ownership, therefore, |ui_image| is retained here, but not released
+      // afterwards.
+      gfx::Image gfx_image([ui_image retain]);
       callback.Run(gfx_image);
       return;
     }
@@ -38,4 +152,10 @@ void IOSImageDecoderImpl::DecodeImage(
   callback.Run(empty_image);
 }
 
+std::unique_ptr<image_fetcher::ImageDecoder> createIOSImageDecoder(
+    scoped_refptr<base::TaskRunner> task_runner) {
+  return std::unique_ptr<image_fetcher::ImageDecoder>(

[sdefresne, 2016/10/05 09:15:28]

Please use base::MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)).
No need to use std::move for a scoped_refptr<T> either.

#include "base/memory/ptr_util.h"

return base::MakeUnique<IOSImageDecoderImpl>(task_runner);

[vitaliii, 2016/10/06 06:35:08]

Done.

+      new IOSImageDecoderImpl(std::move(task_runner)));
+}
+
 }  // namespace suggestions