Continue bitmap fetching for notifications.

chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc

Index: chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc
diff --git a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc b/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..a9c11baa129ba9b5b27e22d5d41e7e2f623390da
--- /dev/null
+++ b/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc
@@ -0,0 +1,134 @@
+// 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 "chrome/common/chrome_notification_types.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_service.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"
+
+namespace {
+// FF0000FF is 100% alpha and max green.(A, R, B, G)
+uint32 kMaxGreen = 0xFF0000FF;
+}  // namespace
+
+namespace notifier {
+
+// Class to catch events from the NotificationBitmapFetcher for testing.
+class NotificationBitmapFetcherTestDelegate
+    : public NotificationBitmapFetcherDelegate {
+ public:
+  NotificationBitmapFetcherTestDelegate()
+      : success_(false), bitmap_(NULL) {}
+
+  ~NotificationBitmapFetcherTestDelegate() {}

[dcheng, 2013/05/28 20:11:34]

The destructor should be explicitly marked virtual.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+
+  // Method inherited from NotificationBitmapFetcherDelegate
+  void OnFetchComplete(const bool success, SkBitmap* bitmap) {

[dcheng, 2013/05/28 20:11:34]

Overridden methods should be marked virtual OVERRIDE (#include
base/compiler_specific.h for OVERRIDE).

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+    success_ = success;
+    bitmap_ = bitmap;
+    MessageLoop::current()->Quit();
+  }
+
+  bool success() { return success_; }

[dcheng, 2013/05/28 20:11:34]

const.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+  SkBitmap* bitmap() { return bitmap_; }

[dcheng, 2013/05/28 20:11:34]

Add const where appropriate.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+
+ private:
+  bool success_;
+  SkBitmap* bitmap_;

[dcheng, 2013/05/28 20:11:34]

It's probably safer to have this be SkBitmap bitmap_; to prevent potential
lifetime issues in the future.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+
+  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,
+                       OnURLFetchCompleteTest) {
+  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);
+  // Test that the image is stored and ready. Pixel [0,0] should be green.
+  EXPECT_EQ(16, image.getSize());
+  EXPECT_EQ(kMaxGreen, image.getColor(0, 0));

[dcheng, 2013/05/28 20:11:34]

These EXPECT statements seem out of place. I wouldn't expect a test to verify
input data.

[Pete Williamson, 2013/05/29 18:05:00]

Removed.  My original thinking was that this was fragile code, so I wanted to be
sure that it did what I expected it to before moving on.  Now that it reliably
does what I expect, I feel safe removing it.

+
+  // Get vital stats from the bitmap for width, height, size,  etc.
+  SkAutoLockPixels lock(image);
+  int width = image.width();
+  int height = image.height();
+  int row_length = static_cast<int>(image.rowBytes());
+  size_t size = row_length * height;
+  // Actual bitmap data in arrays of RGBAs.
+  std::vector<unsigned char> data;
+  data.resize(size);
+  memcpy(&*data.begin(), image.getAddr(0, 0), size);
+
+  // Encode the bits as a PNG.
+  std::vector<unsigned char> compressed;
+  ASSERT_TRUE(gfx::PNGCodec::Encode(&*data.begin(),

[dcheng, 2013/05/28 20:11:34]

Instead of doing this, just use gfx::PNGCodec::EncodeBGRASkBitmap.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+                                    gfx::PNGCodec::FORMAT_BGRA,
+                                    gfx::Size(width, height),
+                                    static_cast<int>(row_length),
+                                    true,
+                                    std::vector<gfx::PNGCodec::Comment>(),
+                                    &compressed));
+
+  // Copy the bits into the string, and put them into the StubURLFetcher.
+  std::string image_string;
+  image_string.resize(compressed.size());
+  // TODO(petewil): It might be better to have STL to write this loop for us
+  // with for_each or a function.  Make sure it handles embedded nulls.
+  for (size_t ii = 0; ii < compressed.size(); ++ii) {
+    image_string[ii] = compressed[ii];
+  }
+
+  // Set up a delegate to wait for the callback.
+  NotificationBitmapFetcherTestDelegate delegate;
+
+  scoped_refptr<NotificationBitmapFetcher> fetcher =
+      new NotificationBitmapFetcher(url, &delegate);
+
+  // The fetcher controls the lifetime of url_fetcher, but we keep a
+  // pointer to it with stub_url_fetcher.
+  net::TestURLFetcher* stub_url_fetcher =

[dcheng, 2013/05/28 20:11:34]

Make this a scoped_ptr.

[Pete Williamson, 2013/05/29 18:05:00]

If I do, won't it get double deleted, since url_fetcher is a scoped ptr which is
cloned from this one?  url_fetcher has to be a scoped ptr so I can pass it, but
I need to have a reference to it anyway so I can pass it to
OnURLFetchComplete().  I'm definitely open to changing this, but this is the
simplest working design that I could see.

[dcheng, 2013/05/29 20:40:29]

If the test uses Start() instead of OnURLFetchComplete(), then this won't be a
problem.

[Pete Williamson, 2013/05/30 00:13:18]

We now use Start, but it is still a problem.  It is still a problem because the
TestURLFetcher doesn't actually call the callback (OnURLFetchComplete), so I
must call it manually.

+      new net::TestURLFetcher(1, url, fetcher.get());
+  scoped_ptr<net::URLFetcher> url_fetcher(stub_url_fetcher);
+  fetcher->SetURLFetcherForTest(url_fetcher);
+  stub_url_fetcher->SetResponseString(image_string);
+
+  // We expect that the image decoder will get called and return
+  // an image in a callback to OnImageDecoded().
+  fetcher->OnURLFetchComplete(stub_url_fetcher);

[dcheng, 2013/05/28 20:11:34]

Maybe just invoke Start()

[Pete Williamson, 2013/05/29 18:05:00]

This test is not set up to be async, and Start is an async call.  In this case
the intent was to just test the OnURLFetchComplete in isolation.  While I am
moving this to a browser test which can be async, so I could call Start()
instead, I feel that I get more solid testing by calling OnURLFetchComplete
specifically, since verifying its behavior is the main point of this test.

[dcheng, 2013/05/29 20:40:29]

That means that Start() is never tested--and it might end up doing nothing.
Now that the tests are browser tests, I think all the tests should be invoking
Start(). Note that ImageDecoder isn't currently very testable; to fix that, we
should ideally have something like what URLFetcher does.

[Pete Williamson, 2013/05/30 00:13:18]

Done.

+
+  // Blocks until test delegate is notified via a callback.
+  content::RunMessageLoop();
+
+  ASSERT_TRUE(delegate.success());
+
+  // Make sure we get back the bitmap we expect.
+  SkBitmap* found_image = delegate.bitmap();

[dcheng, 2013/05/28 20:11:34]

Consider just using BitmapsAreEqual from ui/gfx/skia_util.h.

[Pete Williamson, 2013/05/29 18:05:00]

Done.

+  EXPECT_EQ(16, found_image->getSize());
+  EXPECT_EQ(kMaxGreen, found_image->getColor(0, 0));
+}
+
+}  // namespace notifier