Stop updating SafeBrowsing download hash list.

Stop updating SafeBrowsing download hash list.

BUG=108130
TEST=wireshark


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=116853

[mattm, 2012-01-07 00:31:28]

To keep this simple and mergable, just went with not updating the list any
further.

After looking into the DB code I'm not sure it's worth the effort to actually
delete the orphaned local data.

I will make another CL later to remove the code that uses the list, but that
doesn't need to be merged.

[Scott Hess - ex-Googler, 2012-01-07 02:27:55]

LGTM, with some slight preference for simply not adding the list to the vector
in the first place rather than removing it's impact from lists afterwards.

http://codereview.chromium.org/9138003/diff/10001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/9138003/diff/10001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:1070:
download_listnames.push_back(safe_browsing_util::kBinHashList);
Can you just drop the above line?  The csd case below implies that you can do
it, as long as it doesn't change the positioning w/in the vector. 
UpdateChunkRanges() will still pull all of the data, but the final loop will
only process the adds and subs at index 0.

If you're at the right point in the transaction, deleting could be done like:

void DeleteChunksFromStore(SafeBrowsingStore* store, int listid){
  std::vector<int> chunks;

  store->GetAddChunks(&chunks);
  for (std::vector<int>::const_iterator iter = chunks.begin();
       iter != chunks.end(); ++iter) {
    if (GetListIdBit(*iter) == listid)
      store->DeleteAddChunk(*iter);
  }

  store->GetSubChunks(&chunks);
  for (std::vector<int>::const_iterator iter = chunks.begin();
       iter != chunks.end(); ++iter) {
    if (GetListIdBit(*iter) == listid)
      store->DeleteSubChunk(*iter);
  }
}

Since that will pull all the chunks every time (relatively cheap, but still),
there might be some advantage to calling it after UpdateChunkRanges() returns
results which show that there actually are chunks in there.  But I suspect it'd
be a trivial trivial trivial savings.

[mattm, 2012-01-07 03:18:12]

http://codereview.chromium.org/9138003/diff/10001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_database.cc (right):

http://codereview.chromium.org/9138003/diff/10001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_database.cc:1070:
download_listnames.push_back(safe_browsing_util::kBinHashList);
On 2012/01/07 02:27:56, shess wrote:
> Can you just drop the above line?  The csd case below implies that you can do
> it, as long as it doesn't change the positioning w/in the vector. 
> UpdateChunkRanges() will still pull all of the data, but the final loop will
> only process the adds and subs at index 0.

If we remove that, GetChunkRanges fails because it expects to receive the output
buffer of the correct size.  We could make it more permissive, but not sure
that's better since it could also hide accidental code errors.

> If you're at the right point in the transaction, deleting could be done like:
> 
> void DeleteChunksFromStore(SafeBrowsingStore* store, int listid){
>   std::vector<int> chunks;
> 
>   store->GetAddChunks(&chunks);
>   for (std::vector<int>::const_iterator iter = chunks.begin();
>        iter != chunks.end(); ++iter) {
>     if (GetListIdBit(*iter) == listid)
>       store->DeleteAddChunk(*iter);
>   }
> 
>   store->GetSubChunks(&chunks);
>   for (std::vector<int>::const_iterator iter = chunks.begin();
>        iter != chunks.end(); ++iter) {
>     if (GetListIdBit(*iter) == listid)
>       store->DeleteSubChunk(*iter);
>   }
> }

Thanks! I was thinking about that at the wrong level I think :)
Added.

> Since that will pull all the chunks every time (relatively cheap, but still),
> there might be some advantage to calling it after UpdateChunkRanges() returns
> results which show that there actually are chunks in there.  But I suspect
it'd
> be a trivial trivial trivial savings.

[Scott Hess - ex-Googler, 2012-01-08 02:10:27]

On 2012/01/07 03:18:12, mattm wrote:
> On 2012/01/07 02:27:56, shess wrote:
> > If you're at the right point in the transaction, deleting could be done
like:
<snip> 
> Thanks! I was thinking about that at the wrong level I think :)
> Added.

BTW - it might make sense to break that to a separate CL, if you're aiming to
merge things to a branch.  After thinking on it, I wonder if the delete code
shouldn't just run at startup or something, which would mean it only runs once,
and resolves the problem you mention about the code seeing invalid data, but
makes the problem of where to hang it a bit more complicated.

[mattm, 2012-01-08 22:41:59]

Yeah, took it out again.  Will upload another CL for that some later time.

On 2012/01/08 02:10:27, shess wrote:
> On 2012/01/07 03:18:12, mattm wrote:
> > On 2012/01/07 02:27:56, shess wrote:
> > > If you're at the right point in the transaction, deleting could be done
> like:
> <snip> 
> > Thanks! I was thinking about that at the wrong level I think :)
> > Added.
> 
> BTW - it might make sense to break that to a separate CL, if you're aiming to
> merge things to a branch.  After thinking on it, I wonder if the delete code
> shouldn't just run at startup or something, which would mean it only runs
once,
> and resolves the problem you mention about the code seeing invalid data, but
> makes the problem of where to hang it a bit more complicated.

[commit-bot: I haz the power, 2012-01-08 22:42:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9138003/15001

[commit-bot: I haz the power, 2012-01-08 23:56:46]

Change committed as 116853