Add a GetFullHashWithApis method to Protocol Manager.

Add a GetFullHashWithApis method to Protocol Manager.

BUG=561867
Committed: https://crrev.com/414ab2c40e058cac9610b1a9ae0c6b392e197330
Cr-Commit-Position: refs/heads/master@{#367732}

[kcarattini, 2015-12-23 04:20:24]

kcarattini@chromium.org changed reviewers:
+ awoz@google.com, nparker@chromium.org

[Nathan Parker, 2015-12-27 01:10:16]

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

[Nathan Parker, 2015-12-27 01:10:17]

Let's make this more Pver4-generic since it'll get reused.  thx.

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:237: std::string
SafeBrowsingProtocolManager::GetHashWithApisRequest(
This could be non-"Apis
-specific by passing the threat types as a param, and then calling it
GetV4HashRequest(..)

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:260: void
SafeBrowsingProtocolManager::GetFullHashWithApis(
Same here.  This function could call a generic Pver4 version, with the API_ABUSE
threat type so the caller doesn't need to know about the proto.

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:812: GURL
SafeBrowsingProtocolManager::GetHashWithApisUrl(
This is not "Apis" specific -- we could call it GetV4HashUrl()

[kcarattini, 2015-12-28 00:05:46]

Thank you for the Holiday Review! PTAL.

Kendra

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:237: std::string
SafeBrowsingProtocolManager::GetHashWithApisRequest(
On 2015/12/27 01:10:17, nparker wrote:
> This could be non-"Apis
> -specific by passing the threat types as a param, and then calling it
> GetV4HashRequest(..)

Done.

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:260: void
SafeBrowsingProtocolManager::GetFullHashWithApis(
On 2015/12/27 01:10:17, nparker wrote:
> Same here.  This function could call a generic Pver4 version, with the
API_ABUSE
> threat type so the caller doesn't need to know about the proto.

Done.

https://codereview.chromium.org/1543153002/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:812: GURL
SafeBrowsingProtocolManager::GetHashWithApisUrl(
On 2015/12/27 01:10:17, nparker wrote:
> This is not "Apis" specific -- we could call it GetV4HashUrl()

Done.

[awoz, 2015-12-28 15:57:32]

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:260: void
SafeBrowsingProtocolManager::GetV4FullHash(
nit: This function name should reflect the fact that multiple full hashes may be
returned.

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:274: v4_hash_requests_[fetcher]
= FullHashDetails(callback,
Maybe a TODO to implement a new response processor?

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:275: false  /* is_download */);
nit: indent off

[kcarattini, 2015-12-29 00:12:48]

Thanks for the review! PTAL.

Kendra

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:260: void
SafeBrowsingProtocolManager::GetV4FullHash(
On 2015/12/28 15:57:31, awoz wrote:
> nit: This function name should reflect the fact that multiple full hashes may
be
> returned.

Done.

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:274: v4_hash_requests_[fetcher]
= FullHashDetails(callback,
On 2015/12/28 15:57:32, awoz wrote:
> Maybe a TODO to implement a new response processor?

Done.

https://codereview.chromium.org/1543153002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:275: false  /* is_download */);
On 2015/12/28 15:57:32, awoz wrote:
> nit: indent off

Done.

[awoz, 2015-12-29 16:46:18]

lgtm

[vakh (old account. dont use), 2015-12-29 20:53:33]

https://codereview.chromium.org/1543153002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:823:
"https://safebrowsing.googleapis.com/v4", "encodedFullHashes",
This partial URL string should probably be in safe_browsing_service.cc

[kcarattini, 2015-12-29 22:33:41]

https://codereview.chromium.org/1543153002/diff/80001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/80001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/protocol_manager.cc:823:
"https://safebrowsing.googleapis.com/v4", "encodedFullHashes",
On 2015/12/29 20:53:33, Varun Khaneja wrote:
> This partial URL string should probably be in safe_browsing_service.cc

Defined as a constant in this file to avoid a dependency on
safe_browsing_service.

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager.cc:275: net::URLFetcher::GET,
this)
Changed to GET as per offline discussion with awoz.

[vakh (old account. dont use), 2016-01-04 23:52:52]

lgtm

[kcarattini, 2016-01-05 21:07:16]

I think I'm still waiting for an lgtm from Nathan on this.

Thanks,
Kendra

[Nathan Parker, 2016-01-05 22:44:00]

lgtm

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager.cc:94: const char kSbV4URLPrefix[]
= "https://safebrowsing.googleapis.com/v4";
kSbV4UrlPrefix

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager.cc:250: std::string hash;
nit: I think you can do
std::string hash(ptr, size)

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:303:
hash.append(reinterpret_cast<const char*>(&one), sizeof(SBPrefix));
nit: hash = std::string(ptr, size)

[kcarattini, 2016-01-05 23:30:57]

Thanks!

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_manager.cc (right):

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager.cc:94: const char kSbV4URLPrefix[]
= "https://safebrowsing.googleapis.com/v4";
On 2016/01/05 22:44:00, nparker wrote:
> kSbV4UrlPrefix

Done.

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager.cc:250: std::string hash;
On 2016/01/05 22:44:00, nparker wrote:
> nit: I think you can do
> std::string hash(ptr, size)

Done.

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/protocol_manager_unittest.cc (right):

https://codereview.chromium.org/1543153002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/protocol_manager_unittest.cc:303:
hash.append(reinterpret_cast<const char*>(&one), sizeof(SBPrefix));
On 2016/01/05 22:44:00, nparker wrote:
> nit: hash = std::string(ptr, size)

Done.

[kcarattini, 2016-01-05 23:39:22]

The CQ bit was checked by kcarattini@chromium.org

[kcarattini, 2016-01-05 23:39:22]

The patchset sent to the CQ was uploaded after l-g-t-m from awoz@google.com,
nparker@chromium.org, vakh@google.com
Link to the patchset: https://codereview.chromium.org/1543153002/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2016-01-05 23:42:02]

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

[commit-bot: I haz the power, 2016-01-06 01:04:56]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-01-06 01:06:53]

Description was changed from

==========
Add a GetFullHashWithApis method to Protocol Manager.

BUG=561867
==========

to

==========
Add a GetFullHashWithApis method to Protocol Manager.

BUG=561867

Committed: https://crrev.com/414ab2c40e058cac9610b1a9ae0c6b392e197330
Cr-Commit-Position: refs/heads/master@{#367732}
==========

[commit-bot: I haz the power, 2016-01-06 01:06:54]

Patchset 8 (id:??) landed as
https://crrev.com/414ab2c40e058cac9610b1a9ae0c6b392e197330
Cr-Commit-Position: refs/heads/master@{#367732}