Upstream image_fetcher::ImageFetcher

ios/chrome/browser/net/image_fetcher_unittest.mm

+// Copyright 2011 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.
+
+#import "ios/chrome/browser/net/image_fetcher.h"
+
+#import <UIKit/UIKit.h>
+
+#include "base/mac/bind_objc_block.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "base/run_loop.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "net/http/http_response_headers.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace {
+
+static unsigned char kJPGImage[] = {
+    255,216,255,224,0,16,74,70,73,70,0,1,1,1,0,72,0,72,0,0,255,254,0,19,67,
+    114,101,97,116,101,100,32,119,105,116,104,32,71,73,77,80,255,219,0,67,
+    0,5,3,4,4,4,3,5,4,4,4,5,5,5,6,7,12,8,7,7,7,7,15,11,11,9,12,17,15,18,18,
+    17,15,17,17,19,22,28,23,19,20,26,21,17,17,24,33,24,26,29,29,31,31,31,
+    19,23,34,36,34,30,36,28,30,31,30,255,219,0,67,1,5,5,5,7,6,7,14,8,8,14,
+    30,20,17,20,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,
+    30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,30,
+    30,30,30,30,30,30,30,30,255,192,0,17,8,0,1,0,1,3,1,34,0,2,17,1,3,17,1,
+    255,196,0,21,0,1,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,8,255,196,0,20,16,1,
+    0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,196,0,20,1,1,0,0,0,0,0,0,0,0,0,0,
+    0,0,0,0,0,0,255,196,0,20,17,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,255,218,
+    0,12,3,1,0,2,17,3,17,0,63,0,178,192,7,255,217
+};
+
+static unsigned char kPNGImage[] = {
+    137,80,78,71,13,10,26,10,0,0,0,13,73,72,68,82,0,0,0,1,0,0,0,1,1,0,0,0,
+    0,55,110,249,36,0,0,0,2,98,75,71,68,0,1,221,138,19,164,0,0,0,9,112,72,
+    89,115,0,0,11,18,0,0,11,18,1,210,221,126,252,0,0,0,9,118,112,65,103,0,
+    0,0,1,0,0,0,1,0,199,149,95,237,0,0,0,10,73,68,65,84,8,215,99,104,0,0,
+    0,130,0,129,221,67,106,244,0,0,0,25,116,69,88,116,99,111,109,109,101,
+    110,116,0,67,114,101,97,116,101,100,32,119,105,116,104,32,71,73,77,80,
+    231,175,64,203,0,0,0,37,116,69,88,116,100,97,116,101,58,99,114,101,97,
+    116,101,0,50,48,49,49,45,48,54,45,50,50,84,49,54,58,49,54,58,52,54,43,
+    48,50,58,48,48,31,248,231,223,0,0,0,37,116,69,88,116,100,97,116,101,58,
+    109,111,100,105,102,121,0,50,48,49,49,45,48,54,45,50,50,84,49,54,58,49,
+    54,58,52,54,43,48,50,58,48,48,110,165,95,99,0,0,0,17,116,69,88,116,106,
+    112,101,103,58,99,111,108,111,114,115,112,97,99,101,0,50,44,117,85,159,
+    0,0,0,32,116,69,88,116,106,112,101,103,58,115,97,109,112,108,105,110,
+    103,45,102,97,99,116,111,114,0,50,120,50,44,49,120,49,44,49,120,49,73,
+    250,166,180,0,0,0,0,73,69,78,68,174,66,96,130
+};
+
+static unsigned char kWEBPImage[] = {
+    82,73,70,70,74,0,0,0,87,69,66,80,86,80,56,88,10,0,0,0,16,0,0,0,0,0,0,0,0,0,
+    65,76,80,72,12,0,0,0,1,7,16,17,253,15,68,68,255,3,0,0,86,80,56,32,24,0,0,0,
+    48,1,0,157,1,42,1,0,1,0,3,0,52,37,164,0,3,112,0,254,251,253,80,0
+};
+
+static char kTestUrl[] = "http://www.img.com";

[Ryan Sleevi, 2014/12/12 23:32:01]

s/char/const char/

[sdefresne, 2014/12/15 13:11:58]

Done.

+
+}  // namespace
+
+class ImageFetcherTest : public PlatformTest {
+ protected:
+  ImageFetcherTest()
+      : pool_(new base::SequencedWorkerPool(1, "TestPool")),

[Ryan Sleevi, 2014/12/12 23:32:01]

Do you need to actually spin up a dedicated pool for this test? Can you not just
pass in your own SequencedTaskRunner?

[sdefresne, 2014/12/15 13:11:58]

//ios/web does not have a test helper that create the web::WebThread blocking
pool, so for the moment it is simpler to create our own pool.

+        image_fetcher_(new image_fetcher::ImageFetcher(pool_)),
+        result_(nil),
+        called_(false) {
+    callback_.reset(
+        [^(const GURL& original_url, int http_response_code, NSData* data) {
+            result_ = [UIImage imageWithData:data];
+            called_ = true;
+        } copy]);
+    image_fetcher_->SetRequestContextGetter(
+        new net::TestURLRequestContextGetter(
+            base::MessageLoopProxy::current()));

[Ryan Sleevi, 2014/12/12 23:32:01]

DESIGN: prefer base::ThreadTaskRunnerHandle::Get() to
MessageLoopProxy::current()

One of the biggest differences between these two is that MLP::current() will
return NULL if no associated message loop, whereas TTRH will DCHECK. I have seen
countless bugs in test and production come up with MLP::current() is NULL, so
it's much easier to ensure a guarantee. Plus, it fits with deprecating the MLP
in favour of the TTRH (which is primarily about ensuring Chromium code runs in a
variety of environments, such as NaCl, ChromeOS, etc). Yes, not strictly
applicable here, but better as a pattern to avoid the deprecated thing.

[sdefresne, 2014/12/15 13:11:58]

Done.

+  }
+
+  ~ImageFetcherTest() override { pool_->Shutdown(); }
+
+  net::TestURLFetcher* SetupFetcher() {
+    image_fetcher_->StartDownload(GURL(kTestUrl), callback_);
+    EXPECT_EQ(static_cast<UIImage*>(nil), result_);

[Ryan Sleevi, 2014/12/12 23:32:01]

nullptr rather than nil here? Or is the difference semantically meaningful?

nullptr let's you avoid the cast, IINM, because it's a distinguishable type.

[sdefresne, 2014/12/15 13:11:58]

iOS downstream convention is to use "nil" for pointers to Objective-C types and
"nullptr" for pointers to C++ types. Here we have a pointer to a UIImage, so an
Objective-C object. When compiled with Xcode 6.0 or later, this is a
distinguishable type too (and the code compile cleanly), however some bots are
still running Xcode 5.1 (http://crbug.com/440857) so until they have been
upgrade I need the casts.

+    EXPECT_EQ(false, called_);
+    net::TestURLFetcher* fetcher = factory_.GetFetcherByID(0);
+    DCHECK(fetcher);
+    DCHECK(fetcher->delegate());
+    return fetcher;
+  }
+
+  base::MessageLoop loop_;
+  base::mac::ScopedBlock<image_fetcher::Callback> callback_;
+  net::TestURLFetcherFactory factory_;
+  scoped_refptr<base::SequencedWorkerPool> pool_;
+  scoped_ptr<image_fetcher::ImageFetcher> image_fetcher_;
+  UIImage* result_;
+  bool called_;
+};
+
+TEST_F(ImageFetcherTest, TestError) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(404);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_EQ(static_cast<UIImage*>(nil), result_);
+  EXPECT_TRUE(called_);
+}
+
+TEST_F(ImageFetcherTest, TestJpg) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(200);
+  fetcher->SetResponseString(std::string((char*)kJPGImage, sizeof(kJPGImage)));
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_NE(static_cast<UIImage*>(nil), result_);
+  EXPECT_TRUE(called_);
+}
+
+TEST_F(ImageFetcherTest, TestPng) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(200);
+  fetcher->SetResponseString(std::string((char*)kPNGImage, sizeof(kPNGImage)));
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  EXPECT_NE(static_cast<UIImage*>(nil), result_);
+  EXPECT_TRUE(called_);
+}
+
+TEST_F(ImageFetcherTest, TestGoodWebP) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(200);
+  fetcher->SetResponseString(
+      std::string((char*)kWEBPImage, sizeof(kWEBPImage)));
+  std::string kZero = std::string("\0", 1);
+  std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
+      "Content-type: image/webp" + kZero + kZero;
+  scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
+      header_string));
+  fetcher->set_response_headers(headers);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  pool_->FlushForTesting();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_NE(static_cast<UIImage*>(nil), result_);
+  EXPECT_TRUE(called_);
+}
+
+TEST_F(ImageFetcherTest, TestBadWebP) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(200);
+  fetcher->SetResponseString("This is not a valid WebP image");
+  std::string kZero = std::string("\0", 1);
+  std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
+      "Content-type: image/webp" + kZero + kZero;
+  scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
+      header_string));
+  fetcher->set_response_headers(headers);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  pool_->FlushForTesting();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(static_cast<UIImage*>(nil), result_);
+  EXPECT_TRUE(called_);
+}
+
+TEST_F(ImageFetcherTest, DeleteDuringWebPDecoding) {
+  net::TestURLFetcher* fetcher = SetupFetcher();
+  fetcher->set_response_code(200);
+  fetcher->SetResponseString(
+      std::string((char*)kWEBPImage, sizeof(kWEBPImage)));
+  std::string kZero = std::string("\0", 1);

[Ryan Sleevi, 2014/12/12 23:32:01]

s/kZero/kNull

However, the construction of the header string is needlessly complex.

std::string header_string("HTTP/1.1 200 OK\0"
                          "Content-type: image/webp\0"
                          "\0");

avoid unnecessary concats. Ditto line 153-154, 136-137, etc.

[sdefresne, 2014/12/15 13:11:58]

Done using std::string(const char*, size_t) constructor and arraysize() to get
the length of the string containing NUL bytes (as std::string(const char*) stops
at the first NUL byte).

+  std::string header_string = std::string("HTTP/1.1 200 OK") + kZero +
+      "Content-type: image/webp" + kZero + kZero;
+  scoped_refptr<net::HttpResponseHeaders> headers(new net::HttpResponseHeaders(
+      header_string));
+  fetcher->set_response_headers(headers);
+  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  // Delete the image fetcher, and check that the callback is not called.
+  image_fetcher_.reset();
+  pool_->FlushForTesting();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(static_cast<UIImage*>(nil), result_);
+  EXPECT_FALSE(called_);
+}
+
+TEST_F(ImageFetcherTest, TestCallbacksNotCalledDuringDeletion) {
+  image_fetcher_->StartDownload(GURL(kTestUrl), callback_);
+  image_fetcher_.reset();
+  EXPECT_FALSE(called_);
+}
+