Makes V4UpdateProtocolManager auto-schedule update fetching

Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
Committed: https://crrev.com/d9fe40cf5b17eb771b37837b3b1a198ef109669a
Cr-Commit-Position: refs/heads/master@{#384704}

[vakh (use Gerrit instead), 2016-03-31 21:47:05]

git fetch && git pull && gclient sync

[vakh (use Gerrit instead), 2016-03-31 22:07:24]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org

[vakh (use Gerrit instead), 2016-03-31 22:49:04]

formatting and removed ClearCurrentListStates from test

[Nathan Parker, 2016-04-01 01:04:53]

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:391:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
Do you want this in the local_database_manager, or a new database_manager? 
Mixing the two will make it harder to distinguish v3 from v4 functionality.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.h:179: V4GetHashProtocolManager*
v4_get_hash_protocol_manager_;
Ahh yes, this shouldn't be in the parent class.  I missed that before.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:111:
V4ProtocolManagerUtil(){};
Is this an auto-format-ism?  Seems odd.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:84: return
scoped_ptr<V4UpdateProtocolManager>(new V4UpdateProtocolManager(
I think you can drop the scoped_ptr<..> bit, and it'll still invoke that ctor.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:135: if (!is_headless)
{
The is_headless check seems like the wrong thing... I think we shouldn't have
special paths in prod code for tests, but instead mock out bits in the test
code.  So you could pass in a test timer to use instead of the normal
update_timer_.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:149: if
(config_.disable_auto_update) {
Can this change at runtime?  If not, the update_timer_ shouldn't have been set
already, and you could just return.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:183: void
V4UpdateProtocolManager::GetNextUpdate() {
Can you drop this function altogether? IssueUpdateRequest already checks
request_.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:191: std::string
V4UpdateProtocolManager::GetUpdateRequest(
Nit: "get" and "request" are slightly confounding.  Maybe this could be
"CreateUpdateRequest"?

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:232:
response.minimum_wait_duration().seconds());
a thought: We might want a sanity check on this, like "must be at least 60
seconds" or something.  Maybe it's not important.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:302:
callback_.Run(list_update_responses);
What do you want the callback semantics to be?  Should it call the callback_ on
a regular schedule, or only when there is a successful update?  Seems like the
latter would be fine, since that's the only time a DB-update would need to
happen.  So there's no worry of "blocking" the caller -- it'll just wait forever
w/o issue.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:318:
ScheduleNextUpdate(back_off);
One concern: Now that this class owns the "state" of each list, it has no notion
of if the on-disk and in-memory DBs were successfully updated to that state. 
What do we want to happen then?  (if disk-write fails, or if the update somehow
doesn't make sense)  The answer could be to tear down the updateProtocolManager,
wipe the DB, and start over, or it could be to try again for a while.  We should
do some verification.

One possible path:  You could not schedule the next update until the caller
invokes a method on this class with new states, or a success measure.  This is
sort of back to the way you hand it except this class would still manage most of
the timing.  You just wouldn't start the timer until the disk/memory updates
were done and successful.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:121: void
GetNextUpdate();
ScheduleNextUpdate?

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:159: base::TimeDelta
next_update_interval_;
Is this the time after the _next_ update?

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:37: static
void ValidateGetUpdatesResults(
(I ran out of time -- will look at tests tomorrow)

[vakh (use Gerrit instead), 2016-04-01 02:09:12]

Incorporate review feedback.

[vakh (use Gerrit instead), 2016-04-01 02:21:54]

Delete v4_update_protocol_manager_ on IO thread stop. And fix BUILD.gn

[vakh (use Gerrit instead), 2016-04-01 02:23:02]

Thanks for the review. PTAL.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/database_manager.h:179: V4GetHashProtocolManager*
v4_get_hash_protocol_manager_;
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Ahh yes, this shouldn't be in the parent class.  I missed that before.

Acknowledged.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:111:
V4ProtocolManagerUtil(){};
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Is this an auto-format-ism?  Seems odd.

Yes, auto-format in vim.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:84: return
scoped_ptr<V4UpdateProtocolManager>(new V4UpdateProtocolManager(
On 2016/04/01 01:04:52, Nathan Parker wrote:
> I think you can drop the scoped_ptr<..> bit, and it'll still invoke that ctor.

Tried. It doesn't.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:149: if
(config_.disable_auto_update) {
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Can this change at runtime?  If not, the update_timer_ shouldn't have been set
> already, and you could just return.

Done.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:183: void
V4UpdateProtocolManager::GetNextUpdate() {
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Can you drop this function altogether? IssueUpdateRequest already checks
> request_.

Done.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:191: std::string
V4UpdateProtocolManager::GetUpdateRequest(
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Nit: "get" and "request" are slightly confounding.  Maybe this could be
> "CreateUpdateRequest"?

Done.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:232:
response.minimum_wait_duration().seconds());
On 2016/04/01 01:04:52, Nathan Parker wrote:
> a thought: We might want a sanity check on this, like "must be at least 60
> seconds" or something.  Maybe it's not important.

Done.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:302:
callback_.Run(list_update_responses);
On 2016/04/01 01:04:52, Nathan Parker wrote:
> What do you want the callback semantics to be?  Should it call the callback_
on
> a regular schedule, or only when there is a successful update?  Seems like the
> latter would be fine, since that's the only time a DB-update would need to
> happen.  So there's no worry of "blocking" the caller -- it'll just wait
forever
> w/o issue.

Agree. That was the idea but the comment was incorrect. Fixed.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:318:
ScheduleNextUpdate(back_off);
On 2016/04/01 01:04:52, Nathan Parker wrote:
> One concern: Now that this class owns the "state" of each list, it has no
notion
> of if the on-disk and in-memory DBs were successfully updated to that state. 
> What do we want to happen then?  (if disk-write fails, or if the update
somehow
> doesn't make sense)  The answer could be to tear down the
updateProtocolManager,
> wipe the DB, and start over, or it could be to try again for a while.  We
should
> do some verification.
> 
> One possible path:  You could not schedule the next update until the caller
> invokes a method on this class with new states, or a success measure.  This is
> sort of back to the way you hand it except this class would still manage most
of
> the timing.  You just wouldn't start the timer until the disk/memory updates
> were done and successful.

I think it would be much cleaner and verifiable to just destroy the
V4UpdateProtocolManager and start afresh if the state is corrupted. That
knowledge exists in the DB/DBManager and the protocol manager shouldn't need to
know about it.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:121: void
GetNextUpdate();
On 2016/04/01 01:04:52, Nathan Parker wrote:
> ScheduleNextUpdate?

Removed it entirely and scheduling IssueUpdateRequest on the timer directly.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:159: base::TimeDelta
next_update_interval_;
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Is this the time after the _next_ update?  

The minimum time between the current update request and next.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:37: static
void ValidateGetUpdatesResults(
On 2016/04/01 01:04:52, Nathan Parker wrote:
> (I ran out of time -- will look at tests tomorrow)

Acknowledged.

[vakh (use Gerrit instead), 2016-04-01 02:29:09]

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:391:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
On 2016/04/01 01:04:52, Nathan Parker wrote:
> Do you want this in the local_database_manager, or a new database_manager? 
> Mixing the two will make it harder to distinguish v3 from v4 functionality.

I want to keep this CL small so I don't want to create a new DBManager right
now. I'll work on that once this CL has landed. Is that OK?

[vakh (use Gerrit instead), 2016-04-01 02:29:49]

Description was changed from

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
==========

to

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
==========

[Nathan Parker, 2016-04-01 17:56:59]

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:391:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
On 2016/04/01 02:29:09, vakh wrote:
> On 2016/04/01 01:04:52, Nathan Parker wrote:
> > Do you want this in the local_database_manager, or a new database_manager? 
> > Mixing the two will make it harder to distinguish v3 from v4 functionality.
> 
> I want to keep this CL small so I don't want to create a new DBManager right
> now. I'll work on that once this CL has landed. Is that OK?

Sure, though it's probably better to not add it to local_db_mgr at all now,
since you'll just have to remove it.  And, by instantiating it, it may start
making network calls.

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager.cc (right):

https://codereview.chromium.org/1848973004/diff/40001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:318:
ScheduleNextUpdate(back_off);
On 2016/04/01 02:23:02, vakh wrote:
> On 2016/04/01 01:04:52, Nathan Parker wrote:
> > One concern: Now that this class owns the "state" of each list, it has no
> notion
> > of if the on-disk and in-memory DBs were successfully updated to that state.

> > What do we want to happen then?  (if disk-write fails, or if the update
> somehow
> > doesn't make sense)  The answer could be to tear down the
> updateProtocolManager,
> > wipe the DB, and start over, or it could be to try again for a while.  We
> should
> > do some verification.
> > 
> > One possible path:  You could not schedule the next update until the caller
> > invokes a method on this class with new states, or a success measure.  This
is
> > sort of back to the way you hand it except this class would still manage
most
> of
> > the timing.  You just wouldn't start the timer until the disk/memory updates
> > were done and successful.
> 
> I think it would be much cleaner and verifiable to just destroy the
> V4UpdateProtocolManager and start afresh if the state is corrupted. That
> knowledge exists in the DB/DBManager and the protocol manager shouldn't need
to
> know about it.

That sounds good.  Even without corruption, an update could fire while the last
update was getting written to disk (if laptop goes to sleep for some hours right
after update fires)

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:65: return
scoped_ptr<V4UpdateProtocolManager>(V4UpdateProtocolManager::Create(
the Create() method already returns a scoped_ptr

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:170:
scoped_refptr<base::TestSimpleTaskRunner> runner(
nit: You could make most of these part of the class, so each function has access
and it's less repetitive.

[vakh (use Gerrit instead), 2016-04-01 19:35:20]

Do not instantiate v4_update_protocol_manager_ in local_db_m

[vakh (use Gerrit instead), 2016-04-01 19:35:48]

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/local_database_manager.h (right):

https://codereview.chromium.org/1848973004/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/local_database_manager.h:391:
base::hash_map<UpdateListIdentifier, std::string> current_list_states_;
On 2016/04/01 17:56:58, Nathan Parker wrote:
> On 2016/04/01 02:29:09, vakh wrote:
> > On 2016/04/01 01:04:52, Nathan Parker wrote:
> > > Do you want this in the local_database_manager, or a new database_manager?

> > > Mixing the two will make it harder to distinguish v3 from v4
functionality.
> > 
> > I want to keep this CL small so I don't want to create a new DBManager right
> > now. I'll work on that once this CL has landed. Is that OK?
> 
> Sure, though it's probably better to not add it to local_db_mgr at all now,
> since you'll just have to remove it.  And, by instantiating it, it may start
> making network calls.

Good point. Removed. Will add it in the V4DBManager.

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
File components/safe_browsing_db/v4_update_protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:65: return
scoped_ptr<V4UpdateProtocolManager>(V4UpdateProtocolManager::Create(
On 2016/04/01 17:56:58, Nathan Parker wrote:
> the Create() method already returns a scoped_ptr

Done. Good catch.

https://codereview.chromium.org/1848973004/diff/80001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager_unittest.cc:170:
scoped_refptr<base::TestSimpleTaskRunner> runner(
On 2016/04/01 17:56:58, Nathan Parker wrote:
> nit: You could make most of these part of the class, so each function has
access
> and it's less repetitive.

Not storing any state in the class keeps each of the test independent which
prevents flaky test failures.
I am introducing one boolean in an upcoming CL but I am not too happy about it.

Plus, some init code is different for some of the tests.
For instance, the cases where the response code is 204, or when the response is
"", we do not setup expected_lurs and current_list_states, whereas in other
cases we do.

[vakh (use Gerrit instead), 2016-04-01 20:00:16]

git fetch && git pull && gclient sync

[Nathan Parker, 2016-04-01 21:44:55]

lgtm

[vakh (use Gerrit instead), 2016-04-01 21:52:03]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-04-01 21:52:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848973004/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848973004/120001

[commit-bot: I haz the power, 2016-04-01 22:10:11]

Description was changed from

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
==========

to

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
==========

[commit-bot: I haz the power, 2016-04-01 22:10:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-01 22:13:36]

Description was changed from

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161
==========

to

==========
Makes V4UpdateProtocolManager auto-schedule update fetching

V4UpdateProtocolManager now takes the current status of the lists at
instantiation and auto-schedules updates for those lists throughout its
lifetime.
It also takes a callback method which is called after each successful update.
TODOs: Timeout, callback implementation is empty, backoff tests ...

BUG=543161

Committed: https://crrev.com/d9fe40cf5b17eb771b37837b3b1a198ef109669a
Cr-Commit-Position: refs/heads/master@{#384704}
==========

[commit-bot: I haz the power, 2016-04-01 22:13:37]

Patchset 7 (id:??) landed as
https://crrev.com/d9fe40cf5b17eb771b37837b3b1a198ef109669a
Cr-Commit-Position: refs/heads/master@{#384704}