[Android] Only update safe-browsing database on WiFi

[Android] Only update safe-browsing database on WiFi

Per our discussion, SB database update is only enabled on
WiFi network.

BUG=376016
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273969

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

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:212: ScheduleNextUpdate(false
/* no back off */);
What is next_update_interval_ look like in common case? 30min? 2h?

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

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:27: #endif
The OS-specific section should come after the main group of includes.  [Ignore
the NDEBUG bit, I think it's probably wrong, too.]

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:205: IssueUpdateRequest();
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.

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:212: ScheduleNextUpdate(false
/* no back off */);
On 2014/05/30 05:50:15, klobag.chromium wrote:
> What is next_update_interval_ look like in common case? 30min? 2h?

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]

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:212: ScheduleNextUpdate(false
/* no back off */);
On 2014/05/30 16:21:07, shess wrote:
> On 2014/05/30 05:50:15, klobag.chromium wrote:
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?

> > What is next_update_interval_ look like in common case? 30min? 2h?
> 
> 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.

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

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://chromiumcodereview.appspot.com/302603008/diff/1/chrome/browser/safe_b...
chrome/browser/safe_browsing/protocol_manager.cc:212: ScheduleNextUpdate(false
/* no back off */);
On 2014/05/30 17:15:59, Feng Qian wrote:
> On 2014/05/30 16:21:07, shess wrote:
> > On 2014/05/30 05:50:15, klobag.chromium wrote:
> > > What is next_update_interval_ look like in common case? 30min? 2h?
> > 
> > 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.
> 
> 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?

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.

[Feng Qian, 2014-05-30 17:42:20]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:46:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/feng@chromium.org/302603008/20001

[commit-bot: I haz the power, 2014-05-30 17:46:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:46:56]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Feng Qian, 2014-05-30 17:50:45]

On 2014/05/30 17:46:56, I haz the power (commit-bot) wrote:
> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a provisional committer, _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

oops, confused by LGTM of another CL.

shess, can you take a look at the latest CL?

[Scott Hess - ex-Googler, 2014-05-30 17:53:17]

lgtm

[Feng Qian, 2014-05-30 18:00:16]

The CQ bit was unchecked by feng@chromium.org

[Feng Qian, 2014-05-30 18:00:20]

The CQ bit was checked by feng@chromium.org

[commit-bot: I haz the power, 2014-05-30 18:00:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/feng@chromium.org/302603008/20001

[commit-bot: I haz the power, 2014-05-31 01:02:52]

Change committed as 273969