Safe-browsing SBAddPrefix from vector to deque.

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 5210a6616688fc2b52d108409793d81bad4e6556..f41834f82f7de82b8341bf542cf6e4d452cacbfc 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -264,15 +264,16 @@ void RecordPrefixSetInfo(PrefixSetEvent event_type) {
 // 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());
@@ -708,13 +709,14 @@ bool SafeBrowsingDatabaseNew::MatchDownloadAddPrefixes(
     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);
       }
     }
@@ -1138,7 +1140,7 @@ void SafeBrowsingDatabaseNew::UpdateWhitelistStore(
 
   // 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)) {
@@ -1168,7 +1170,7 @@ void SafeBrowsingDatabaseNew::UpdateDownloadStore() {
 
   // 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,
@@ -1218,7 +1220,7 @@ void SafeBrowsingDatabaseNew::UpdateBrowseStore() {
 
   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)) {
@@ -1232,8 +1234,9 @@ void SafeBrowsingDatabaseNew::UpdateBrowseStore() {
   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>
@@ -1345,7 +1348,7 @@ void SafeBrowsingDatabaseNew::LoadBloomFilter() {
 
   // 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));
 }