[AiS] ImageService

chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h

Index: chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h
diff --git a/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h b/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h
new file mode 100644
index 0000000000000000000000000000000000000000..ebc92e778c9898b9539d53eb0b5e3e0f1a2122ae
--- /dev/null
+++ b/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.h
@@ -0,0 +1,99 @@
+// 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_BITMAP_FETCHER_BITMAP_FETCHER_SERVICE_H_
+#define CHROME_BROWSER_BITMAP_FETCHER_BITMAP_FETCHER_SERVICE_H_
+
+#include "base/compiler_specific.h"
+#include "base/containers/mru_cache.h"
+#include "base/containers/scoped_ptr_hash_map.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
+#include "chrome/browser/bitmap_fetcher_delegate.h"
+#include "components/keyed_service/core/keyed_service.h"
+
+namespace content {
+class BrowserContext;
+}  // namespace content
+
+namespace chrome {
+class BitmapFetcher;
+}  // namespace chrome
+
+class BitmapFetcherRequest;
+class GURL;
+class SkBitmap;
+
+typedef int RequestId;
+static const int REQUEST_ID_INVALID = 0;

[sky, 2014/06/17 00:13:40]

I would be inclined to make this a member of BitmapFetcherService. Better
scoping then.

[groby-ooo-7-16, 2014/06/17 18:06:23]

Done.

+
+class BitmapFetcherObserver {
+ public:
+  virtual ~BitmapFetcherObserver() {}
+
+  virtual void OnImageChanged(const SkBitmap& answers_image) = 0;

[sky, 2014/06/17 00:13:40]

It's not clear to me (without looking at the code), why there are two methods
here and if this (OnImageChanged) is invoked multiple times. It is possible to
have just one method that is invoked with the id and SkBitmap? If the fetch
fails the SkBitmap could be empty.

[groby-ooo-7-16, 2014/06/17 18:06:23]

I can probably roll this into one. Giving it a spin.

+  virtual void OnRequestFinished(int request_id) = 0;
+};
+
+// Service to retrieve images for Answers in Suggest.
+class BitmapFetcherService : public KeyedService,
+                            public chrome::BitmapFetcherDelegate {

[sky, 2014/06/17 00:13:40]

nit: indent one more.

[groby-ooo-7-16, 2014/06/17 18:06:23]

Done.

+ public:
+  explicit BitmapFetcherService(content::BrowserContext* context);
+  virtual ~BitmapFetcherService();
+
+  // Cancels a request, if it is still in-flight.
+  void CancelRequest(int requestId);

[sky, 2014/06/17 00:13:40]

int->RequestId

[groby-ooo-7-16, 2014/06/17 18:06:23]

Done.

+
+  // Requests a new image. Will either trigger download or satisfy from cache.

[sky, 2014/06/17 00:13:40]

Looks like this can fail if there are already too many outstanding requests too.

[groby-ooo-7-16, 2014/06/17 18:06:23]

Clarified docs.

+  // Takes ownership of |observer|.
+  RequestId RequestImage(const GURL& url, BitmapFetcherObserver* observer);

[sky, 2014/06/17 00:13:40]

Make this take a scoped_ptr. You should also document notifies, and destroies,
observer if image already downloaded.

[groby-ooo-7-16, 2014/06/17 18:06:23]

The destruction is kind of implicit in "takes ownership" - I'd rather not commit
to a _specific_ lifetime here. Clarified notification.

[sky, 2014/06/17 19:19:47]

I wasn't clear. I think the interesting bit is that you may end up notifying
synchronously, before the function returns, that often times can be unexpected.

[groby-ooo-7-16, 2014/06/17 21:24:30]

Clarified comment some more :)

+
+  // Start fetching the image at the given |url|.
+  void Prefetch(const GURL& url);
+
+ protected:
+  // Create a bitmap fetcher for the given |url| and start it. Virtual method
+  // so tests can override this for different behavior.
+  virtual chrome::BitmapFetcher* CreateFetcher(const GURL& url);

[sky, 2014/06/17 00:13:40]

Subclassing for tests always results in introducing weird APIs. How about a
delegate for this? That way no subclassing and separation is clearer.

[groby-ooo-7-16, 2014/06/17 18:06:23]

How's a subclassable factory method a weird API? I thought about a delegate, but
that's an additional indirection, extra data storage.

If you still feel bad about the API as-is, I'll look at changing it, but I
prefer to keep this.

[sky, 2014/06/17 19:19:47]

Having a pure virtual delegate is a lot cleaner and better separation, in my
opinion. Once you head down the subclass route it's all too easy to start
opening up API in weird ways that makes the code harder to understand.
I'm ok with what you have.

+
+ private:
+  friend class BitmapFetcherServiceTest;
+
+  // Gets the existing fetcher for |url| or constructs a new one if it doesn't
+  // exist.
+  const chrome::BitmapFetcher* EnsureFetcherForUrl(const GURL& url);
+
+  // Find a fetcher with a given |url|. Return NULL if none is found.
+  const chrome::BitmapFetcher* FindFetcherForUrl(const GURL& url);
+
+  // Remove |fetcher| from list of active fetchers. |fetcher| MUST be part of
+  // the list.
+  void RemoveFetcher(const chrome::BitmapFetcher* fetcher);
+
+  // BitmapFetcherDelegate implementation.
+  virtual void OnFetchComplete(const GURL url, const SkBitmap* bitmap) OVERRIDE;
+
+  // Currently active image fetchers.
+  ScopedVector<chrome::BitmapFetcher> active_fetchers_;
+
+  // Currently active requests.
+  ScopedVector<BitmapFetcherRequest> requests_;
+
+  // Cache of retrieved images.
+  struct CacheEntry {
+    CacheEntry();
+    ~CacheEntry();
+
+    scoped_ptr<const SkBitmap> bitmap;
+  };
+  base::OwningMRUCache<GURL, CacheEntry*> cache_;
+
+  // Current request ID to be used.
+  int current_request_id_;
+
+  // Browser context this service is active for.
+  content::BrowserContext* context_;
+};

[sky, 2014/06/17 00:13:40]

DISALLOW...

[groby-ooo-7-16, 2014/06/17 18:06:23]

Done.

+
+#endif  // CHROME_BROWSER_BITMAP_FETCHER_BITMAP_FETCHER_SERVICE_H_