[Suggestions] Adding a Thumbnail Manager for the Suggestions Service

chrome/browser/search/suggestions/thumbnail_manager.cc

Index: chrome/browser/search/suggestions/thumbnail_manager.cc
diff --git a/chrome/browser/search/suggestions/thumbnail_manager.cc b/chrome/browser/search/suggestions/thumbnail_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..8f2425db5d46c43cce93dad345ffc61367815aab
--- /dev/null
+++ b/chrome/browser/search/suggestions/thumbnail_manager.cc
@@ -0,0 +1,96 @@
+// Copyright 2014 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/search/suggestions/thumbnail_manager.h"
+
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/suggestions/proto/suggestions.pb.h"
+#include "content/public/browser/browser_thread.h"
+#include "net/base/load_flags.h"
+
+namespace suggestions {
+
+ThumbnailManager::ThumbnailManager(Profile* profile)
+    : url_request_context_(profile->GetRequestContext()) {}
+
+ThumbnailManager::~ThumbnailManager() {}
+
+ThumbnailManager::Session::Session() {}

[Jered, 2014/05/23 16:57:48]

Initialize the struct fields.

[huangs, 2014/05/23 17:23:35]

Per discussion, can remove ThumbnailManager::Session consturctor and destructor.

[Mathieu, 2014/05/23 19:21:58]

Actually removed this.

[Mathieu, 2014/05/23 19:21:58]

Done.

+
+ThumbnailManager::Session::~Session() {}
+
+void ThumbnailManager::InitializeThumbnailMap(
+    const SuggestionsProfile& suggestions) {
+  thumbnail_map_.clear();
+  for (int i = 0; i < suggestions.suggestions_size(); ++i) {
+    const ChromeSuggestion& suggestion = suggestions.suggestions(i);
+    if (suggestion.has_thumbnail()) {
+      thumbnail_map_[GURL(suggestion.url())] = GURL(suggestion.thumbnail());
+    }
+  }
+}
+
+void ThumbnailManager::GetPageThumbnail(const GURL& url,
+                                        BitmapResponseCallback callback) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  // If |url| is not found in |thumbnail_map_|, then invoke |callback| with NULL
+  // since there is no associated thumbnail.
+  GURL thumbnail_url;
+  if (!GetThumbnailURL(url, &thumbnail_url)) {
+    callback.Run(url, NULL);
+    return;
+  }
+
+  // Look for a fetch in progress for |thumbnail_url|.
+  ThumbnailSessionMap::iterator it = session_map_.find(thumbnail_url);
+  if (it == session_map_.end()) {
+    // |thumbnail_url| is not being fetched, so create a session and initiate
+    // the fetch.
+    Session* session = &session_map_[thumbnail_url];

[Jered, 2014/05/23 16:57:48]

A different approach here that would help you to clarify the ownership semantics
of session->fetcher would be: Session owns the BitmapFetcher.
~Session would destroy it. You'd add a Session(chrome::BitmapFetcher*)
constructor. Here, you'd write

Session request(new chrome::BitmapFetcher(thumbnail_url, this));
request->callbacks.push(callback);
request->fetcher->Start(...);
session_map_[thumbnail_url] = request;

[Mathieu, 2014/05/23 19:21:58]

I actually like the approach Sam and I had come up with. Having the map create
the default instance and initializing it manually avoids a copy. Since it's a
private struct it's pretty safe to manage deallocation ourselves. Let me know if
you feel strongly.

[huangs, 2014/05/23 20:16:22]

To Jered: Since we use std::map<>, the suggested change would require
BitmapFetcher to have a default constructor anyway.  And if ~Session destroys
fetcher, then we would need

std::swap(session_map_[thumbnail_url], request);

[Jered, 2014/05/23 21:25:00]

A default constructor seems reasonable. map.erase() calls the destructor for
class types.

[Jered, 2014/05/23 21:25:00]

This is one of my pet peeves in C++... correctness is more important than
eliding copies of small objects, that's premature optimization. Here, the
correctness question is who owns the fetcher pointer? It's tied to the lifetime
of the thumbnail request, and there's an object for that. With RAII, we should
tie the ownership to the lifecycle of the object. so
struct ThumbnailRequest {
  ThumbnailRequest(chrome::BitmapFetcher* f)
    : fetcher(f) {}
  scoped_ptr<chrome::BitmapFetcher*> fetcher;
};

This is nice because you don't have to "remember" to destroy the pointer later.

[huangs, 2014/05/23 22:34:26]

We tried this, but std::map<GURL, ThumbnailSessionMap> will need to copy (no
C++11 emplace() yet), and then scope_ptr<> complains.

What we ultimately want is to have std::map<T, scoped_ptr<U> >, but this is
unavailable...

[Jered, 2014/05/23 22:37:01]

Even if we can't use scoped_ptr, a raw pointer with a destructor that calls
delete still seems clearer to me. :-)

[Mathieu, 2014/05/23 22:40:25]

Indeed we tried this along with many variations (Sam means std::map<GURL,
ThumbnailRequest>) and it didn't work. The only sensible way is to have the
fetcher as a pointer, which is low-risk being private. 

Use scoped_ptr<> in a map and you're going to have a bad day... I'm interested
in doing it the right way but we just can't seem to find it.

[Mathieu, 2014/05/23 22:40:25]

Ack.

[Mathieu, 2014/05/23 22:54:58]

Have a look at the latest version. Had to leave the default constructor on
ThumbnailRequest for it to compile, and thus put a guard on the deletion of
fetcher. Not sure if that's too much. Let me know.

+    session->url = url;
+    session->callbacks.push(callback);
+    // This is deallocated in OnFetchComplete().
+    session->fetcher = new chrome::BitmapFetcher(thumbnail_url, this);
+    session->fetcher->Start(
+        url_request_context_, std::string(),
+        net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,
+        net::LOAD_NORMAL);
+  } else {
+    // Fetch in progress. Register as an interested callback.
+    it->second.callbacks.push(callback);
+  }
+}
+
+void ThumbnailManager::OnFetchComplete(const GURL thumbnail_url,
+                                       const SkBitmap* bitmap) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  ThumbnailSessionMap::iterator it = session_map_.find(thumbnail_url);
+  DCHECK(it != session_map_.end());
+
+  Session* session = &it->second;
+
+  // Make sure the |fetcher| is deallocated.
+  scoped_ptr<chrome::BitmapFetcher> fetcher_destructor(session->fetcher);
+  // Here |bitmap| could be NULL or a pointer to a bitmap which is owned by the
+  // BitmapFetcher and which ceases to exist after this function. Pass the
+  // un-owned pointer to the registered callbacks.
+  std::queue<BitmapResponseCallback> callbacks = session->callbacks;

[huangs, 2014/05/23 17:23:35]

Comment that you're purposefully creating a copy of session->callbacks.

Actually, this is mainly useful for thread safety; the idea is that you would
lock |session_map_|, copy the stuff you need, clear from |session_map_|, unlock,
then loop over the copy and dispatch at the current thread, without blocking the
other thread that writes to |session_map_|.

Since you're always on the UI thread, an alternative (to commenting) is that you
can just operate on session->callbacks directly.

[Mathieu, 2014/05/23 19:21:58]

No copying sounds good since we're on the UI thread, thanks.

+  while (!callbacks.empty()) {
+    callbacks.front().Run(session->url, bitmap);
+    callbacks.pop();
+  }
+  session_map_.erase(it);
+}
+
+bool ThumbnailManager::GetThumbnailURL(const GURL& url, GURL* thumbnail_url) {
+  std::map<GURL, GURL>::iterator it = thumbnail_map_.find(url);
+  if (it == thumbnail_map_.end())
+    return false;  // Not found.
+  *thumbnail_url = it->second;
+  return true;
+}
+
+}  // namespace suggestions