Repurpose NotificationBitmapFetcher to BitmapFetcher

chrome/browser/bitmap_fetcher.h

Index: chrome/browser/bitmap_fetcher.h
diff --git a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h b/chrome/browser/bitmap_fetcher.h
similarity index 70%
rename from chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h
rename to chrome/browser/bitmap_fetcher.h
index 7a268e6a22d2c4a59488dfaee85221bcaf6af659..6320caddfe31a66631415cfdd2bae40ee6ef22b0 100644
--- a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h
+++ b/chrome/browser/bitmap_fetcher.h
@@ -2,8 +2,8 @@
 // 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_
+#ifndef CHROME_BROWSER_BITMAP_FETCHER_H_
+#define CHROME_BROWSER_BITMAP_FETCHER_H_
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/image_decoder.h"
@@ -17,10 +17,10 @@ class URLFetcher;
 
 class Profile;
 
-namespace notifier {
+namespace chrome {
 
-// A delegate interface for users of NotificationBitmapFetcher.
-class NotificationBitmapFetcherDelegate {
+// A delegate interface for users of BitmapFetcher.
+class BitmapFetcherDelegate {

[sky, 2014/02/13 18:55:05]

nit: move into its own file.

[gone, 2014/02/13 23:39:38]

Done.

  public:
   // This will be called when the bitmap has been requested, whether or not the
   // request succeeds.  |url| is the URL that was originally fetched so we can
@@ -28,17 +28,14 @@ class NotificationBitmapFetcherDelegate {
   virtual void OnFetchComplete(const GURL url, const SkBitmap* bitmap) = 0;
 
  protected:
-  virtual ~NotificationBitmapFetcherDelegate() {}
+  virtual ~BitmapFetcherDelegate() {}
 };
 
-class NotificationBitmapFetcher
-    : public net::URLFetcherDelegate,
-      public ImageDecoder::Delegate {
+class BitmapFetcher : public net::URLFetcherDelegate,

[sky, 2014/02/13 18:55:05]

Description?

[gone, 2014/02/13 23:39:38]

Done.

+                      public ImageDecoder::Delegate {
  public:
-  NotificationBitmapFetcher(
-      const GURL& url,
-      NotificationBitmapFetcherDelegate* delegate);
-  virtual ~NotificationBitmapFetcher();
+  BitmapFetcher(const GURL& url, BitmapFetcherDelegate* delegate);
+  virtual ~BitmapFetcher();
 
   GURL url() const { return url_; }

[sky, 2014/02/13 18:55:05]

const GURL&

[gone, 2014/02/13 23:39:38]

Done.

 
@@ -56,7 +53,8 @@ class NotificationBitmapFetcher
   // 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;
+                                          int64 current,
+                                          int64 total) OVERRIDE;
 
   // Methods inherited from ImageDecoder::Delegate
 
@@ -74,11 +72,11 @@ class NotificationBitmapFetcher
   scoped_refptr<ImageDecoder> image_decoder_;
   const GURL url_;
   scoped_ptr<SkBitmap> bitmap_;

[sky, 2014/02/13 18:55:05]

Why do we need this as a member? Why not declare it on the stack when needed?

[Pete Williamson, 2014/02/13 21:30:31]

The intent was to use this object to both fetch and store the bitmap - once it
was fetched, I wanted to come back to this object and retrieve it so I did not
need to make another copy.

If you think that is poor design, it wouldn't hurt the process to get rid of
this member variable and just make copies (though it may degrade perf a bit).

The general idea is one that I borrowed from the Win7 TCP/IP stack - don't copy
the data more than necessary, and never if you can avoid it.

[sky, 2014/02/13 23:12:30]

When you are not even exposing the bitmap publicly in this class  (there is no
getter for this) something tells me you are not actually using a design like you
outline:)

In general I prefer classes as stateless as possible. If the delegate wants to
keep around the bitmap, they should copy it.

As to the expense of copying skbitmaps, they are designed such that copy is
cheap and explicitly support copy semantics. I would have pushed the delegate to
take a const SkBitmap&, but you want NULL to be meaningful so it can't do that.

[Pete Williamson, 2014/02/13 23:27:06]

My bad, I think I was remembering another part of the design elsewhere.  In that
case, getting rid of the member function is fine.

[gone, 2014/02/13 23:39:38]

Done.

-  NotificationBitmapFetcherDelegate* const delegate_;
+  BitmapFetcherDelegate* const delegate_;
 
-  DISALLOW_COPY_AND_ASSIGN(NotificationBitmapFetcher);
+  DISALLOW_COPY_AND_ASSIGN(BitmapFetcher);
 };
 
-}  // namespace notifier
+}  // namespace chrome
 
-#endif  // CHROME_BROWSER_NOTIFICATIONS_SYNC_NOTIFIER_NOTIFICATION_BITMAP_FETCHER_H_
+#endif  // CHROME_BROWSER_BITMAP_FETCHER_H_