Move common PVer4 code into a V4ProtocolManagerUtil class.

Move common PVer4 code into a V4ProtocolManagerUtil class.

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

[vakh (use Gerrit instead), 2016-02-19 02:38:31]

GetHashUrl -> GetRequestUrl

[vakh (use Gerrit instead), 2016-02-19 02:45:23]

unit_tests depends on target :v4_protocol_manager_util

[vakh (use Gerrit instead), 2016-02-22 17:54:21]

sync and fix compile

[vakh (use Gerrit instead), 2016-02-22 17:57:34]

Description was changed from

==========
Move common PVer4 code into a V4ProtocolManagerUtil class. Tests pass.

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

to

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

[vakh (use Gerrit instead), 2016-02-22 17:58:48]

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

[Nathan Parker, 2016-02-22 22:39:32]

lgtm

after comments are addressed, and Kendra has a look

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:17: namespace {
Do you want this in an anonymous namespace in the .h file?  I guess that makes
it visible to anyone who includes the .h, but won't collide with any other
same-named types in other .h files.  Seems better to just put it in the SB
namespace, unless there's a specific reason to do it this way.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:21: enum ParseResultType
{
Is this the union of Full-Hash checks and update checks?

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:52: extern const char
kSbV4UrlPrefix[];
If this isn't needed by anyone other that the v4_protocol_manager_util.cc, then
you could just put it there.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:68:
~V4ProtocolManagerUtil() {};
Probably don't need this.  And you could make the constructor private so noone
can instantiate it.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util_unittest.cc (right):

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util_unittest.cc:29: void
PopulateV4ProtocolConfig(V4ProtocolConfig& config) {
Use
  V4ProtocolConfig*
since it's writing to it.

[kcarattini, 2016-02-23 00:49:13]

kcarattini@chromium.org changed reviewers:
+ kcarattini@chromium.org

[kcarattini, 2016-02-23 00:49:13]

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:91: static
base::TimeDelta GetNextBackOffInterval(size_t* error_count,
If this is specific to GetHash then why put it in util? I thought it was, but if
not, please update the comment.

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

rebase

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

Minor changes. Incorporate CR feedack.

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

V4GetHashProtocolConfig->V4ProtocolConfig in remote_database_manager.

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

Pass config as pointer, instead of reference

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

Thanks for the review. PTAL.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util.h (right):

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:17: namespace {
On 2016/02/22 22:39:32, nparker wrote:
> Do you want this in an anonymous namespace in the .h file?  I guess that makes
> it visible to anyone who includes the .h, but won't collide with any other
> same-named types in other .h files.  Seems better to just put it in the SB
> namespace, unless there's a specific reason to do it this way.

Done.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:21: enum ParseResultType
{
On 2016/02/22 22:39:32, nparker wrote:
> Is this the union of Full-Hash checks and update checks?  

At first I thought both modules could share this, but it looks like they'll have
pretty different parse errors so moving it to v4_get_hash_protocol_manager.cc in
anon namespace.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:52: extern const char
kSbV4UrlPrefix[];
On 2016/02/22 22:39:32, nparker wrote:
> If this isn't needed by anyone other that the v4_protocol_manager_util.cc,
then
> you could just put it there.

Done.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:68:
~V4ProtocolManagerUtil() {};
On 2016/02/22 22:39:32, nparker wrote:
> Probably don't need this.  And you could make the constructor private so noone
> can instantiate it.

Done.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util.h:91: static
base::TimeDelta GetNextBackOffInterval(size_t* error_count,
On 2016/02/23 00:49:13, kcarattini wrote:
> If this is specific to GetHash then why put it in util? I thought it was, but
if
> not, please update the comment.

Done. Updated the comment.
Backoff is used by both classes.

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
File components/safe_browsing_db/v4_protocol_manager_util_unittest.cc (right):

https://codereview.chromium.org/1703413002/diff/120001/components/safe_browsi...
components/safe_browsing_db/v4_protocol_manager_util_unittest.cc:29: void
PopulateV4ProtocolConfig(V4ProtocolConfig& config) {
On 2016/02/22 22:39:32, nparker wrote:
> Use
>   V4ProtocolConfig*
> since it's writing to it.

Done.

[Nathan Parker, 2016-02-23 02:01:56]

lgtm

[kcarattini, 2016-02-23 02:28:45]

lgtm

You should update the bug in the cl description.

[vakh (use Gerrit instead), 2016-02-23 02:35:40]

Description was changed from

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

to

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

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

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-02-23 02:36:39]

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

[commit-bot: I haz the power, 2016-02-23 04:14:23]

Description was changed from

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

to

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

[commit-bot: I haz the power, 2016-02-23 04:14:24]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-02-23 04:16:27]

Description was changed from

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

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

to

==========
Move common PVer4 code into a V4ProtocolManagerUtil class.

BUG=543161

Committed: https://crrev.com/98a95013345caefc21dcd078f95967b67197ec26
Cr-Commit-Position: refs/heads/master@{#376921}
==========

[commit-bot: I haz the power, 2016-02-23 04:16:28]

Patchset 11 (id:??) landed as
https://crrev.com/98a95013345caefc21dcd078f95967b67197ec26
Cr-Commit-Position: refs/heads/master@{#376921}