Safe-browsing SBAddPrefix from vector to deque.

chrome/browser/safe_browsing/safe_browsing_database.cc

 // Copyright (c) 2011 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/safe_browsing/safe_browsing_database.h"
 
 #include <algorithm>
 #include <iterator>
 
 #include "base/bind.h"
@@ skipping 246 matching lines @@
   UMA_HISTOGRAM_ENUMERATION("SB2.PrefixSetEvent", event_type,
                             PREFIX_SET_EVENT_MAX);
 }
 
 // Generate a |PrefixSet| instance from the contents of
 // |add_prefixes|.  Additionally performs various checks to make sure
 // that the resulting prefix set is valid, so that the
 // PREFIX_SET_EVENT_BLOOM_MISS_PREFIX_HIT_INVALID histogram in
 // ContainsBrowseUrl() can be trustworthy.
 safe_browsing::PrefixSet* PrefixSetFromAddPrefixes(
-    const std::vector<SBAddPrefix>& add_prefixes) {
+    const SBAddPrefixContainer& add_prefixes) {
   // TODO(shess): If |add_prefixes| were sorted by the prefix, it
   // could be passed directly to |PrefixSet()|, removing the need for
   // |prefixes|.  For now, |prefixes| is useful while debugging
   // things.
   std::vector<SBPrefix> prefixes;
   prefixes.reserve(add_prefixes.size());
-  for (size_t i = 0; i < add_prefixes.size(); ++i) {
-    prefixes.push_back(add_prefixes[i].prefix);
+  for (SBAddPrefixContainer::const_iterator iter = add_prefixes.begin();
+       iter != add_prefixes.end(); ++iter) {
+    prefixes.push_back(iter->prefix);
   }
 
   std::sort(prefixes.begin(), prefixes.end());
   prefixes.erase(std::unique(prefixes.begin(), prefixes.end()),
                  prefixes.end());
 
   scoped_ptr<safe_browsing::PrefixSet>
       prefix_set(new safe_browsing::PrefixSet(prefixes));
 
   std::vector<SBPrefix> restored;
@@ skipping 415 matching lines @@
                                full_hits, last_update);
   return true;
 }
 
 bool SafeBrowsingDatabaseNew::MatchDownloadAddPrefixes(
     int list_bit,
     const std::vector<SBPrefix>& prefixes,
     std::vector<SBPrefix>* prefix_hits) {
   prefix_hits->clear();
 
-  std::vector<SBAddPrefix> add_prefixes;
+  SBAddPrefixContainer add_prefixes;
   download_store_->GetAddPrefixes(&add_prefixes);
-  for (size_t i = 0; i < add_prefixes.size(); ++i) {
+  for (SBAddPrefixContainer::const_iterator iter = add_prefixes.begin();
+       iter != add_prefixes.end(); ++iter) {
     for (size_t j = 0; j < prefixes.size(); ++j) {
       const SBPrefix& prefix = prefixes[j];
-      if (prefix == add_prefixes[i].prefix &&
-          GetListIdBit(add_prefixes[i].chunk_id) == list_bit) {
+      if (prefix == iter->prefix &&
+          GetListIdBit(iter->chunk_id) == list_bit) {
         prefix_hits->push_back(prefix);
       }
     }
   }
   return !prefix_hits->empty();
 }
 
 bool SafeBrowsingDatabaseNew::ContainsDownloadUrl(
     const std::vector<GURL>& urls,
     std::vector<SBPrefix>* prefix_hits) {
@@ skipping 403 matching lines @@
 
   // For the whitelists, we don't cache and save full hashes since all
   // hashes are already full.
   std::vector<SBAddFullHash> empty_add_hashes;
 
   // Not needed for the whitelists.
   std::set<SBPrefix> empty_miss_cache;
 
   // Note: prefixes will not be empty.  The current data store implementation
   // stores all full-length hashes as both full and prefix hashes.
-  std::vector<SBAddPrefix> prefixes;
+  SBAddPrefixContainer prefixes;

[jar (doing other things), 2011/10/21 19:10:45]

Use of SB as a part of the name actually tricked me... I understand it now...
but can look at the style guide and suggest you not use abreviations.  Besides,
the type is only defined in the SafeBrowsing class... so heck... lets drop the
SB.

[Scott Hess - ex-Googler, 2011/10/21 23:39:32]

SBAddPrefixContainer is defined in global scope in safe_browsing_store.h. 
Someone needs to cover the code and namespace the heck out of it.

   std::vector<SBAddFullHash> full_hashes;
   if (!store->FinishUpdate(empty_add_hashes, empty_miss_cache, &prefixes,
                            &full_hashes)) {
     RecordFailure(FAILURE_WHITELIST_DATABASE_UPDATE_FINISH);
     WhitelistEverything(whitelist);
     return;
   }
 
 #if defined(OS_MACOSX)
   base::mac::SetFileBackupExclusion(store_filename);
 #endif
 
   LoadWhitelist(full_hashes, whitelist);
 }
 
 void SafeBrowsingDatabaseNew::UpdateDownloadStore() {
   if (!download_store_.get())
     return;
 
   // For download, we don't cache and save full hashes.
   std::vector<SBAddFullHash> empty_add_hashes;
 
   // For download, backend lookup happens only if a prefix is in add list.
   // No need to pass in miss cache when call FinishUpdate to caculate
   // bloomfilter false positives.
   std::set<SBPrefix> empty_miss_cache;
 
   // These results are not used after this call. Simply ignore the
   // returned value after FinishUpdate(...).
-  std::vector<SBAddPrefix> add_prefixes_result;
+  SBAddPrefixContainer add_prefixes_result;

[jar (doing other things), 2011/10/21 19:10:45]

It might even be the case that the type name should simply be:
Prefixes

I like using plural to denote a container ;-).

[Scott Hess - ex-Googler, 2011/10/21 23:39:32]

I'm reasonable with s/SBAddPrefixContainer/SBAddPrefixes/.  I think I was too
much in the mode of meta-container.

   std::vector<SBAddFullHash> add_full_hashes_result;
 
   if (!download_store_->FinishUpdate(empty_add_hashes,
                                      empty_miss_cache,
                                      &add_prefixes_result,
                                      &add_full_hashes_result))
     RecordFailure(FAILURE_DOWNLOAD_DATABASE_UPDATE_FINISH);
 
   int64 size_64;
   if (file_util::GetFileSize(download_filename_, &size_64)) {
@@ skipping 29 matching lines @@
 #endif
   );
 
   // IoCounters are currently not supported on Mac, and may not be
   // available for Linux, so we check the result and only show IO
   // stats if they are available.
   const bool got_counters = metric->GetIOCounters(&io_before);
 
   const base::Time before = base::Time::Now();
 
-  std::vector<SBAddPrefix> add_prefixes;
+  SBAddPrefixContainer add_prefixes;
   std::vector<SBAddFullHash> add_full_hashes;
   if (!browse_store_->FinishUpdate(pending_add_hashes, prefix_miss_cache_,
                                    &add_prefixes, &add_full_hashes)) {
     RecordFailure(FAILURE_BROWSE_DATABASE_UPDATE_FINISH);
     return;
   }
 
   // Create and populate |filter| from |add_prefixes|.
   // TODO(shess): The bloom filter doesn't need to be a
   // scoped_refptr<> for this code.  Refactor that away.
   const int filter_size =
       BloomFilter::FilterSizeForKeyCount(add_prefixes.size());
   scoped_refptr<BloomFilter> filter(new BloomFilter(filter_size));
-  for (size_t i = 0; i < add_prefixes.size(); ++i) {
-    filter->Insert(add_prefixes[i].prefix);
+  for (SBAddPrefixContainer::const_iterator iter = add_prefixes.begin();
+       iter != add_prefixes.end(); ++iter) {
+    filter->Insert(iter->prefix);
   }
 
   scoped_ptr<safe_browsing::PrefixSet>
       prefix_set(PrefixSetFromAddPrefixes(add_prefixes));
 
   // This needs to be in sorted order by prefix for efficient access.
   std::sort(add_full_hashes.begin(), add_full_hashes.end(),
             SBAddFullHashPrefixLess);
 
   // Swap in the newly built filter and cache.
@@ skipping 91 matching lines @@
   const base::TimeTicks before = base::TimeTicks::Now();
   browse_bloom_filter_ = BloomFilter::LoadFile(bloom_filter_filename_);
   DVLOG(1) << "SafeBrowsingDatabaseNew read bloom filter in "
            << (base::TimeTicks::Now() - before).InMilliseconds() << " ms";
 
   if (!browse_bloom_filter_.get())
     RecordFailure(FAILURE_DATABASE_FILTER_READ);
 
   // Manually re-generate the prefix set from the main database.
   // TODO(shess): Write/read for prefix set.
-  std::vector<SBAddPrefix> add_prefixes;
+  SBAddPrefixContainer add_prefixes;
   browse_store_->GetAddPrefixes(&add_prefixes);

[jar (doing other things), 2011/10/21 19:10:45]

I won't push for all teh names changed to be readable.... but I think this stuff
should be:

GetAdditionalPrefixes(&additional_prefixes).

The word "Add" seems like a verb, but I *think* you need a noun or noun phrase
(...and as I said... you don't have to fix this... but it often makes code more
readable, an susceptible to spotting bugs ;-) )

[Scott Hess - ex-Googler, 2011/10/21 23:39:32]

Unfortunately ... sigh.  I hate this code.

The "add" here describes the type of prefix.  These are not additional prefixes,
it is the complete set of prefixes of type "add", which came from an "add"
chunk.  There are also "sub" chunks.  There are also deletions of add chunks and
sub chunks, just to make things more enjoyable.

Perhaps most importantly, the safe-browsing protocol docs use the terms "add"
chunk and "sub" chunk.

At root, the server is sending a set of edits to a database of prefixes.  The
edits are first-class, so you can't simply apply them and move on, because the
server may later say "Delete all the edits I sent in the past" and now you get a
different database of prefixes.  It's terribly mysterious.

   prefix_set_.reset(PrefixSetFromAddPrefixes(add_prefixes));
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {
   DCHECK_EQ(creation_loop_, MessageLoop::current());
 
   const bool r1 = browse_store_->Delete();
   if (!r1)
     RecordFailure(FAILURE_DATABASE_STORE_DELETE);
 
@@ skipping 65 matching lines @@
   if (std::binary_search(new_whitelist.begin(), new_whitelist.end(),
                          kill_switch)) {
     // The kill switch is whitelisted hence we whitelist all URLs.
     WhitelistEverything(whitelist);
   } else {
     base::AutoLock locked(lookup_lock_);
     whitelist->second = false;
     whitelist->first.swap(new_whitelist);
   }
 }