[blimp] Add SkPicture caching support.

blimp/common/compositor/blimp_picture_cache_registry.cc

+// Copyright 2016 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 "blimp/common/compositor/blimp_picture_cache_registry.h"
+
+#include <stdint.h>
+#include <map>
+#include <set>
+
+#include "base/logging.h"
+
+namespace blimp {
+
+BlimpPictureCacheRegistry::BlimpPictureCacheRegistry() {}
+
+BlimpPictureCacheRegistry::~BlimpPictureCacheRegistry() {}
+
+void BlimpPictureCacheRegistry::Stage(uint32_t item) {
+  auto it = staging_.find(item);

[vmpstr, 2016/05/25 01:54:06]

This whole function is equivalent to:

++staging_[item];

[nyquist, 2016/05/26 01:08:09]

Done.

+  if (it == staging_.end()) {
+    staging_.insert(std::make_pair(item, 1));
+    return;
+  }
+  ++it->second;
+}
+
+void BlimpPictureCacheRegistry::Unstage(uint32_t item) {
+  auto it = staging_.find(item);
+  if (it == staging_.end()) {

[vmpstr, 2016/05/25 01:54:06]

If you skip this check (and please let me know why you have it instead of a
DCHECK), then this is all --staging_[item];

[nyquist, 2016/05/26 01:08:09]

Done.

+    NOTREACHED() << "Trying to unstage " << item
+                 << " which has not been staged.";
+    return;
+  }
+  --it->second;
+  DCHECK_GE(it->second, 0U);

[vmpstr, 2016/05/25 01:54:06]

unsigned value is always >= 0

[nyquist, 2016/05/26 01:08:09]

... Done.

+}
+
+void BlimpPictureCacheRegistry::Commit(std::set<uint32_t>* added_entries,

[vmpstr, 2016/05/25 01:54:06]

Because these are generated from an std::map, this is guaranteed to be unique
even if it's a vector. Do you need this to be a set? It has different complexity
behavior (depending on what you need it for)

[nyquist, 2016/05/26 01:08:09]

Done.

+                                       std::set<uint32_t>* removed_entries) {
+  // Staged entries with 0 usage.
+  std::set<uint32_t> unused;
+  // Staged entries with > 0 usage.
+  std::set<uint32_t> used;
+
+  for (const auto& it : staging_) {
+    if (it.second > 0) {

[vmpstr, 2016/05/25 01:54:06]

Maybe:

auto& which = it.second > 0 ? used : unused;
which.insert(it.first);

[nyquist, 2016/05/26 01:08:09]

Irrelevant after the rest of the change.

+      used.insert(it.first);
+    } else {
+      unused.insert(it.first);
+    }
+  }
+
+  // Remove unused entries from |committed_| and update |removed_entries|.

[vmpstr, 2016/05/25 01:54:06]

Below here, you iterate over unused (without using used) and then iterate over
used (without using unused). Why not just do this in the loop above?

something along the lines of

for (auto it = staging_.begin(); it != staging_.end();) {
  uint32_t key = it->first;
  uint32_t ref_count = it->second;
  if (ref_count == 0) {
    // This is unused.
    if (committed_.find(key) != committed_.end()) {
      committed_.erase(key);
      removed_entries->insert(key);
    }
    it = staging_.erase(it);
  } else {
    // This is used.
    if (committed_.find(key) == committed_.end()) {
      committed_.insert(key);
      added_entries->insert(key);
    }
    ++it;
  }
}

Also, if using the typical terminology after a "commit" step, you shouldn't have
any staged changes. 

[nyquist, 2016/05/26 01:08:09]

Done.

+  for (const auto it : unused) {

[vmpstr, 2016/05/25 01:54:06]

don't name it "it" if it's not an iterator. Also, put the actual type instead of
auto. 

[nyquist, 2016/05/26 01:08:09]

Done.

+    if (committed_.find(it) != committed_.end()) {
+      committed_.erase(it);
+      removed_entries->insert(it);
+    }
+    // The entry has no references, so should not be staged anymore.
+    staging_.erase(it);
+  }
+
+  // Add new entries to |committed_| and update |added_entries|.
+  for (const auto it : used) {
+    if (committed_.find(it) == committed_.end()) {
+      committed_.insert(it);
+      added_entries->insert(it);
+    }
+  }
+}
+
+}  // namespace blimp