Continue bitmap fetching for notifications.

chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h

+// 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.
+
+#ifndef CHROME_BROWSER_NOTIFICATIONS_SYNC_NOTIFIER_NOTIFICATION_BITMAP_FETCHER_H_
+#define CHROME_BROWSER_NOTIFICATIONS_SYNC_NOTIFIER_NOTIFICATION_BITMAP_FETCHER_H_
+
+#include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop/message_loop_proxy.h"
+#include "chrome/browser/image_decoder.h"
+#include "googleurl/src/gurl.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "third_party/skia/include/core/SKBitmap.h"
+
+namespace net {
+class URLFetcher;
+}  // namespace net
+
+class NotificationBitmapFetcherTest;
+class NotificationBitmapFetcherBrowserTest;
+
+namespace notifier {
+
+// A delegate interface for users of NotificationBitmapFetcher.
+class NotificationBitmapFetcherDelegate {
+ public:
+  // This will be called when the bitmap has been fetched, successfully or not.
+  virtual void OnFetchComplete(const bool success, SkBitmap* bitmap) = 0;

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

Generally, it's not required to add a const to integer type parameters. However,
the SkBitmap should probably be passed as a const pointer/ref.

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

Done.

+
+ protected:
+  virtual ~NotificationBitmapFetcherDelegate() {}
+};
+
+class NotificationBitmapFetcher
+    : public net::URLFetcherDelegate,
+      public ImageDecoder::Delegate,
+      public base::RefCountedThreadSafe<NotificationBitmapFetcher> {
+ public:
+        explicit NotificationBitmapFetcher(
+            const GURL& url,
+            NotificationBitmapFetcherDelegate* delegate);
+  ~NotificationBitmapFetcher();
+
+  // Start fetching the URL with the fetcher.  The operation will be continued
+  // in the OnURLFetchComplete callback.
+  void StartImageFetch();
+
+  // If a bitmap image decode fails, notify observers of the failure.
+  void HandleImageFailed();

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

This is declared but not defined anywhere. Perhaps this is leftover from an
earlier version?

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

Yep.  Removed.

+
+  // Methods inherited from URLFetcherDelegate
+
+  // This will be called when the URL has been fetched, successfully or not.
+  // Use accessor methods on |source| to get the results.
+  virtual void OnURLFetchComplete(const net::URLFetcher* source) OVERRIDE;
+
+  // This will be called when some part of the response is read. |current|
+  // denotes the number of bytes received up to the call, and |total| is the
+  // expected total size of the response (or -1 if not determined).
+  virtual void OnURLFetchDownloadProgress(const net::URLFetcher* source,
+                                          int64 current, int64 total) OVERRIDE;
+
+  // Methods inherited from ImageDecoder::Delegate
+
+  // Called when image is decoded. |decoder| is used to identify the image in
+  // case of decoding several images simultaneously.  This will not be called
+  // on the UI thread.
+  virtual void OnImageDecoded(const ImageDecoder* decoder,
+                              const SkBitmap& decoded_image) OVERRIDE;
+
+  // Called when decoding image failed.
+  virtual void OnDecodeImageFailed(const ImageDecoder* decoder) OVERRIDE;
+
+ private:
+  scoped_ptr<net::URLFetcher> url_fetcher_;
+  scoped_refptr<ImageDecoder> image_decoder_;
+  // |image_ready_| will be set to true once the image is fetched and decoded.
+  bool image_ready_;
+  GURL url_;

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

Nit: constify this.

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

Done.

+  scoped_ptr<SkBitmap> bitmap_;
+  NotificationBitmapFetcherDelegate* delegate_;

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

Nit: NotificationBitmapFetcherDelegate* const delegate_;

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

Done.

+
+  // Used to pass in a fetcher for dependency injection in tests.
+  void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher);

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

Just make this a public method. Also, is it intentional that url_fetcher is
being passed as a mutable ref to a scoped_ptr?

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

Done.

Yes, it is intentional.  We need to override the url_fetcher member variable
after construction, we can't do that if the member variable is const.

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

Yes, but why does the argument have to a mutable reference?

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

My thinking is that if this is a const reference, we will be unable to assign it
to the non-const member variable without a const_cast, which I would rather
avoid.  I would also prefer to keep the existing test even when we add a new
test for Start, I think the existing test provides value.

[dcheng, 2013/05/30 00:38:14]

Smart pointers aren't generally meant to be passed by reference or pointer. I
think this can just be passed by value.

[Pete Williamson, 2013/05/30 22:22:17]

Done.

+
+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest, ImageReadyTest);

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

Are all these friend declarations required?

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

Removed.

+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest, StartFetchTest);
+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest, OnImageDecodedTest);
+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest,
+                           OnURLFetchFailureTest);
+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest,
+                           HandleImageFailedTest);
+  FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherBrowserTest,
+                           OnURLFetchCompleteTest);
+
+
+  DISALLOW_COPY_AND_ASSIGN(NotificationBitmapFetcher);
+};
+
+}  // namespace notifier
+
+#endif  // CHROME_BROWSER_NOTIFICATIONS_SYNC_NOTIFIER_NOTIFICATION_BITMAP_FETCHER_H_