[Suggestions] Adding a Thumbnail Manager for the Suggestions Service

chrome/browser/search/suggestions/thumbnail_manager.h

+// 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.
+
+#ifndef CHROME_BROWSER_SEARCH_SUGGESTIONS_THUMBNAIL_MANAGER_H_
+#define CHROME_BROWSER_SEARCH_SUGGESTIONS_THUMBNAIL_MANAGER_H_
+
+#include <memory>

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

#include <map>

#include <utility>    (for std::pair)

Probably don't need <memory> and <vector>.

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

Done.

+#include <queue>
+#include <vector>
+
+#include "base/basictypes.h"
+#include "base/callback.h"
+#include "chrome/browser/bitmap_fetcher.h"
+#include "ui/gfx/image/image_skia.h"
+#include "url/gurl.h"
+
+class Profile;
+
+namespace suggestions {
+
+class SuggestionsProfile;
+
+// An interface to maintain and fetch server thumbnails asynchronously.

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

NIT: "base class" instead of interface?  To me interface sounds like a class
whose member functions are all virtual, and has no member variables.

This class is not thread-safe.  Perhaps state on which thread is this meant to
be run?

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

I stated it below in a comment to GetPageThumbnail (previously GetURLThumbnail)

+class ThumbnailManager : public chrome::BitmapFetcherDelegate {
+ public:
+  typedef base::Callback<void(const GURL&, const SkBitmap*)>
+      BitmapResponseCallback;
+
+  explicit ThumbnailManager(Profile* profile);
+  virtual ~ThumbnailManager();
+
+  // Initialize the |thumbnail_map_| with the proper mapping from URL to
+  // thumbnail URL.
+  void InitializeThumbnailMap(const SuggestionsProfile& suggestions);
+
+  // For a given |url|, if a thumbnail URL is known for it, will start the fetch

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

Suggested comment:

// Retrieves stored thumbnail for |url| asynchronously. Returns thumbnail image
a Bitmap pointer to |callback| if found, and NULL otherwise.

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

Done.

+  // of the image and |callback|. In the case of a |url| for which the thumbnail
+  // is  not known, the bitmap pointer given to the |callback| will be NULL.
+  void GetURLThumbnail(const GURL& url, BitmapResponseCallback callback);
+
+  // Inherited from BitmapFetcherDelegate.
+  virtual void OnFetchComplete(const GURL thumbnail_url,
+                               const SkBitmap* bitmap) OVERRIDE;
+
+ private:
+  FRIEND_TEST_ALL_PREFIXES(ThumbnailManagerTest, InitializeThumbnailMapTest);
+  FRIEND_TEST_ALL_PREFIXES(ThumbnailManagerBrowserTest, FetchThumbnails);
+  FRIEND_TEST_ALL_PREFIXES(ThumbnailManagerBrowserTest, FetchThumbnailsInvalid);
+  FRIEND_TEST_ALL_PREFIXES(ThumbnailManagerBrowserTest,
+                           FetchThumbnailsMultiple);
+
+  // Given a website |url|, fill in a |thumbnail_url| and return whether it
+  // exists or not.
+  bool GetThumbnailURL(const GURL& url, GURL* thumbnail_url);
+
+  // Used for substituting the request context during testing.
+  void set_request_context(net::URLRequestContextGetter* context) {
+    url_request_context_ = context;
+  }
+
+  // Map from URL to thumbnail URL. Should be kept up to date when a new
+  // SuggestionsProfile is available.
+  std::map<GURL, GURL> thumbnail_map_;
+
+  // Map from each thumbnail URL to a queue of pending callbacks.

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

Both |pending_callbacks_| and |fetcher_map_| use the same keys.

Perhaps combine the two structures, and define:

class ThumbnailManagerSession {
 public:
  GURL url;
  scoped_ptr<chrome::BitmapFetcher> fetcher;
  std::queue<BitmapResponseCallback> callbacks;

 private:
  DISALLOW_COPY_AND_ASSIGN(ThumbnailManagerSession);
};

then have

std::map<GURL, ThumbnailManagerSession> session_map_;

Another advantage is that deallocation of fetcher is automatic on destruction of
this class, and when you do

session_map_.erase(url);

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

Done.

+  std::map<GURL, std::queue<BitmapResponseCallback>> pending_callbacks_;
+
+  // Map from each thumbnail URL to a pair composed of
+  // (website URL, bitmap fetcher).
+  std::map<GURL, std::pair<GURL, chrome::BitmapFetcher*>> fetcher_map_;
+
+  Profile* profile_;

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

|profile_| seems to be unused?

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

Done.

+
+  net::URLRequestContextGetter* url_request_context_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThumbnailManager);
+};
+
+}  // namespace suggestions
+
+#endif  // CHROME_BROWSER_SEARCH_SUGGESTIONS_THUMBNAIL_MANAGER_H_