Instant: Allow empty sets to be added to the cache.

chrome/common/instant_restricted_id_cache.h

 // Copyright 2013 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_COMMON_INSTANT_RESTRICTED_ID_CACHE_H_
 #define CHROME_COMMON_INSTANT_RESTRICTED_ID_CACHE_H_
 
 #include <set>
 #include <utility>
 #include <vector>
@@ skipping 50 matching lines @@
   // Returns true if the |restricted_id| is present in the cache and if so,
   // returns a copy of the item.
   bool GetItemWithRestrictedID(InstantRestrictedID restricted_id,
                                T* item) const;
 
  private:
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, AutoIDGeneration);
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, CrazyIDGeneration);
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, ManualIDGeneration);
   FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, MixIDGeneration);
+  FRIEND_TEST_ALL_PREFIXES(InstantRestrictedIDCacheTest, AddEmptySet);
 
   typedef base::MRUCache<InstantRestrictedID, T> CacheImpl;
 
   mutable CacheImpl cache_;
   typename CacheImpl::reverse_iterator last_add_start_;
   InstantRestrictedID last_restricted_id_;
 
   DISALLOW_COPY_AND_ASSIGN(InstantRestrictedIDCache);
 };
 
 template <typename T>
 InstantRestrictedIDCache<T>::InstantRestrictedIDCache(size_t max_cache_size)
     : cache_(max_cache_size),
       last_add_start_(cache_.rend()),
       last_restricted_id_(0) {
   DCHECK(max_cache_size);
 }
 
 template <typename T>
 InstantRestrictedIDCache<T>::~InstantRestrictedIDCache() {
 }
 
 template <typename T>
 void InstantRestrictedIDCache<T>::AddItems(const ItemVector& items) {
-  if (items.size() == 0 || items.size() > cache_.max_size())
+  if (items.size() > cache_.max_size())
     return;

[brettw, 2013/03/25 22:26:52]

I'm wondering about this. If you add too many items, does this actually do what
we want? The current items will still be the old ones. It seems like you would
want the same behavior as you currently have in the "add an empty vector" case.

[sreeram, 2013/03/25 22:29:33]

Adding too many items would be an error situation, i.e., not supported. Changing
it to be the same as adding an empty set (which _is_ supported) would give the
wrong impression of stuff working, while in fact silently breaking things. I
don't mind changing the 'if' to a DCHECK(), if you prefer?

[brettw, 2013/03/25 22:34:04]

Yeah, I like the dcheck.

[sreeram, 2013/03/25 22:39:26]

Done.

 
+  if (items.empty()) {
+    last_add_start_ = cache_.rend();
+    return;
+  }
+
   for (size_t i = 0; i < items.size(); ++i) {
     InstantRestrictedID id = ++last_restricted_id_;
     cache_.Put(id, items[i]);
     if (i == 0)
       last_add_start_ = --cache_.rend();
   }
 }
 
 template <typename T>
 void InstantRestrictedIDCache<T>::AddItemsWithRestrictedID(
     const ItemIDVector& items) {
-  if (items.size() == 0 || items.size() > cache_.max_size())
+  if (items.size() > cache_.max_size())
     return;
 
+  if (items.empty()) {
+    last_add_start_ = cache_.rend();
+    return;
+  }
+
   std::set<InstantRestrictedID> ids_added;
   for (size_t i = 0; i < items.size(); ++i) {
     const ItemIDPair& item_id = items[i];
 
     DCHECK(ids_added.find(item_id.first) == ids_added.end());
     ids_added.insert(item_id.first);
 
     cache_.Put(item_id.first, item_id.second);
     if (i == 0)
       last_add_start_ = --cache_.rend();
@@ skipping 18 matching lines @@
   DCHECK(item);
 
   typename CacheImpl::const_iterator cache_it = cache_.Peek(restricted_id);
   if (cache_it == cache_.end())
     return false;
   *item = cache_it->second;
   return true;
 }
 
 #endif  // CHROME_COMMON_INSTANT_RESTRICTED_ID_CACHE_H_