[AiS] ImageService

chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc

Index: chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc
diff --git a/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc b/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7e332d811901933f9e6a8f4cc943d60eba34a827
--- /dev/null
+++ b/chrome/browser/bitmap_fetcher/bitmap_fetcher_service.cc
@@ -0,0 +1,179 @@
+// 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/bitmap_fetcher/bitmap_fetcher_service.h"
+
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/bitmap_fetcher/bitmap_fetcher.h"
+#include "chrome/browser/profiles/profile.h"
+#include "net/base/load_flags.h"
+#include "third_party/skia/include/core/SkBitmap.h"
+
+namespace {
+
+const int kMaxRequests = 25;  // Maximum number of in-flight requests allowed.
+const int kMaxCacheEntries = 5;  // Maximum number of cache entries.
+
+}  // namespace.
+
+class BitmapFetcherRequest {
+ public:
+  BitmapFetcherRequest(RequestId request_id, BitmapFetcherObserver* observer);
+  ~BitmapFetcherRequest();
+
+  void NotifyImageChanged(const SkBitmap& bitmap);
+  RequestId request_id() const { return request_id_; }
+
+  // Weak ptr |fetcher| is used to identify associated fetchers.
+  void set_fetcher(const chrome::BitmapFetcher* fetcher) { fetcher_ = fetcher; }
+  const chrome::BitmapFetcher* get_fetcher() const { return fetcher_; }
+
+ private:
+  RequestId request_id_;

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

const?

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

Done.

+  scoped_ptr<BitmapFetcherObserver> observer_;
+  const chrome::BitmapFetcher* fetcher_;
+};

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

DISALLOW...

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

Done.

+
+BitmapFetcherRequest::BitmapFetcherRequest(RequestId request_id,
+                                         BitmapFetcherObserver* observer)
+    : request_id_(request_id), observer_(observer) {
+}
+
+BitmapFetcherRequest::~BitmapFetcherRequest() {
+  observer_->OnRequestFinished(request_id_);
+}
+
+void BitmapFetcherRequest::NotifyImageChanged(const SkBitmap& bitmap) {
+  observer_->OnImageChanged(bitmap);
+}
+
+BitmapFetcherService::CacheEntry::CacheEntry() {
+}
+
+BitmapFetcherService::CacheEntry::~CacheEntry() {
+}
+
+BitmapFetcherService::BitmapFetcherService(content::BrowserContext* context)
+    : cache_(kMaxCacheEntries), current_request_id_(1), context_(context) {
+  Profile::FromBrowserContext(context);

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

Why do you need this?

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

I don't - missed to delete this.

+}
+
+BitmapFetcherService::~BitmapFetcherService() {
+}
+
+void BitmapFetcherService::CancelRequest(int request_id) {
+  ScopedVector<BitmapFetcherRequest>::iterator iter;
+  for (iter = requests_.begin(); iter != requests_.end(); ++iter) {
+    if ((*iter)->request_id() == request_id) {
+      requests_.erase(iter);

[sky, 2014/06/17 03:00:02]

Shouldn't you potentially delete the BitmapFetcher? Otherwise it's unnecessarily
limiting the number of requests that can be outstanding.

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

That's actually intentional - we will likely need the data later, anyways. 

This is for "Answers in Suggest" - getting Onebox answers into the Omnibox. The
only images right now are weather images, so unless you only care about LA
weather, you'll need a variety of them. At the same time, there really
_shouldn't_ be too many requests in-flight, since it's triggered by a user query
for which we have high confidence that we know the completion.

Added a short comment that this is deliberate.

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

Isn't this is a general service though that could be used anywhere else there
are similar needs?

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

Yes, but

1) There are currently no consumers requiring a different behavior
2) BitmapFetchers are shared across multiple requests. I could ref-count them,
but that seems overkill from here.
3) Worst part, BitmapFetcher is not cancelable. If you issue a fetch request,
you better want that image.

+      return;
+    }
+  }
+}
+
+const chrome::BitmapFetcher* BitmapFetcherService::FindFetcherForUrl(
+    const GURL& url) {
+  ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();

[sky, 2014/06/17 03:00:02]

nit: move into for loop.

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

Done. Even though it's ugly :)

+  for (; iter != active_fetchers_.end(); ++iter) {
+    if (url == (*iter)->url())
+      return *iter;
+  }
+  return NULL;
+}
+
+void BitmapFetcherService::RemoveFetcher(const chrome::BitmapFetcher* fetcher) {
+  ScopedVector<chrome::BitmapFetcher>::iterator iter = active_fetchers_.begin();
+  for (; iter != active_fetchers_.end(); ++iter) {
+    if (fetcher == (*iter))
+      break;

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

Any reason you don't erase here then return? Similar to what you do on 70.

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

Done.

+  }
+  DCHECK(iter != active_fetchers_.end());
+  active_fetchers_.erase(iter);
+}
+
+chrome::BitmapFetcher* BitmapFetcherService::CreateFetcher(const GURL& url) {
+  chrome::BitmapFetcher* new_fetcher = new chrome::BitmapFetcher(url, this);
+
+  new_fetcher->Start(
+      context_->GetRequestContext(),
+      std::string(),
+      net::URLRequest::CLEAR_REFERRER_ON_TRANSITION_FROM_SECURE_TO_INSECURE,
+      net::LOAD_NORMAL);
+  return new_fetcher;
+}
+
+const chrome::BitmapFetcher* BitmapFetcherService::EnsureFetcherForUrl(
+    const GURL& url) {
+  const chrome::BitmapFetcher* fetcher = FindFetcherForUrl(url);
+  if (fetcher)
+    return fetcher;
+
+  chrome::BitmapFetcher* new_fetcher = CreateFetcher(url);
+  active_fetchers_.push_back(new_fetcher);
+  return new_fetcher;
+}
+
+void BitmapFetcherService::Prefetch(const GURL& url) {
+  (void)EnsureFetcherForUrl(url);

[sky, 2014/06/17 03:00:02]

Remove (void)

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

Done.

+}
+
+RequestId BitmapFetcherService::RequestImage(const GURL& url,

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

make order match header.

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

Done.

+                                            BitmapFetcherObserver* observer) {

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

indentation is off (maybe for all lines that you've wrapped, not sure).

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

Done.

+  // Create a new request, assigning next available request ID.
+  ++current_request_id_;
+  if (current_request_id_ == REQUEST_ID_INVALID)
+    ++current_request_id_;
+  int request_id = current_request_id_;
+  scoped_ptr<BitmapFetcherRequest> request(
+      new BitmapFetcherRequest(request_id, observer));
+
+  // Check for existing images first.
+  base::OwningMRUCache<GURL, CacheEntry*>::iterator iter = cache_.Get(url);
+  if (iter != cache_.end()) {
+    BitmapFetcherService::CacheEntry* entry = iter->second;
+    request->NotifyImageChanged(*(entry->bitmap.get()));
+
+    // There is no request ID associated with this - data is already delivered.
+    return REQUEST_ID_INVALID;
+  }
+
+  // Limit number of simultaneous in-flight requests.
+  if (requests_.size() > kMaxRequests)
+    return REQUEST_ID_INVALID;
+
+  // Make sure there's a fetcher for this URL and attach to request.
+  const chrome::BitmapFetcher* fetcher = EnsureFetcherForUrl(url);
+  request->set_fetcher(fetcher);
+
+  requests_.push_back(request.release());
+  return requests_.back()->request_id();
+}
+
+void BitmapFetcherService::OnFetchComplete(const GURL url,
+                                          const SkBitmap* bitmap) {
+  DCHECK(bitmap);  // can never be NULL, guaranteed by BitmapFetcher.
+
+  const chrome::BitmapFetcher* fetcher = FindFetcherForUrl(url);
+  DCHECK(fetcher);
+
+  // Notify all attached requests of completion.
+  ScopedVector<BitmapFetcherRequest>::iterator iter;

[sky, 2014/06/17 03:00:02]

Use for loop and move iter into it.

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

Can't use for(), since iter is only incremented if the request isn't erased.

+  iter = requests_.begin();
+  while (iter != requests_.end()) {
+    if ((*iter)->get_fetcher() == fetcher) {
+      (*iter)->NotifyImageChanged(*bitmap);
+      iter = requests_.erase(iter);
+    } else {
+      ++iter;
+    }
+  }
+
+  if (!bitmap->isNull()) {
+    CacheEntry* entry = new CacheEntry;
+    entry->bitmap.reset(new SkBitmap(*bitmap));
+    cache_.Put(fetcher->url(), entry);
+  }
+
+  RemoveFetcher(fetcher);
+}