Simple: Move PVer4 related code from util.* to v4_protocol_manager_util.*

Move code for PVer4 from util.* to v4_protocol_manager_util.*

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

[vakh (use Gerrit instead), 2016-08-05 01:48:12]

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

[commit-bot: I haz the power, 2016-08-05 01:48:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 01:51:48]

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

[commit-bot: I haz the power, 2016-08-05 01:51:49]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vakh (use Gerrit instead), 2016-08-05 17:34:42]

git fetch

[vakh (use Gerrit instead), 2016-08-05 17:34:44]

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

[commit-bot: I haz the power, 2016-08-05 17:35:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-08-05 17:37:43]

Tiny: Add :v4_protocol_manager_util as a DEPS of :util

[vakh (use Gerrit instead), 2016-08-05 17:37:45]

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

[commit-bot: I haz the power, 2016-08-05 17:37:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-08-05 17:41:35]

Patchset #2 (id:20001) has been deleted

[vakh (use Gerrit instead), 2016-08-05 17:41:48]

Patchset #1 (id:1) has been deleted

[vakh (use Gerrit instead), 2016-08-05 17:45:31]

Tiny: Remove a comment

[vakh (use Gerrit instead), 2016-08-05 17:45:33]

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

[commit-bot: I haz the power, 2016-08-05 17:45:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-08-05 17:48:03]

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

[commit-bot: I haz the power, 2016-08-05 17:59:09]

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

[commit-bot: I haz the power, 2016-08-05 17:59:10]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[Nathan Parker, 2016-08-05 18:07:39]

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.cc (right):

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.cc:23: 
I assume these were just moved...

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

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:34: // The sorted list of
hash prefixes.
Is this needed by anything other than the v4LocalDBManager?  It could go there
instead, since the big-string-of-concatenated-hashes is an implementation
detail.

That might apply to PrefixSize as well, not sure.  HashPrefix has a built-in
size.

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:102: const
UpdateListIdentifier kUrlMalware(PLATFORM_TYPE, URL, MALWARE_THREAT);
nit: The names could include "Id" or "ListId", so it doesn't look like it's a
URL.

Will we need one for API blacklist as well (for hash-checking)?

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:198: static void
RecordHttpResponseOrErrorCode(const char* metric_name,
Is this specific to the V4ProtocolManager? Maybe there's not a better place for
it.  I just want to make sure we're not over-namespacing these things.

[vakh (use Gerrit instead), 2016-08-05 18:25:22]

nparker@ feedback. Fix compilation errors.

[vakh (use Gerrit instead), 2016-08-05 18:25:25]

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

[commit-bot: I haz the power, 2016-08-05 18:25:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vakh (use Gerrit instead), 2016-08-05 18:25:48]

Thanks for the review. PTAL.

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
File components/safe_browsing_db/v4_protocol_manager_util.cc (right):

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.cc:23: 
On 2016/08/05 18:07:39, Nathan Parker wrote:
> I assume these were just moved...

Yes, from util.cc

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

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:34: // The sorted list of
hash prefixes.
On 2016/08/05 18:07:39, Nathan Parker wrote:
> Is this needed by anything other than the v4LocalDBManager?  It could go there
> instead, since the big-string-of-concatenated-hashes is an implementation
> detail.
Currently it is being used by the Store only.
I moved them all here so that they can be defined together. My assumption is
that anyone looking into this code would need to understand why we need
HashPrefix, HashPrefixes, and PrefixSize.

But I also think it is just my assumption and you have a good argument about why
HashPrefixes and PrefixSize don't belong here. Moving them to V4Store.

> 
> That might apply to PrefixSize as well, not sure.  HashPrefix has a built-in
> size.

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:102: const
UpdateListIdentifier kUrlMalware(PLATFORM_TYPE, URL, MALWARE_THREAT);
On 2016/08/05 18:07:39, Nathan Parker wrote:
> nit: The names could include "Id" or "ListId", so it doesn't look like it's a
> URL.
Done.

> 
> Will we need one for API blacklist as well (for hash-checking)?

Yes. Will add it later.

https://codereview.chromium.org/2218723002/diff/60001/components/safe_browsin...
components/safe_browsing_db/v4_protocol_manager_util.h:198: static void
RecordHttpResponseOrErrorCode(const char* metric_name,
On 2016/08/05 18:07:39, Nathan Parker wrote:
> Is this specific to the V4ProtocolManager? Maybe there's not a better place
for
> it.  I just want to make sure we're not over-namespacing these things.

It records the information about the response received from the server and is
only used in both protocol manager classes (update/get_hash) so I think this is
the right place for this method.

[commit-bot: I haz the power, 2016-08-05 19:44:41]

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

[commit-bot: I haz the power, 2016-08-05 19:44:42]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2016-08-05 21:11:00]

lgtm

[vakh (use Gerrit instead), 2016-08-05 21:11:30]

The CQ bit was checked by vakh@chromium.org

[commit-bot: I haz the power, 2016-08-05 21:12:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-05 21:17:57]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-08-05 21:19:42]

Description was changed from

==========
Move code for PVer4 from util.* to v4_protocol_manager_util.*

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

to

==========
Move code for PVer4 from util.* to v4_protocol_manager_util.*

BUG=543161

Committed: https://crrev.com/4424f4abf1e9b4ad4b4a52371e2646921e6872b5
Cr-Commit-Position: refs/heads/master@{#410164}
==========

[commit-bot: I haz the power, 2016-08-05 21:19:43]

Patchset 3 (id:??) landed as
https://crrev.com/4424f4abf1e9b4ad4b4a52371e2646921e6872b5
Cr-Commit-Position: refs/heads/master@{#410164}

[Robert Sesek, 2016-08-08 16:51:52]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.chromium.org/2229533002/ by rsesek@chromium.org.

The reason for reverting is: Causing static initializers:

https://build.chromium.org/p/chromium/builders/Mac/builds/18359

# Static initializers in /b/c/b/mac_ng/src/out/Release/Chromium
Framework.framework/Chromium Framework:
# HINT: To get this list, run tools/mac/show_mod_init_func.py
# HINT: diff against the log from the last run to see what changed
/b/c/b/mac_ng/src/out/Release/Chromium Framework.unstripped
0x00000000013489f0 @ _GLOBAL__sub_I_v4_store.cc (in Chromium
Framework.unstripped) (v4_store.cc:0)
0x000000000168c3f0 @ _GLOBAL__sub_I_safe_browsing_service.cc (in Chromium
Framework.unstripped) (safe_browsing_service.cc:0)
0x00000000016bbc30 @ _GLOBAL__sub_I_local_database_manager.cc (in Chromium
Framework.unstripped) (local_database_manager.cc:0)
0x00000000016e6300 @ _GLOBAL__sub_I_services_delegate_impl.cc (in Chromium
Framework.unstripped) (services_delegate_impl.cc:0)
0x000000000391ed70 @ _GLOBAL__sub_I_database_manager.cc (in Chromium
Framework.unstripped) (database_manager.cc:0)
0x000000000391fd00 @ _GLOBAL__sub_I_util.cc (in Chromium Framework.unstripped)
(util.cc:0)
0x0000000003934180 @ _GLOBAL__sub_I_v4_get_hash_protocol_manager.cc (in Chromium
Framework.unstripped) (v4_get_hash_protocol_manager.cc:0)
0x0000000003936ef0 @ _GLOBAL__sub_I_v4_local_database_manager.cc (in Chromium
Framework.unstripped) (v4_local_database_manager.cc:0)
0x0000000003938990 @ _GLOBAL__sub_I_v4_database.cc (in Chromium
Framework.unstripped) (v4_database.cc:0)
0x000000000393a580 @ _GLOBAL__sub_I_v4_update_protocol_manager.cc (in Chromium
Framework.unstripped) (v4_update_protocol_manager.cc:0)
.