Ignore: v4_update_protocol_manager: Basic implementation with TODOs

v4_update_protocol_manager: Basic implementation with TODOs
See https://codereview.chromium.org/1727033003/ instead

BUG=543161

[vakh (use Gerrit instead), 2016-02-23 02:52:57]

Basic implementation of the update protocol manager with histograms and TODOs.
No tests yet.

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

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

[vakh (use Gerrit instead), 2016-02-23 17:48:05]

Some more changes and tests to follow.

[vakh (use Gerrit instead), 2016-02-23 17:50:55]

Description was changed from

==========
More progress towards defining the v4_update_protocol_manager

BUG=
==========

to

==========
v4_update_protocol_manager: Basic implementation with TODOs

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

[Nathan Parker, 2016-02-23 18:47:07]

Cool!

I did only a light review of the .cc file, since my comments in the .h apply to
it.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:252:
RecordGetHashResult(ORT_BACKOFF_ERROR);
What's ORT? An idea: You could make these a "class enum" and then use
OperationResultType::BACKOFF_ERROR

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:58: struct
UpdateListIdentifier {
Is this the key for the list, like in the map of key->list? (maybe add a
comment)

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:148: // TODO(vakh):
Update the list_states_ map correctly.
I wonder if the client state should be passed into GetUpdateRequest, rather than
stored within the class.  When we load from disk, we'll get a state there. 
Also, when an update response comes in, this class doesn't know for sure if the
update will be successfully written to disk (or merged in memory), so it's
probably up to the DB (DatabaseStore?) to track the state.

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:8: // A class that
implements Chrome's interface with the SafeBrowsing V4 protocol.
"...V4 update protocol"

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:12: // class is to get
full hash matches from the SB server for the given set of
Is this right?  (the full hash match line)

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:119:
scoped_ptr<net::URLFetcher> request_;
If there's only one, then we don't need a map above, ya? We probably shouldn't
have more than one at once.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:128: UpdateRequests
update_requests_;
Is this this the UpdateRequests for the outstanding request_?

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:130: // For v4, the
next update time is set to the backoff time is the last
rm "For v4"

[vakh (use Gerrit instead), 2016-02-23 23:57:03]

CR feedback. Basic response validation after parsing.

[vakh (use Gerrit instead), 2016-02-23 23:58:20]

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
File components/safe_browsing_db/v4_get_hash_protocol_manager.cc (right):

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_get_hash_protocol_manager.cc:252:
RecordGetHashResult(ORT_BACKOFF_ERROR);
On 2016/02/23 18:47:06, nparker wrote:
> What's ORT? An idea: You could make these a "class enum" and then use
> OperationResultType::BACKOFF_ERROR

Done.
ORT was short for OperationResultType. I gave it this obscure name so that it
doesn't collide with another enum in the namespace but putting it in a class is
a better idea.

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.cc:148: // TODO(vakh):
Update the list_states_ map correctly.
On 2016/02/23 18:47:06, nparker wrote:
> I wonder if the client state should be passed into GetUpdateRequest, rather
than
> stored within the class.  When we load from disk, we'll get a state there. 
> Also, when an update response comes in, this class doesn't know for sure if
the
> update will be successfully written to disk (or merged in memory), so it's
> probably up to the DB (DatabaseStore?) to track the state.

Done.

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:8: // A class that
implements Chrome's interface with the SafeBrowsing V4 protocol.
On 2016/02/23 18:47:06, nparker wrote:
> "...V4 update protocol"

Done.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:12: // class is to get
full hash matches from the SB server for the given set of
On 2016/02/23 18:47:06, nparker wrote:
> Is this right?  (the full hash match line)

Done.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:119:
scoped_ptr<net::URLFetcher> request_;
On 2016/02/23 18:47:06, nparker wrote:
> If there's only one, then we don't need a map above, ya? We probably shouldn't
> have more than one at once.

Done.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:128: UpdateRequests
update_requests_;
On 2016/02/23 18:47:06, nparker wrote:
> Is this this the UpdateRequests for the outstanding request_?

Yes. Changed.

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_update_protocol_manager.h:130: // For v4, the
next update time is set to the backoff time is the last
On 2016/02/23 18:47:06, nparker wrote:
> rm "For v4"

Done.

[vakh (use Gerrit instead), 2016-02-24 00:15:09]

Manage the lifetime of the pending request fetcher better

[vakh (use Gerrit instead), 2016-02-24 03:24:25]

Add a few basic tests. More tests in follow-up CLs.

[vakh (use Gerrit instead), 2016-02-24 03:25:56]

PTAL. Now with some tests.

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

https://codereview.chromium.org/1719883003/diff/20001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:58: struct
UpdateListIdentifier {
On 2016/02/23 18:47:06, nparker wrote:
> Is this the key for the list, like in the map of key->list? (maybe add a
> comment)

Done.

[vakh (use Gerrit instead), 2016-02-24 10:07:56]

rebase (and branching funkiness)

[vakh (use Gerrit instead), 2016-02-24 10:21:22]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[vakh (use Gerrit instead), 2016-02-24 10:21:24]

rebase again (and more branching funkiness)

[vakh (use Gerrit instead), 2016-02-24 10:21:49]

The CQ bit was unchecked by vakh@chromium.org

[vakh (use Gerrit instead), 2016-02-24 11:04:48]

I'm having a number of patch issues with the this CL so I've created a new CL
that doesn't have those problems:
https://codereview.chromium.org/1727033003/

Please ignore this CL and review that one. Sorry for the inconvenience.

[vakh (use Gerrit instead), 2016-02-24 11:05:52]

Description was changed from

==========
v4_update_protocol_manager: Basic implementation with TODOs

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

to

==========
v4_update_protocol_manager: Basic implementation with TODOs
See https://codereview.chromium.org/1727033003/ instead

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