Fetch incremental updates. Store new state in V4Store.

components/safe_browsing_db/v4_protocol_manager_util.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 "components/safe_browsing_db/v4_protocol_manager_util.h"
 
 #include "base/base64.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/base/escape.h"
 #include "net/http/http_request_headers.h"
 
 using base::Time;
 using base::TimeDelta;
 
 namespace safe_browsing {
 
 // The Safe Browsing V4 server URL prefix.
 const char kSbV4UrlPrefix[] = "https://safebrowsing.googleapis.com/v4";
 
+std::ostream& operator<<(std::ostream& os, const UpdateListIdentifier& id) {
+  os << "hash: " << id.hash() << "; platform_type: " << id.platform_type
+     << "; threat_entry_type: " << id.threat_entry_type
+     << "; threat_type: " << id.threat_type;
+  return os;
+}

[Scott Hess - ex-Googler, 2016/06/17 22:53:43]

I'm not sure how this integrates with the overall stream.  Maybe get some {}
around the aggregate so that it's clear that this is a unit.

Actually, I'm not _entirely_ comfortable having logging stuff like this.  It has
an unclear cost which probably won't be of general utility.

[vakh (use Gerrit instead), 2016/06/20 22:28:43]

You mean just empty {}? It's not clear to me how that would be beneficial.
This isn't logging stuff on its own and is very useful in debugging things
(specially when gdb isn't that useful). It does provide value in terms of time
saved debugging. Is there a recommended way of checking in code that is useful
in the near future (while building stuff), but may be not in the long term?

I could add a crbug to remove this (and other debugging aids) after a quarter.

[Scott Hess - ex-Googler, 2016/06/21 21:03:44]

I mean that whoever is invoking this might be coded like:
  LOG(ERROR) << "Got to " << __FUNCTION__ << ", list_id: " << list_id << ",
something_else: " << something_else;
this is going to come out like:
  Got to MyFunction, list_id: hash: 0x8888; platform_type: WINDOWS;
threat_entry_type: TYPE; threat_type: TYPE, something_else: some string or
something.
everything is flat, so it's not clear where the list_id ends.  So I'm suggesting
something to make that clear, like:
  Got to MyFunction, list_id: {hash: 0x8888; platform_type: WINDOWS;
threat_entry_type: TYPE; threat_type: TYPE}, something_else: some string or
something.
Do you expect that you'll be regularly communicating with other developers to
find these debug lines?  What I've generally found with DLOG output is that
often enough, I end up having to rewrite whatever is there anyhow, because
whatever thing I was debugging last time was fixed, and the new thing is new
otherwise I'd have debugged it last time.  If it's just a convenience so that
you don't have to rewrite the log lines in the future, you should keep them on a
local git branch, rather than checking them into the repo.  [BTW, I do this all
the time.  Usually it's pretty straight-forward, I'll just cherry-pick the
logging patch over as needed to debug something.]

I guess where I'm going is that IMHO you should only checkin logging code if you
have a reason to believe you'll regularly ask for the results of the logging
code.  Often enough, the easiest answer is to not checkin the logging code until
you find the case where you would have asked for it, _then_ check it in.  99% of
the time you'd have never asked, and the logging code will site in the tree
forever, waiting for someone to actually use it.

The kinds of cases I hate are when I'm trying to debug a build break, and there
are a hundred lines of logging output which I can't tell if it is actually
telling me there's a problem, or if someone is logging it just in case it might
be useful someday.  Almost all the time, it's just-in-case logging, and I'm
annoyed that I have to wade through it looking for the pertinent data.  I
_think_ you're using DVLOG, so that's less of an issue - but, of course, in that
case it means that you can't have the case of finding the log output
retrospectively, you have to intend to get it.

[vakh (use Gerrit instead), 2016/06/21 23:19:35]

Done.
Acknowledged. WRT this particular method, it is not outputting any log by itself
so there's no harm in keeping it. As you pointed out, the other statements are
also DVLOGs so they'll only be printed in a DEBUG build and that too only about
once in half-an-hour so other developers shouldn't be too affected.

[Scott Hess - ex-Googler, 2016/06/24 23:01:19]

I'll grant that there may not be _much_ harm in keeping it, but I think "no harm
in keeping it" isn't right.  Unnecessary log lines add to compile time in all
cases, reduce readability, and in this case add to binary size in debug, and
possibly adds irrelevant output if someone uses the wrong --verbose rules. 
Those costs potentially apply to everyone on the project, whereas the benefits
probably only apply to you.

[This be didn't get into my bonnet with Chrome - I used to greatly despise
having a production emergency where my first order of business was to write
filters to let me try to figure out which bits of a 100MB daemond.ERROR file
might be relevant.]

[vakh (use Gerrit instead), 2016/06/27 19:38:27]

It is being used in a valid case: if we get an update for a list that we didn't
expect, it prints that list's identifier in logs.
In V4Database::ApplyUpdate()
The alternative is to "inline" this method in that error condition.

Given that, if you still feel that it is not useful to have this method, please
continue reading this comment.
Otherwise please ignore the rest of my comment.
It is only being used when an expected condition occurs so I wouldn't call it
unnecessary.
Sure, but so would the inline case.
Better than inline case.
So would the inline case
I've removed all other calls to this method already so there shouldn't be any
irrelevant output.
 
The alternative is that I remove this method and the "inline" printing of the
identifier and fail with no information in the logs.
Off topic, feel free to ignore: "bonnet with X" -- I couldn't understand what
this means and even Google search didn't reveal anything.

[Scott Hess - ex-Googler, 2016/06/27 22:19:17]

Sigh, "bee in my bonnet".  Basically "Why does Scott hate log lines so much?" 
I've pummeled you enough with that thread for one review.

[vakh (use Gerrit instead), 2016/06/27 23:58:52]

Acknowledged.

+
 bool UpdateListIdentifier::operator==(const UpdateListIdentifier& other) const {
   return platform_type == other.platform_type &&
          threat_entry_type == other.threat_entry_type &&
          threat_type == other.threat_type;
 }
 
 bool UpdateListIdentifier::operator!=(const UpdateListIdentifier& other) const {
   return !operator==(other);
 }
 
 size_t UpdateListIdentifier::hash() const {
   std::size_t first = std::hash<unsigned int>()(platform_type);
   std::size_t second = std::hash<unsigned int>()(threat_entry_type);
   std::size_t third = std::hash<unsigned int>()(threat_type);
 
   std::size_t interim = base::HashInts(first, second);
   return base::HashInts(interim, third);
 }
 
+UpdateListIdentifier::UpdateListIdentifier() {}

[Scott Hess - ex-Googler, 2016/06/17 22:53:43]

Is the bare version ever used?

If so, will everything work correctly if the member variables are initialized
with random data which may be out of range?  If there aren't obvious defaults to
use, it might be worthwhile to see if you can make this private: to prevent
accidents.

[vakh (use Gerrit instead), 2016/06/20 22:28:43]

Done.

+
+UpdateListIdentifier::UpdateListIdentifier(PlatformType platform_type,
+                                           ThreatEntryType threat_entry_type,
+                                           ThreatType threat_type)
+    : platform_type(platform_type),
+      threat_entry_type(threat_entry_type),
+      threat_type(threat_type) {}
+
+UpdateListIdentifier::UpdateListIdentifier(const ListUpdateResponse& response)
+    : UpdateListIdentifier(response.platform_type(),
+                           response.threat_entry_type(),
+                           response.threat_type()) {}
+
 V4ProtocolConfig::V4ProtocolConfig() : disable_auto_update(false) {}
 
 V4ProtocolConfig::V4ProtocolConfig(const V4ProtocolConfig& other) = default;
 
 V4ProtocolConfig::~V4ProtocolConfig() {}
 
 // static
 // Backoff interval is MIN(((2^(n-1))*15 minutes) * (RAND + 1), 24 hours) where
 // n is the number of consecutive errors.
 base::TimeDelta V4ProtocolManagerUtil::GetNextBackOffInterval(
@@ skipping 56 matching lines @@
 
 // static
 void V4ProtocolManagerUtil::UpdateHeaders(net::HttpRequestHeaders* headers) {
   // NOTE(vakh): The following header informs the envelope server (which sits in
   // front of Google's stubby server) that the received GET request should be
   // interpreted as a POST.
   headers->SetHeaderIfMissing("X-HTTP-Method-Override", "POST");
 }
 
 }  // namespace safe_browsing