Repurpose NotificationBitmapFetcher to BitmapFetcher

chrome/browser/bitmap_fetcher.cc

Index: chrome/browser/bitmap_fetcher.cc
diff --git a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc b/chrome/browser/bitmap_fetcher.cc
similarity index 64%
rename from chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc
rename to chrome/browser/bitmap_fetcher.cc
index b4b50fa4ef6969520a4a6be6a708344bea7214e4..d8d5e53502cdaee7067808f7a43d001a2b481d45 100644
--- a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc
+++ b/chrome/browser/bitmap_fetcher.cc
@@ -2,25 +2,22 @@
 // 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/browser/bitmap_fetcher.h"
 
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_status.h"
 
-namespace notifier {
+namespace chrome {
 
-NotificationBitmapFetcher::NotificationBitmapFetcher(
-    const GURL& url,
-    NotificationBitmapFetcherDelegate* delegate)
+BitmapFetcher::BitmapFetcher(const GURL& url, BitmapFetcherDelegate* delegate)
     : url_(url), delegate_(delegate) {}

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

nit: since everything doesn't fit on one line, wrap each arg to its own line.

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

Done.

 
-NotificationBitmapFetcher::~NotificationBitmapFetcher() {}
+BitmapFetcher::~BitmapFetcher() {}
 
-void NotificationBitmapFetcher::Start(Profile* profile) {
-  url_fetcher_.reset(
-      net::URLFetcher::Create(url_, net::URLFetcher::GET, this));
+void BitmapFetcher::Start(Profile* profile) {
+  url_fetcher_.reset(net::URLFetcher::Create(url_, net::URLFetcher::GET, this));

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

I believe the expectation is this is only invoked once. So, add a DCHECK that
url_fetcher_ is NULL.

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

Done.

   // The RequestContext is coming from the current profile.
   // TODO(petewil): Make sure this is the right profile to use.

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

Is this comment still relevant? Seems obvious that since a profile is supplied
it's relevant to the profile.

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

It is OK to remove this comment.  The feature is multi-profile, and the comment
was to make sure we aren't fetching a bitmap for notifications for one profile
from a different profile.  As Sky points out, we are using a profile here, so
that should no longer be a concern.

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

Done.

   // It seems to work, but we might prefer to use a blank profile with
@@ -31,8 +28,7 @@ void NotificationBitmapFetcher::Start(Profile* profile) {
 
 // Methods inherited from URLFetcherDelegate.
 
-void NotificationBitmapFetcher::OnURLFetchComplete(
-    const net::URLFetcher* source) {
+void BitmapFetcher::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   if (source->GetStatus().status() != net::URLRequestStatus::SUCCESS) {
@@ -43,8 +39,8 @@ void NotificationBitmapFetcher::OnURLFetchComplete(
   std::string image_data;
   source->GetResponseAsString(&image_data);
   // Create an ImageDecoder with the data and assign it to the refptr.

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

This comment just describes the code and so isn't particularly helpful.

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

Done.

-  image_decoder_ = new ImageDecoder(this, image_data,
-                                    ImageDecoder::DEFAULT_CODEC);
+  image_decoder_ =

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

I thought we decoded images in the renderer for two reasons. If there is a
security exploit in a decoder it is isolated to the sandboxed renderer.
Similarly if decoding crashes its only a renderer. I realize you don't have a
renderer here, so it's worth pinging the security folks about this.

[gone, 2014/02/13 19:28:31]

Doesn't the ImageDecoder run in a sandboxed process?  At least its header
suggests it.

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

By all means check with security.

We do in fact run on a browser thread in the calling code:
  // Call start to begin decoding.  The ImageDecoder will call OnImageDecoded
  // with the data when it is done.
  scoped_refptr<base::MessageLoopProxy> task_runner =
      content::BrowserThread::GetMessageLoopProxyForThread(
          content::BrowserThread::UI);
  image_decoder_->Start(task_runner);

If we are making this class more general, it might be nice to move the logic to
call this in the browser process into this new, more general class to reduce the
potential for abuse.

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

Should be fine, according to a reply later on.

+      new ImageDecoder(this, image_data, ImageDecoder::DEFAULT_CODEC);
 
   // Call start to begin decoding.  The ImageDecoder will call OnImageDecoded
   // with the data when it is done.
@@ -54,15 +50,16 @@ void NotificationBitmapFetcher::OnURLFetchComplete(
   image_decoder_->Start(task_runner);
 }
 
-void NotificationBitmapFetcher::OnURLFetchDownloadProgress(
-    const net::URLFetcher* source, int64 current, int64 total) {
+void BitmapFetcher::OnURLFetchDownloadProgress(const net::URLFetcher* source,
+                                               int64 current,
+                                               int64 total) {
   // Do nothing here.
 }
 
 // Methods inherited from ImageDecoder::Delegate.
 
-void NotificationBitmapFetcher::OnImageDecoded(
-    const ImageDecoder* decoder, const SkBitmap& decoded_image) {
+void BitmapFetcher::OnImageDecoded(const ImageDecoder* decoder,
+                                   const SkBitmap& decoded_image) {
   // Make a copy of the bitmap which we pass back to the UI thread.
   bitmap_.reset(new SkBitmap());
   decoded_image.deepCopyTo(bitmap_.get(), decoded_image.getConfig());
@@ -71,11 +68,9 @@ void NotificationBitmapFetcher::OnImageDecoded(
   delegate_->OnFetchComplete(url_, bitmap_.get());
 }
 
-void NotificationBitmapFetcher::OnDecodeImageFailed(
-    const ImageDecoder* decoder) {
-
+void BitmapFetcher::OnDecodeImageFailed(const ImageDecoder* decoder) {
   // Report failure.
   delegate_->OnFetchComplete(url_, NULL);
 }
 
-}  // namespace notifier
+}  // namespace chrome