Continue bitmap fetching for notifications.

chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc

Index: chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc
diff --git a/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc b/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2d3f5ed1607ed9903bb6d6fe149893a9070f4af8
--- /dev/null
+++ b/chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc
@@ -0,0 +1,90 @@
+// 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.
+
+#include "chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h"
+
+#include "chrome/common/chrome_notification_types.h"

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

Nit: None of the content notification headers are required any more.

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

Done.

+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/notification_details.h"
+#include "content/public/browser/notification_service.h"
+#include "net/url_request/url_fetcher.h"
+
+namespace notifier {
+
+NotificationBitmapFetcher::NotificationBitmapFetcher(
+    const GURL& url,
+    NotificationBitmapFetcherDelegate* delegate)
+    : url_(url), delegate_(delegate), image_ready_(false) {}
+
+NotificationBitmapFetcher::~NotificationBitmapFetcher() {}
+
+void NotificationBitmapFetcher::StartImageFetch() {
+  if (url_fetcher_ == NULL)
+    url_fetcher_.reset(
+        net::URLFetcher::Create(url_, net::URLFetcher::GET, this));
+  url_fetcher_->Start();
+}
+
+void NotificationBitmapFetcher::SetURLFetcherForTest(
+    scoped_ptr<net::URLFetcher>& url_fetcher) {
+  url_fetcher_ = url_fetcher.Pass();
+}
+
+// Methods inherited from URLFetcherDelegate.
+
+void NotificationBitmapFetcher::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  std::string image_data;
+
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Copy the data into the string.  Keep in mind it may have embedded nulls.
+  source->GetResponseAsString(&image_data);
+
+  // Handle fetch failure.  If it failed, set failed to true, and fire
+  // notification to listeners.
+  if (image_data.length() == 0) {
+    OnDecodeImageFailed(NULL);
+  }
+
+  // Create an ImageDecoder with the data and assign it to the refptr.
+  image_decoder_ = 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) {
+  // Do nothing here.
+}
+
+// Methods inherited from ImageDecoder::Delegate.
+
+// This comes in on another thread, so we send the bitmap back to the UI
+// thread so that the object stays thread safe.
+void NotificationBitmapFetcher::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());

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

Is it still necessary to make a copy of the bitmap?

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

I think so.  I don't know that ImageDecoder provides us any lifetime guarantees
of the bitmap, and copying it is the only way I can see to make sure the bits
are still valid later when we try to use them.

Looking at the source code, ImageDecoder gets a callback when the image decoding
is done with a bitmap reference on the callback, and it just passes along that
reference to me.  That implies that the data is coming from another thread
asynchronously, and there is no guarantee that the other thread will keep the
data alive longer than the span of the callback.

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

Can we just use SkBitmap's operator=?

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

I tried this without the additional call to deep copy, and it failed for me. 
operator= makes a shallow copy, which works well if something is holding the
bits in memory until you need them.  However, in this case, that object is in
another process and there are no guarantees that I can find that it will always
keep the bitmap as long as I want it.  I took a look at the code, and found that
operator= just does a memcpy of the skbitmap object, including the pointer to
the allocated memory where the pixels are stored, which did not appear to me to
be a shared_ptr.

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

I see. I misinterpreted the comment (which remarks that the pixels are shared)
but not catch the part which describes that ownership remains with src. Thanks
for investigating this.

+  decoded_image.deepCopyTo(bitmap_.get(), decoded_image.getConfig());
+  image_ready_ = true;
+
+  // Report success.
+  delegate_->OnFetchComplete(image_ready_, bitmap_.get());

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

Rather than using image_ready_, is it possible to either:
- Have the delegate have two callbacks (OnFetchSucceeeded, OnFetchFailed)
- Or just use the fact that the bitmap will be NULL if it failed.

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

Done (using NULL)

+}
+
+void NotificationBitmapFetcher::OnDecodeImageFailed(
+    const ImageDecoder* decoder) {
+
+  // Report failure.
+  delegate_->OnFetchComplete(image_ready_, NULL);
+}
+
+}  // namespace notifier