[Android] Only update safe-browsing database on WiFi

chrome/browser/safe_browsing/protocol_manager.cc

 // Copyright (c) 2012 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/protocol_manager.h"
 
 #ifndef NDEBUG
 #include "base/base64.h"
 #endif
 #include "base/environment.h"
 #include "base/logging.h"
 #include "base/metrics/histogram.h"
 #include "base/rand_util.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/safe_browsing/protocol_parser.h"
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/env_vars.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
+#if defined(OS_ANDROID)
+#include "net/base/network_change_notifier.h"
+#endif

[Scott Hess - ex-Googler, 2014/05/30 16:21:07]

The OS-specific section should come after the main group of includes.  [Ignore
the NDEBUG bit, I think it's probably wrong, too.]

 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_status.h"
 
 using base::Time;
 using base::TimeDelta;
 
 namespace {
 
 // UpdateResult indicates what happened with the primary and/or backup update
@@ skipping 155 matching lines @@
   parser.FormatGetHash(prefixes, &get_hash);
 
   fetcher->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   fetcher->SetRequestContext(request_context_getter_.get());
   fetcher->SetUploadData("text/plain", get_hash);
   fetcher->Start();
 }
 
 void SafeBrowsingProtocolManager::GetNextUpdate() {
   DCHECK(CalledOnValidThread());
-  if (!request_.get() && request_type_ == NO_REQUEST)
+  if (!request_.get() && request_type_ == NO_REQUEST) {
+#if !defined(OS_ANDROID)
     IssueUpdateRequest();

[Scott Hess - ex-Googler, 2014/05/30 16:21:07]

This function could be restructured as:

{
  if (request_.get() || request_type_ != NO_REQUEST)
    return;

#if defined(OS_ANDROID)
  if (your_new_condition) {
    // Turn off updates.
    return;
  }
#endif

  IssueUpdateRequest();
}

I think that would be a more Chromium-style format for the function.

This will also make it easier to inline IssueUpdateRequest() later.  Looking at
things, I can't see why it's even separate.

+#else
+    net::NetworkChangeNotifier::ConnectionType type =
+      net::NetworkChangeNotifier::GetConnectionType();
+    if (type == net::NetworkChangeNotifier::CONNECTION_WIFI) {
+      IssueUpdateRequest();
+    } else {
+      ScheduleNextUpdate(false /* no back off */);

[klobag.chromium, 2014/05/30 05:50:15]

What is next_update_interval_ look like in common case? 30min? 2h?

[Scott Hess - ex-Googler, 2014/05/30 16:21:07]

The server sends things down, it generally seems to be around 30 minutes.  I
think the idea is that if the server needs to break things up into chunks for
some reason, it can send a shorter period.

[Feng Qian, 2014/05/30 17:15:59]

If a user is off WiFi for awhile, it will use the last update_interval from
server. Should we let Chrome back off in this case?

[Scott Hess - ex-Googler, 2014/05/30 17:24:57]

I don't think backoff is right for this case.  I think the best long-term option
would be to notice that we came back onto wifi and schedule an update like the
one at startup, which happens in the first five minutes or so.

I think the main downside of this solution is that it'll keep checking rather
than just going quiet.  But I'll leave that kind of policy question on your end.

+    }
+#endif
+  }
 }
 
 // net::URLFetcherDelegate implementation ----------------------------------
 
 // All SafeBrowsing request responses are handled here.
 // TODO(paulg): Clarify with the SafeBrowsing team whether a failed parse of a
 //              chunk should retry the download and parse of that chunk (and
 //              what back off / how many times to try), and if that effects the
 //              update back off. For now, a failed parse of the chunk means we
 //              drop it. This isn't so bad because the next UPDATE_REQUEST we
@@ skipping 571 matching lines @@
     FullHashCallback callback, bool is_download)
     : callback(callback),
       is_download(is_download) {
 }
 
 SafeBrowsingProtocolManager::FullHashDetails::~FullHashDetails() {
 }
 
 SafeBrowsingProtocolManagerDelegate::~SafeBrowsingProtocolManagerDelegate() {
 }