[Android] Only update safe-browsing database on WiFi

chrome/browser/safe_browsing/protocol_manager.cc

Index: chrome/browser/safe_browsing/protocol_manager.cc
diff --git a/chrome/browser/safe_browsing/protocol_manager.cc b/chrome/browser/safe_browsing/protocol_manager.cc
index 78d96ca102abcf7cb20ffa97c408eec0a222f946..af72cb22b33d8cf78dfbb075c4cafef433809a20 100644
--- a/chrome/browser/safe_browsing/protocol_manager.cc
+++ b/chrome/browser/safe_browsing/protocol_manager.cc
@@ -22,6 +22,9 @@
 #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"
@@ -197,8 +200,19 @@ void SafeBrowsingProtocolManager::GetFullHash(
 
 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 ----------------------------------