Repurpose NotificationBitmapFetcher to BitmapFetcher

chrome/browser/bitmap_fetcher.h

 // Copyright 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_
+#ifndef CHROME_BROWSER_BITMAP_FETCHER_H_
+#define CHROME_BROWSER_BITMAP_FETCHER_H_
 
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/image_decoder.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "url/gurl.h"
 
 namespace net {
 class URLFetcher;
 }  // namespace net
 
 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
   // match up the bitmap with a specific request.

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

Document that bitmap may be NULL for either failure of download or failure to
decode.

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

Done.

   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.

 
   // Start fetching the URL with the fetcher.  The operation will be continued

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

Will be continued?

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

The idea here in the original code this came from was that we do the URL fetch
on one thread, and the callback for the URLFetch then calls the image decode on
a browser thread (in a browser process).  Since that callback implementation is
not in this source file, it might have been hard to tell.  We should move the
callback implementation into this file.

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

My comment was more that particular sentence is confusing and didn't really help
things. I personally like: 
"Start fetching the URL with the fetcher. The delegate is notified
asynchronously when done."

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

OK, I agree that the new wording would be good.

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

Done.

   // in the OnURLFetchComplete callback.
   void Start(Profile* profile);
 
   // 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;
+                                          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_;
   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_