FileTypePolicies class to load types from proto.

FileTypePolicies class to load types from proto.

There will be a single instance of this owned by the
SafeBrowsingService, and updated periodically via a component update.

BUG=596555
Committed: https://crrev.com/db0171fd53ec52c6326c40478c3a9feed1407c45
Cr-Commit-Position: refs/heads/master@{#389703}

[Nathan Parker, 2016-04-20 20:33:12]

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

[Jialiu Lin, 2016-04-22 21:36:41]

A couple of nits, though 5 try bots are not happy right now. I'll check back
later.

https://codereview.chromium.org/1909653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right):

https://codereview.chromium.org/1909653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/gen_file_type_proto.py:25: ifile =
open(opts.infile, 'rb')
should it be 'r', since you're not reading a binary proto?

https://codereview.chromium.org/1909653002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies_unittest.cc (right):

https://codereview.chromium.org/1909653002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies_unittest.cc:39:
TEST_F(FileTypePoliciesTest, UnpackResourceBundle) {
This test seems to fail on a couple of try bots.

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right):

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:5: # See
README.md before editing this file.
Where is the README.md file?

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:10:
version_id: 1
somehow I cannot find the download_file_types.proto either. Reverted?

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:90: // Check that we haven't
dropped more than 1/2 the list.
Just curious, why 1/2 ? Any rationale behind checking delta?

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:143: return
PolicyForFile(file).platform_settings(0);
Maybe do a safety check on platform_settings() in case this repeated field is
empty?

[Nathan Parker, 2016-04-25 22:39:20]

https://codereview.chromium.org/1909653002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/gen_file_type_proto.py (right):

https://codereview.chromium.org/1909653002/diff/20001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/gen_file_type_proto.py:25: ifile =
open(opts.infile, 'rb')
On 2016/04/22 21:36:41, JialiuLin wrote:
> should it be 'r', since you're not reading a binary proto?

Done.

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right):

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:5: # See
README.md before editing this file.
On 2016/04/22 21:36:41, JialiuLin wrote:
> Where is the README.md file?

It was all reverted. I'll wait till I successfully reland it all.

https://codereview.chromium.org/1909653002/diff/60001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:10:
version_id: 1
On 2016/04/22 21:36:41, JialiuLin wrote:
> somehow I cannot find the download_file_types.proto either. Reverted?

Acknowledged.

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:90: // Check that we haven't
dropped more than 1/2 the list.
On 2016/04/22 21:36:41, JialiuLin wrote:
> Just curious, why 1/2 ? Any rationale behind checking delta?

I picked 1/2 out of the air.  

If someone screws up the file, or it gets truncated in the push process, or it
fails to read the file from disk and it ends up basically empty, this would
catch a sudden unexpected change in size.  I doubt we'd ever intentionally
shorten the file at all.

If you think this doesn't add value, I can remove it.

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:143: return
PolicyForFile(file).platform_settings(0);
On 2016/04/22 21:36:41, JialiuLin wrote:
> Maybe do a safety check on platform_settings() in case this repeated field is
> empty?

It should have exactly one entry, per line 80 above. I've added a DCHECK here
just to be safe.

[Jialiu Lin, 2016-04-25 23:26:48]

LGTM

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
File chrome/common/safe_browsing/file_type_policies.cc (right):

https://codereview.chromium.org/1909653002/diff/60001/chrome/common/safe_brow...
chrome/common/safe_browsing/file_type_policies.cc:143: return
PolicyForFile(file).platform_settings(0);
On 2016/04/25 at 22:39:20, Nathan Parker wrote:
> On 2016/04/22 21:36:41, JialiuLin wrote:
> > Maybe do a safety check on platform_settings() in case this repeated field
is
> > empty?
> 
> It should have exactly one entry, per line 80 above. I've added a DCHECK here
just to be safe.
Ah, you're totally right. line 80 already did the check.

[Nathan Parker, 2016-04-26 04:17:34]

The dependent patch finally landed, and I think it'll stick, so I'm submitting.

[Nathan Parker, 2016-04-26 04:17:44]

The CQ bit was checked by nparker@chromium.org

[commit-bot: I haz the power, 2016-04-26 04:18:00]

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

[commit-bot: I haz the power, 2016-04-26 05:15:39]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-26 05:16:31]

Description was changed from

==========
FileTypePolicies class to load types from proto.

There will be a single instance of this owned by the
SafeBrowsingService, and updated periodically via a component update.

BUG=596555
==========

to

==========
FileTypePolicies class to load types from proto.

There will be a single instance of this owned by the
SafeBrowsingService, and updated periodically via a component update.

BUG=596555

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

[commit-bot: I haz the power, 2016-04-26 05:16:32]

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