[Suggestions] Adding a Thumbnail Manager for the Suggestions Service

chrome/browser/search/suggestions/thumbnail_manager.cc

+// 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 "net/base/load_flags.h"
+
+namespace suggestions {
+
+ThumbnailManager::ThumbnailManager(Profile* profile)
+  : profile_(profile), url_request_context_(profile->GetRequestContext()) {}
+
+ThumbnailManager::~ThumbnailManager() {}
+
+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()) {

[huangs, 2014/05/22 06:17:35]

What if there are multiple suggestions with the same URL?  Also, some may have
thumbnails and some may not.

[Mathieu, 2014/05/22 15:45:51]

I feel like the server should worry about this. In our current version it is not
possible. I'm comfortable with undefined behavior on the client (wrt which would
get a thumbnail), but thanks for bringing it up.

+      thumbnail_map_[GURL(suggestion.url())] = GURL(suggestion.thumbnail());
+    }
+  }
+}
+
+void ThumbnailManager::GetURLThumbnail(const GURL& url,
+                                       BitmapResponseCallback callback) {
+  // If the |url| is not in the |thumbnail_map_|, there is no associated
+  // thumbnail for it. Will callback with an empty bitmap.
+  GURL thumbnail_url;
+  if (!GetThumbnailURL(url, &thumbnail_url)) {
+    callback.Run(url, NULL);
+    return;
+  }
+
+  // If a fetch of the |thumbnail_url| is already in progress, register as an
+  // interested callback. Will allocate a new queue if not present.
+  pending_callbacks_[thumbnail_url].push(callback);
+
+  // If we are the first ones requesting this |thumbnail_url|, start the fetch.
+  if (pending_callbacks_[thumbnail_url].size() == 1) {

[huangs, 2014/05/22 06:17:35]

I'd prefer using queue<>::empty() instead of checking for size == 1.  How about:

// Allocates a new queue if none exists.
std::queue<BitmapResponseCallback>* pending_callback =
    &pending_callbacks_[thumbnail_url];

// Fetch request if this is no in-flight request for |thumbnail_url|.
bool fetch_required = pending_callback->empty();

// Enqueue |callback| before fetching to be robust against race condition.
pending_callback->push(callback);

if (fetch_required) {
  ...
}

[Mathieu, 2014/05/22 15:45:51]

Reworked this, let me know

+    chrome::BitmapFetcher* fetcher =
+        new chrome::BitmapFetcher(thumbnail_url, this);
+    fetcher_map_[thumbnail_url] = std::make_pair(url, fetcher);
+    fetcher->Start(
+        url_request_context_, std::string(),

[huangs, 2014/05/22 06:17:35]

NIT: 1 parameter per line.

[Mathieu, 2014/05/22 15:45:51]

clang-format would disagree!

+        net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,
+        net::LOAD_NORMAL);
+  }
+}
+
+void ThumbnailManager::OnFetchComplete(const GURL thumbnail_url,
+                                       const SkBitmap* bitmap) {
+  // Make sure the BitmapFetcher gets destroyed.
+  scoped_ptr<chrome::BitmapFetcher> fetcher_destructor(

[huangs, 2014/05/22 06:17:35]

You only need to do one lookup if you store the iterator:

std::map<GURL, std::pair<GURL, chrome::BitmapFetcher*>>::iterator it =
    fetcher_map_.find(thumbnail_url);
// Make sure the BitmapFetcher gets destroyed.
scoped_ptr<chrome::BitmapFetcher> fetcher_destructor(it->second);
GURL url(it->first);
fetcher_map_.erase(it);

[Mathieu, 2014/05/22 15:45:51]

Done. Have a look!

+      fetcher_map_[thumbnail_url].second);
+  GURL url(fetcher_map_[thumbnail_url].first);
+  fetcher_map_.erase(fetcher_map_.find(thumbnail_url));
+
+  // 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 =

[huangs, 2014/05/22 06:17:35]

I think it's cleaner to just swap data data:

std::queue<BitmapResponseCallback> callbacks;
callbacks.swap(pending_callbacks_[thumbnail_url]);

I'm not sure if this works on all platform though.  http://www.cplusplus.com/
says this is C++11...

[Mathieu, 2014/05/22 15:45:51]

Done.

+      pending_callbacks_[thumbnail_url];
+  while (!callbacks.empty()) {
+    callbacks.front().Run(url, bitmap);
+    callbacks.pop();
+  }
+  pending_callbacks_.erase(pending_callbacks_.find(thumbnail_url));

[huangs, 2014/05/22 06:17:35]

You can just direct erase with the key:

pending_callbacks_.erase(thumbnail_url);

[Mathieu, 2014/05/22 15:45:51]

Have a look at how it is now?

+}
+
+bool ThumbnailManager::GetThumbnailURL(const GURL& url,
+                                       GURL* thumbnail_url) {
+  std::map<GURL, GURL>::iterator it = thumbnail_map_.find(url);
+  if (it == thumbnail_map_.end()) {
+    // Not found.
+    return false;
+  }
+  *thumbnail_url = it->second;
+  return true;
+}
+
+}  // namespace suggestions