[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 <map>
+#include <queue>
+#include <utility>
+
+#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 {
+
+typedef base::Callback<void(const GURL&, const SkBitmap*)>
+    BitmapResponseCallback;
+
+class SuggestionsProfile;
+

[huangs, 2014/05/22 18:08:22]

NIT: remove extra empty lines.

[Mathieu, 2014/05/22 19:46:32]

Done.

+
+
+// A base class to maintain and fetch server thumbnails asynchronously.
+class ThumbnailManager : public chrome::BitmapFetcherDelegate {
+ public:
+  explicit ThumbnailManager(Profile* profile);
+  virtual ~ThumbnailManager();
+
+  // Initialize the |thumbnail_map_| with the proper mapping from URL to

[huangs, 2014/05/22 18:08:22]

NIT: s/Initialize/Initializes/

[Mathieu, 2014/05/22 19:46:32]

Done.

+  // thumbnail URL.
+  void InitializeThumbnailMap(const SuggestionsProfile& suggestions);

[huangs, 2014/05/22 18:08:22]

Should this routine only be called once? What if it's called while there are
pending requests?

[Mathieu, 2014/05/22 19:46:32]

It can be updated when new suggestions are fetched. The thumbnail_map_ is only
accessed when starting requests, so it's not affected by pending requests. The
worst that can happen if it's being refreshed while a request is being prepared
is that the thumbnail URL is not found.

+
+  // Retrieves stored thumbnail for website |url| asynchronously. Returns
+  // thumbnail image as a Bitmap pointer to |callback| if found, and NULL
+  // otherwise. Calls should originate from the UI thread.
+  void GetPageThumbnail(const GURL& url, BitmapResponseCallback callback);
+
+  // Inherited from BitmapFetcherDelegate.
+  virtual void OnFetchComplete(const GURL thumbnail_url,

[huangs, 2014/05/22 18:08:22]

I suspect this is not called from the UI thread?  If this were the case, then
GetPageThumbnail() and OnFetchComplete() may conflict over |session_map_|.  To
solve this, you can add Mutex to safeguard |session_map_|.

[Mathieu, 2014/05/22 19:46:32]

Done.

+                               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);
+
+  // Contains the information related with a thumbnail fetch (fetcher, pending

[huangs, 2014/05/22 18:08:22]

also mention |url|?

[Mathieu, 2014/05/22 19:46:32]

Done.

+  // callbacks).
+  class Session {

[huangs, 2014/05/22 18:08:22]

Since Session doesn't do anything interesting anymore, may as well make it a
struct, and get rid of consturctors / destructors.

Also, public member variables don't have to end with "_".

[Mathieu, 2014/05/22 19:46:32]

I can't get rid of the constructors, nor inline them, etc.

+   public:
+    Session();
+    ~Session();
+
+    GURL url_;
+    chrome::BitmapFetcher* fetcher_;
+    std::queue<BitmapResponseCallback> callbacks_;

[huangs, 2014/05/22 18:08:22]

// Queue for pending callbacks, which may accumulate while the request is in
flight.

[Mathieu, 2014/05/22 19:46:32]

Done.

+  };
+
+  typedef std::map<const GURL, Session> ThumbnailSessionMap;
+
+  // Given a website |url|, fill in a |thumbnail_url| and return whether it

[huangs, 2014/05/22 18:08:22]

// Looks up thumbnail for |url|. If found, writes the result to |thumbnail_url|
and returns true. Otherwise just returns false.

[Mathieu, 2014/05/22 19:46:32]

Done.

+  // 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 the session information (fetcher,

[huangs, 2014/05/22 18:08:22]

url is also part of session information.

[Mathieu, 2014/05/22 19:46:32]

Done.

+  // pending callbacks).
+  ThumbnailSessionMap session_map_;
+
+  net::URLRequestContextGetter* url_request_context_;
+
+  DISALLOW_COPY_AND_ASSIGN(ThumbnailManager);
+};
+
+}  // namespace suggestions
+
+#endif  // CHROME_BROWSER_SEARCH_SUGGESTIONS_THUMBNAIL_MANAGER_H_