Add download_file_types.proto with ascii->binary conversion, as a resource.

Add download_file_types.proto with ascii->binary conversion, as a resource.

We'll maintain an ASCII version of the data in the repo, and build
the binary version as a Resource at build time.  The data will later be
updated at runtime.

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

[Nathan Parker, 2016-04-04 21:07:25]

nparker@chromium.org changed reviewers:
+ asanka@chromium.org, jialiul@google.com, thestig@chromium.org

[Nathan Parker, 2016-04-04 21:07:25]

asanka@ and jialiul@: Please review download_file_types.proto

thestig@: Please review the build config changes

Thanks!

[Lei Zhang, 2016-04-04 21:24:48]

mostly nitty:

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/BUILD.gn (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/BUILD.gn:1: # Generate the binary proto
form of "file_types" from the ascii proto.
Missing copyright header.

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:1: #
Placeholder for list of file extensions that the download manager
Copyright header.

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp...
chrome/chrome_resources.gyp:226: 'dependencies': [ 'make_file_types_protobuf' ]
2 space indent, not 1.

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp...
chrome/chrome_resources.gyp:307: 'chrome.gyp:safe_browsing_proto',
2 space indent, not 4.

[Nathan Parker, 2016-04-04 22:00:53]

Thanks thestig!

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/BUILD.gn (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/BUILD.gn:1: # Generate the binary proto
form of "file_types" from the ascii proto.
On 2016/04/04 21:24:48, Lei Zhang wrote:
> Missing copyright header.

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/browser/resources/sa...
chrome/browser/resources/safe_browsing/download_file_types.asciipb:1: #
Placeholder for list of file extensions that the download manager
On 2016/04/04 21:24:48, Lei Zhang wrote:
> Copyright header.

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp
File chrome/chrome_resources.gyp (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp...
chrome/chrome_resources.gyp:226: 'dependencies': [ 'make_file_types_protobuf' ]
On 2016/04/04 21:24:48, Lei Zhang wrote:
> 2 space indent, not 1.

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/chrome_resources.gyp...
chrome/chrome_resources.gyp:307: 'chrome.gyp:safe_browsing_proto',
On 2016/04/04 21:24:48, Lei Zhang wrote:
> 2 space indent, not 4.

Done. Vim wants to do 4.

[Jialiu Lin, 2016-04-04 22:05:20]

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

[Jialiu Lin, 2016-04-04 22:05:21]

Mostly clarification questions.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/download_file_types.proto (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:4: //
empty line instead of the last "//"?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:33: // action.
If "user gesture" and "user action" refer to the same thing, maybe just stick
with one of them. Also can you give an example of "familiar site"?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:44: // The file type
should not be allowed to open automatically.
Remove "The" at the beginning?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:48: // Includes file types
that upon opening may either:
maybe add indentation or bulleted symbol to indicate this is one of the
criteria?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:53: // Doesn't include
file types that upon opening:
Similar here.
Also, it might sound confusing to ppl not familiar with download protection
logic. Will it be more understandable if we put all these explanation next to
code instead of in the proto definition?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:75: // Don’t send pings
except maybe light-pings.
some explanation of "light-ping"? Any "verdict" comes with "light-pings"?

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:96: // own single entry,
with |platform| not populated.
Since platform can be ALL, how to handle conflicts between different
PlatformSettings? Or should we assume it would never happen?

[Nathan Parker, 2016-04-05 23:20:31]

Thanks for the review, jialiul!  PTAL.

Asanka: I refactored some of the descriptions, and I want to make sure they
still reflect reality. One part I didn't understand so I left you a question in
README.md

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
File chrome/common/safe_browsing/download_file_types.proto (right):

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:4: //
On 2016/04/04 22:05:21, JialiuLin wrote:
> empty line instead of the last "//"?

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:33: // action.
On 2016/04/04 22:05:21, JialiuLin wrote:
> If "user gesture" and "user action" refer to the same thing, maybe just stick
> with one of them. Also can you give an example of "familiar site"?

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:44: // The file type
should not be allowed to open automatically.
On 2016/04/04 22:05:21, JialiuLin wrote:
> Remove "The" at the beginning?

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:48: // Includes file types
that upon opening may either:
On 2016/04/04 22:05:21, JialiuLin wrote:
> maybe add indentation or bulleted symbol to indicate this is one of the
> criteria?

Done.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:53: // Doesn't include
file types that upon opening:
On 2016/04/04 22:05:21, JialiuLin wrote:
> Similar here.
> Also, it might sound confusing to ppl not familiar with download protection
> logic. Will it be more understandable if we put all these explanation next to
> code instead of in the proto definition?

Yes, from offline conv: I'll put this in a README.md file and strip out much of
the comments here.

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:75: // Don’t send pings
except maybe light-pings.
On 2016/04/04 22:05:21, JialiuLin wrote:
> some explanation of "light-ping"? Any "verdict" comes with "light-pings"?

This is explained in the design doc. I'll link to it in the README

https://codereview.chromium.org/1857983002/diff/1/chrome/common/safe_browsing...
chrome/common/safe_browsing/download_file_types.proto:96: // own single entry,
with |platform| not populated.
On 2016/04/04 22:05:21, JialiuLin wrote:
> Since platform can be ALL, how to handle conflicts between different
> PlatformSettings? Or should we assume it would never happen?

I'll document the behavior.  If a platform isn't present, it'll use "ALL."  If
there are duplicates, last entry with that value wins.

[Jialiu Lin, 2016-04-05 23:25:38]

lgtm

[Lei Zhang, 2016-04-05 23:39:10]

lgtm

[asanka, 2016-04-06 02:12:31]

I reviewed the earlier patchset, but the comments should be relevant to the new
patchset as well.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/download_file_types.proto (right):

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:25: enum
DownloadDangerLevel {
Nit: The /Download/ prefixes aren't strictly necessary since it's implicit that
all defined properties apply to DownloadFileType.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:66: ALL = 0;
This is exposed as DownloadFileType::ALL? Perhaps PLATFORM_ALL or ALL_PLATFORMS
would be clearer?

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:96: // own single entry,
with |platform| not populated.
As far as maintainability goes, would it be easier to have the _<OS>.asciipb in
the Chromium tree as well? That way each platform only needs to ship their
relevant settings instead of all settings. It might be a bit more involved to
maintain, but we'd need to keep the _<OS>.asciipb in sync with the Chromium tree
anyway.

Or/also, we can assume a <PLATFORM_ALL, NOT_DANGEROUS, ALLOW_AUTO_OPEN,
TREAT_AS_UNKNOWN> entry always exists if none of the platform entries match the
current platform.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:113: repeated
DownloadFileType file_type = 3;
Nit: style guide says this should be called "file_types"

https://codereview.chromium.org/1857983002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/1857983002/diff/40001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/README.md:4: interactions with Safe
Browsing.  The metadata in download_file_types.asciipb
Embedded underscores could be interpreted as _emphasis_.

[Nathan Parker, 2016-04-11 20:12:07]

Thanks Asanka.  PTAL.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/download_file_types.proto (right):

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:25: enum
DownloadDangerLevel {
On 2016/04/06 02:12:30, asanka wrote:
> Nit: The /Download/ prefixes aren't strictly necessary since it's implicit
that
> all defined properties apply to DownloadFileType.

Good call, done.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:66: ALL = 0;
On 2016/04/06 02:12:31, asanka wrote:
> This is exposed as DownloadFileType::ALL? Perhaps PLATFORM_ALL or
ALL_PLATFORMS
> would be clearer?

I renamed it to PLATFORM_ANY since that's actually more accurate, and made it
the default so most entries can leave it out.  We'll also leave out is_archive
and use the default.  These two shorten the .asciipb file a bit and make it
easier to read.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:96: // own single entry,
with |platform| not populated.
On 2016/04/06 02:12:31, asanka wrote:
> As far as maintainability goes, would it be easier to have the _<OS>.asciipb
in
> the Chromium tree as well? That way each platform only needs to ship their
> relevant settings instead of all settings. It might be a bit more involved to
> maintain, but we'd need to keep the _<OS>.asciipb in sync with the Chromium
tree
> anyway.

My plan was to have one asciipb file, but then we could generate per-platform
binary files (will do in a separate CL).  That way it's easy to compare
platforms and there'll be no divergence on the cross-platform settings
(extension name, uma, etc). Is there a usecase for varying settings other than
those in platform_settings? 

> 
> Or/also, we can assume a <PLATFORM_ALL, NOT_DANGEROUS, ALLOW_AUTO_OPEN,
> TREAT_AS_UNKNOWN> entry always exists if none of the platform entries match
the
> current platform.

That's an interesting idea. It could fall back to the default_file_type's
setting. Once I generate the full list in proto form, I'll look at how common
that one is to see if it's worthwhile.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:113: repeated
DownloadFileType file_type = 3;
On 2016/04/06 02:12:31, asanka wrote:
> Nit: style guide says this should be called "file_types"

Done.

https://codereview.chromium.org/1857983002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/1857983002/diff/40001/chrome/browser/resource...
chrome/browser/resources/safe_browsing/README.md:4: interactions with Safe
Browsing.  The metadata in download_file_types.asciipb
On 2016/04/06 02:12:31, asanka wrote:
> Embedded underscores could be interpreted as _emphasis_.

yea, hrm.  I thought intra-word was disabled.  I'll see how it looks rendered,
and maybe adjust it. I suspect most people will read it in .txt form.

[asanka, 2016-04-11 20:45:03]

LGTM w/nits.

You can use tools/md_browser to preview Markdown in case you didn't know.

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
File chrome/common/safe_browsing/download_file_types.proto (right):

https://codereview.chromium.org/1857983002/diff/20001/chrome/common/safe_brow...
chrome/common/safe_browsing/download_file_types.proto:96: // own single entry,
with |platform| not populated.
On 2016/04/11 at 20:12:07, Nathan Parker wrote:
> On 2016/04/06 02:12:31, asanka wrote:
> > As far as maintainability goes, would it be easier to have the _<OS>.asciipb
in
> > the Chromium tree as well? That way each platform only needs to ship their
> > relevant settings instead of all settings. It might be a bit more involved
to
> > maintain, but we'd need to keep the _<OS>.asciipb in sync with the Chromium
tree
> > anyway.
> 
> My plan was to have one asciipb file, but then we could generate per-platform
binary files (will do in a separate CL).  That way it's easy to compare
platforms and there'll be no divergence on the cross-platform settings
(extension name, uma, etc). Is there a usecase for varying settings other than
those in platform_settings? 

SGTM. I had misunderstood what we were going to store server-side vs in the
Chromium tree.

> > Or/also, we can assume a <PLATFORM_ALL, NOT_DANGEROUS, ALLOW_AUTO_OPEN,
> > TREAT_AS_UNKNOWN> entry always exists if none of the platform entries match
the
> > current platform.
> 
> That's an interesting idea. It could fall back to the default_file_type's
setting. Once I generate the full list in proto form, I'll look at how common
that one is to see if it's worthwhile.

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:23: Leave it unset for 'false'.
Nit: backticks.

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:29: last one wins.  Keep them
sorted by platform.
Implementation-wise, "first one wins" is better. That way we can stop searching
as soon as we see an entry that matches the current platform (either an exact
match or we find an entry with no platform). Otherwise we'd need to keep
searching after finding a matching entry and also keep track of whether we saw
an exact match or an entry with no platform set.

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:51: Clarify this sentence: We
don't warn for files that...
To rephrase:

"Some file types (e.g.: .local and .manifest) aren't dangerous to open. However,
their presence on the file system may cause potentially dangerous changes in
behavior for other programs. We allow automatically opening these file types,
but always warn when they are downloaded."

[Nathan Parker, 2016-04-12 21:03:42]

Thanks!

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/safe_browsing/README.md (right):

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:23: Leave it unset for 'false'.
On 2016/04/11 20:45:03, asanka wrote:
> Nit: backticks.

Done.

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:29: last one wins.  Keep them
sorted by platform.
On 2016/04/11 20:45:02, asanka wrote:
> Implementation-wise, "first one wins" is better. That way we can stop
searching
> as soon as we see an entry that matches the current platform (either an exact
> match or we find an entry with no platform). Otherwise we'd need to keep
> searching after finding a matching entry and also keep track of whether we saw
> an exact match or an entry with no platform set.

Done.

https://codereview.chromium.org/1857983002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/safe_browsing/README.md:51: Clarify this sentence: We
don't warn for files that...
On 2016/04/11 20:45:02, asanka wrote:
> To rephrase:
> 
> "Some file types (e.g.: .local and .manifest) aren't dangerous to open.
However,
> their presence on the file system may cause potentially dangerous changes in
> behavior for other programs. We allow automatically opening these file types,
> but always warn when they are downloaded."

Done.

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

The CQ bit was checked by nparker@chromium.org

[Nathan Parker, 2016-04-12 21:04:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, jialiul@chromium.org, asanka@chromium.org
Link to the patchset: https://codereview.chromium.org/1857983002/#ps120001
(title: "Tweaks to README.md and proto")

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

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

[commit-bot: I haz the power, 2016-04-12 21:55:46]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-12 21:58:49]

Description was changed from

==========
Add download_file_types.proto with ascii->binary conversion, as a resource.

We'll maintain an ASCII version of the data in the repo, and build
the binary version as a Resource at build time.  The data will later be
updated at runtime.

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

to

==========
Add download_file_types.proto with ascii->binary conversion, as a resource.

We'll maintain an ASCII version of the data in the repo, and build
the binary version as a Resource at build time.  The data will later be
updated at runtime.

BUG=596555

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

[commit-bot: I haz the power, 2016-04-12 21:58:51]

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

[Not at Google. Contact bengr, 2016-04-12 23:07:24]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1881273002/ by kundaji@chromium.org.

The reason for reverting is: This cl is breaking  Android Debug (Nexus 5):
https://build.chromium.org/p/chromium.gpu/builders/Android%20Debug%20%28Nexus...

Error log:
FAILED: python
../../chrome/browser/resources/safe_browsing/gen_file_type_proto.py -i
../../chrome/browser/resources/safe_browsing/download_file_types.asciipb -o
gen/chrome/browser/resources/safe_browsing/download_file_types.pb -p
pyproto/chrome/common/safe_browsing
Traceback (most recent call last):
  File "../../chrome/browser/resources/safe_browsing/gen_file_type_proto.py",
line 13, in <module>
    import google.protobuf.text_format as text_format
ImportError: No module named google.protobuf.text_format
ninja: build stopped: subcommand failed..