Switch Safe Browsing's metadata from string to struct.

Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct. https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and clean up.

BUG=589610
Committed: https://crrev.com/c7614e617a9a7830a6b0da02701b0340699566b3
Cr-Commit-Position: refs/heads/master@{#378082}

[Nathan Parker, 2016-02-26 00:16:10]

Description was changed from

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.

Later CLs will add parsing methods in the appropriate place in the stack:
  - For Pver3: protocol_parser.cc
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc**

** I've added the parsing in this CL so a subsequent clank/ CL can use it.

At the end I'll remove the string and use the parsed fields from the struct.

BUG=589610
==========

to

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct
  - Enable parsing for Pver3, and switch ui_manager.cc to use the parsed values,
and clean up.

BUG=589610
==========

[Nathan Parker, 2016-02-26 00:27:20]

Patchset #2 (id:20001) has been deleted

[Nathan Parker, 2016-02-26 00:27:38]

Patchset #2 (id:40001) has been deleted

[Nathan Parker, 2016-02-26 00:27:55]

Patchset #2 (id:60001) has been deleted

[Nathan Parker, 2016-02-26 00:28:23]

Patchset #2 (id:80001) has been deleted

[Nathan Parker, 2016-02-26 00:40:29]

Description was changed from

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct
  - Enable parsing for Pver3, and switch ui_manager.cc to use the parsed values,
and clean up.

BUG=589610
==========

to

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct.
https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and
clean up.

BUG=589610
==========

[Nathan Parker, 2016-02-26 00:56:09]

nparker@chromium.org changed reviewers:
+ mattm@chromium.org, thestig@chromium.org

[Nathan Parker, 2016-02-26 00:56:09]

thestig: Please review changes in safe_browsing_resource_throttle.*

mattm: Please review everything else.

Thanks!

[mattm, 2016-02-26 02:32:23]

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

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/safe_browsing_api_handler_util.h:50:
UmaRemoteCallResult ParseJson(const std::string& metadata_str,
less generic name

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

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/util.h:66: struct ThreatMetadata {
Should SBThreatType be in here too?

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/util.h:89: 
nit: extra newline

[Lei Zhang, 2016-02-26 02:36:54]

On 2016/02/26 00:56:09, nparker wrote:
> thestig: Please review changes in safe_browsing_resource_throttle.*

lgtm

[Nathan Parker, 2016-02-26 18:54:48]

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

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/util.h:66: struct ThreatMetadata {
On 2016/02/26 02:32:23, mattm wrote:
> Should SBThreatType be in here too?

I considered that but since that would add SBThreatType to SBFullHashResult, it
would be a larger refactoring.  Some places that create SBFullHashResult's don't
have a SBThreatType to add to it, so we'd need to ensure that users of it
weren't expecting the value to be populated.

I think that's a good idea for simplicity, but should be a separate CL.

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/util.h:89: 
On 2016/02/26 02:32:23, mattm wrote:
> nit: extra newline

Done.

[mattm, 2016-02-26 23:23:20]

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

https://codereview.chromium.org/1726403006/diff/120001/components/safe_browsi...
components/safe_browsing_db/util.h:66: struct ThreatMetadata {
On 2016/02/26 18:54:48, nparker wrote:
> On 2016/02/26 02:32:23, mattm wrote:
> > Should SBThreatType be in here too?
> 
> I considered that but since that would add SBThreatType to SBFullHashResult,
it
> would be a larger refactoring.  Some places that create SBFullHashResult's
don't
> have a SBThreatType to add to it, so we'd need to ensure that users of it
> weren't expecting the value to be populated.
> 
> I think that's a good idea for simplicity, but should be a separate CL.

sgtm. I'm fine having that in a separate CL. Or not doing it if it turns out to
make some things confusing/error prone.

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
File components/safe_browsing_db/safe_browsing_api_handler_unittest.cc (right):

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
components/safe_browsing_db/safe_browsing_api_handler_unittest.cc:22: return
ParseJson(json, &threat_, &meta_);
missed this one

I don't understand why this is causing a compile failure on the
linux_chromium_compile_dbg_ng trybot too though. Shouldn't this test only be
compiled on android builds?

[Nathan Parker, 2016-02-26 23:34:52]

PTAL

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
File components/safe_browsing_db/safe_browsing_api_handler_unittest.cc (right):

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
components/safe_browsing_db/safe_browsing_api_handler_unittest.cc:22: return
ParseJson(json, &threat_, &meta_);
On 2016/02/26 23:23:19, mattm wrote:
> missed this one
> 
> I don't understand why this is causing a compile failure on the
> linux_chromium_compile_dbg_ng trybot too though. Shouldn't this test only be
> compiled on android builds?

Done.

Huh.  I don't see where its picking that up.  It's as if that built bot is using
safe_browsing_mode==2:

https://code.google.com/p/chromium/codesearch#search/&q=unit_tests_mobile&sq=...

[mattm, 2016-02-26 23:43:24]

lgtm

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
File components/safe_browsing_db/safe_browsing_api_handler_unittest.cc (right):

https://codereview.chromium.org/1726403006/diff/140001/components/safe_browsi...
components/safe_browsing_db/safe_browsing_api_handler_unittest.cc:22: return
ParseJson(json, &threat_, &meta_);
On 2016/02/26 23:34:52, nparker wrote:
> On 2016/02/26 23:23:19, mattm wrote:
> > missed this one
> > 
> > I don't understand why this is causing a compile failure on the
> > linux_chromium_compile_dbg_ng trybot too though. Shouldn't this test only be
> > compiled on android builds?
> 
> Done.
> 
> Huh.  I don't see where its picking that up.  It's as if that built bot is
using
> safe_browsing_mode==2:
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=unit_tests_mobile&sq=...

yeah, it's weird. Maybe file an infra bug about it?

[Nathan Parker, 2016-02-26 23:45:56]

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-02-26 23:45:57]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1726403006/#ps160001
(title: "Fix ParseJson in test")

[commit-bot: I haz the power, 2016-02-26 23:48:11]

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

[Nathan Parker, 2016-02-26 23:55:19]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=590389 , though I'm
not sure I have the right labels to have it be addressed.

[commit-bot: I haz the power, 2016-02-27 01:12:18]

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

[commit-bot: I haz the power, 2016-02-27 01:12:19]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nathan Parker, 2016-02-27 01:17:51]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2016-02-27 01:21:43]

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

[commit-bot: I haz the power, 2016-02-27 02:12:36]

Description was changed from

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct.
https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and
clean up.

BUG=589610
==========

to

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct.
https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and
clean up.

BUG=589610
==========

[commit-bot: I haz the power, 2016-02-27 02:12:37]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-02-27 02:14:49]

Description was changed from

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct.
https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and
clean up.

BUG=589610
==========

to

==========
Switch Safe Browsing's metadata from string to struct.

This is needed so we can add more types to it.  This CL plumbs through
the struct but keeps the string in-tact.  This is backward compatible with
clank/'s current implementation.

I've added parsing methods in the appropriate place in the stack:
  - For Pver4: v4_get_hash_protocol_manager.cc
  - For XLB: safe_browsing_api_handler_util.cc
  - For Pver3: protocol_parser.cc [separate CL]

The next two CLs:
  - Switch clank/ to use the struct.
https://chrome-internal-review.googlesource.com/#/c/249571/
  - Enable parsing for Pver3, switch ui_manager.cc to use the parsed values, and
clean up.

BUG=589610

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

[commit-bot: I haz the power, 2016-02-27 02:14:50]

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