Continue bitmap fetching for notifications.

chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc

+// Copyright (c) 2013 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 "chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h"
+
+#include "base/compiler_specific.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_utils.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_fetcher.h"
+#include "net/url_request/url_request_status.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/skia/include/core/SKBitmap.h"
+#include "ui/gfx/codec/png_codec.h"
+#include "ui/gfx/size.h"
+#include "ui/gfx/skia_util.h"
+
+namespace {
+// FF0000FF is 100% alpha and max green.(A, R, B, G)
+uint32 kMaxGreen = 0xFF0000FF;
+const bool kAsyncCall = true;
+const bool kSyncCall = false;
+const bool kSuccess = true;
+const bool kFailure = false;
+}  // namespace
+
+namespace notifier {
+
+// Class to catch events from the NotificationBitmapFetcher for testing.
+class NotificationBitmapFetcherTestDelegate
+    : public NotificationBitmapFetcherDelegate {
+ public:
+  explicit NotificationBitmapFetcherTestDelegate(bool async)

[dcheng, 2013/05/30 22:38:09]

Since all tests are now async, the async_ parameters and its associated
constants are no longer needed.

[Pete Williamson, 2013/05/31 00:17:39]

We still have one sync test (and I may add more someday)

[dcheng, 2013/05/31 00:27:38]

How is StartTest different from OnImageDecodedTest?

[Pete Williamson, 2013/05/31 01:01:44]

The primary difference is that we are testing a different callback.  Instead of
testing the URL fetch, which is the first stage of processing, here we are
testing the results from the second stage of processing, the bitmap decoding.

There are several philosophies of unit testing.  This was originally written to
be a unit test trying to test this function in isolation.  I think there is
value in testing functions in isolation, so I'd like to leave this test as is. 
If you are convinced that we also need a more end-to-end test, I can certainly
add one later.

[dcheng, 2013/05/31 02:34:52]

I'm trying to think of a situation where StartTest will pass and
OnImageDecodedTest will not, and I can't think of anything. This, combined with
that the code flow being tested will never occur in practice in the browser,
makes it hard for me to understand why OnImageDecodedTest is needed.

[Pete Williamson, 2013/05/31 16:42:20]

If start test fails, I can quickly tell by whether ImageDecocedTest also fails
whether the problem was in image decoding or URL fetching.

+      : success_(false), async_(async) {}
+
+  virtual ~NotificationBitmapFetcherTestDelegate() {}
+
+  // Method inherited from NotificationBitmapFetcherDelegate
+  void OnFetchComplete(const SkBitmap* bitmap) OVERRIDE {
+    if (NULL != bitmap) {
+      success_ = true;
+      bitmap->deepCopyTo(&bitmap_, bitmap->getConfig());
+    }
+    // For async calls, we need to quit the message loop so the test can
+    // continue.
+    if (async_) {
+      MessageLoop::current()->Quit();
+    }
+  }
+
+  bool success() const { return success_; }
+  const SkBitmap& bitmap() const { return bitmap_; }
+
+ private:
+  bool success_;
+  bool async_;
+  SkBitmap bitmap_;
+
+  DISALLOW_COPY_AND_ASSIGN(NotificationBitmapFetcherTestDelegate);
+};
+
+typedef InProcessBrowserTest NotificationBitmapFetcherBrowserTest;
+
+// WARNING:  These tests work with --single_process, but not
+// --single-process.  The reason is that the sandbox does not get created
+// for us by the test process if --single-process is used.
+
+IN_PROC_BROWSER_TEST_F(NotificationBitmapFetcherBrowserTest,
+                       StartTest) {
+  GURL url("http://localhost");
+
+  // Put some realistic looking bitmap data into the url_fetcher.
+  SkBitmap image;
+
+  // Put a real bitmap into "image".  2x2 bitmap of green 32 bit pixels.
+  image.setConfig(SkBitmap::kARGB_8888_Config, 2, 2);
+  image.allocPixels();
+  SkColor c = kMaxGreen;
+  image.eraseColor(c);
+
+  // Encode the bits as a PNG.
+  std::vector<unsigned char> compressed;
+  ASSERT_TRUE(gfx::PNGCodec::EncodeBGRASkBitmap(image, true, &compressed));
+
+  // Copy the bits into the string, and put them into the StubURLFetcher.

[dcheng, 2013/05/30 22:38:09]

Nit: FakeURLFetcher.

[Pete Williamson, 2013/05/31 01:01:44]

Done.

+  std::string image_string(compressed.begin(), compressed.end());
+
+  // Set up a delegate to wait for the callback.
+  NotificationBitmapFetcherTestDelegate delegate(kAsyncCall);
+
+  scoped_refptr<NotificationBitmapFetcher> fetcher =
+      new NotificationBitmapFetcher(url, &delegate);
+
+  scoped_ptr<net::URLFetcher> url_fetcher(new net::FakeURLFetcher(
+      url, fetcher, image_string, kSuccess));

[dcheng, 2013/05/30 22:38:09]

This is a personal preference, but I don't think that aliasing true/false to
constants is adding to the clarity of this code. It makes it unclear what type
FakeURLFetcher takes in its constructor (bool? enum? int? string? something
else?). If a bool parameter is unclear in its context (which is often the case),
the general convention I've seen is to put its purpose in an inline comment
before or after, e.g.:
new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true)

Or something along those lines.

[Pete Williamson, 2013/05/31 00:17:39]

I think that knowing the meaning (succeeded, failed) is more important than
knowing the type in this case, I'd prefer to leave the code as is.

[dcheng, 2013/05/31 00:27:38]

Right, that's why I suggested putting it in comments. This is pretty common
throughout Chrome; defining local bool consts is much rarer and not used in this
way. Consistency is nice to have in a codebase the size of Chrome's, since
having constructs like this that are unique to your code make it that much
harder (even if only a little, it adds up) for other developers to parse.

[Pete Williamson, 2013/05/31 01:01:44]

Done.

+  fetcher->SetURLFetcherForTest(url_fetcher.Pass());
+
+  // We expect that the image decoder will get called and return
+  // an image in a callback to OnImageDecoded().
+  fetcher->Start();
+
+  // Blocks until test delegate is notified via a callback.
+  content::RunMessageLoop();
+
+  ASSERT_TRUE(delegate.success());
+
+  // Make sure we get back the bitmap we expect.
+  const SkBitmap& found_image = delegate.bitmap();
+  EXPECT_TRUE(gfx::BitmapsAreEqual(image, found_image));
+}
+
+
+IN_PROC_BROWSER_TEST_F(NotificationBitmapFetcherBrowserTest,
+                       OnImageDecodedTest) {
+  GURL url("http://localhost");
+  SkBitmap image;
+
+  // Put a real bitmap into "image".  2x2 bitmap of green 16 bit pixels.
+  image.setConfig(SkBitmap::kARGB_8888_Config, 2, 2);
+  image.allocPixels();
+  SkColor c = kMaxGreen;
+  image.eraseColor(c);
+
+  NotificationBitmapFetcherTestDelegate delegate(kSyncCall);
+
+  scoped_refptr<NotificationBitmapFetcher> fetcher =
+      new NotificationBitmapFetcher(url, &delegate);
+
+  scoped_ptr<net::URLFetcher> url_fetcher(
+      new net::TestURLFetcher(1, url, fetcher.get()));
+  fetcher->SetURLFetcherForTest(url_fetcher.Pass());
+
+  fetcher->OnImageDecoded(NULL, image);
+
+  // Ensure image is marked as succeeded.
+  EXPECT_TRUE(delegate.success());
+
+  // Test that the image is stored and ready. Pixel [0,0] should be green.
+  EXPECT_TRUE(gfx::BitmapsAreEqual(image, delegate.bitmap()));
+
+}
+
+IN_PROC_BROWSER_TEST_F(NotificationBitmapFetcherBrowserTest,
+                       OnURLFetchFailureTest) {
+  GURL url("http://localhost");
+
+  // We intentionally put no data into the bitmap to simulate a failure.
+
+  // Set up a delegate to wait for the callback.
+  NotificationBitmapFetcherTestDelegate delegate(kAsyncCall);
+
+  // The fetcher controls the lifetime of url_fetcher, but we keep a
+  // pointer to it with stub_url_fetcher.
+  scoped_refptr<NotificationBitmapFetcher> fetcher =
+      new NotificationBitmapFetcher(url, &delegate);
+
+  scoped_ptr<net::URLFetcher> url_fetcher(new net::FakeURLFetcher(
+      url, fetcher, std::string(), kFailure));
+  fetcher->SetURLFetcherForTest(url_fetcher.Pass());
+
+  fetcher->Start();
+
+  EXPECT_FALSE(delegate.success());
+}
+
+IN_PROC_BROWSER_TEST_F(NotificationBitmapFetcherBrowserTest,
+                       HandleImageFailedTest) {
+  GURL url("http://localhost");
+  NotificationBitmapFetcherTestDelegate delegate(kAsyncCall);
+  scoped_refptr<NotificationBitmapFetcher> fetcher =
+      new NotificationBitmapFetcher(url, &delegate);
+  scoped_ptr<net::URLFetcher> url_fetcher(new net::FakeURLFetcher(
+      url, fetcher, std::string("Not a real bitmap"), kSuccess));
+  fetcher->SetURLFetcherForTest(url_fetcher.Pass());
+
+  fetcher->Start();
+
+  EXPECT_FALSE(delegate.success());
+}
+
+}  // namespace notifier