Repurpose NotificationBitmapFetcher to BitmapFetcher

chrome/browser/bitmap_fetcher.cc

 // 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.
 
-#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
   // no cookies.
   url_fetcher_->SetRequestContext(profile->GetRequestContext());
   url_fetcher_->Start();
 }
 
 // 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) {
     OnDecodeImageFailed(NULL);

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

Less error prone if OnDecodeImageFailed and this call into a common method that
doesn't take a ImageDecoder.

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

Done.

     return;
   }
 
   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.
   scoped_refptr<base::MessageLoopProxy> task_runner =
       content::BrowserThread::GetMessageLoopProxyForThread(
           content::BrowserThread::UI);
   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());
 
   // Report success.
   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