Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(156)

Issue 1929693003: Populate download_file_types proto and filter it for each platform (Closed)

Created:
4 years, 7 months ago by Nathan Parker
Modified:
4 years, 7 months ago
CC:
arv+watch_chromium.org, chromium-reviews, grt+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@file_type_polices2
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Populate download_file_types proto and filter it for each platform. Except for a few extensions that were missing in the old code, these data retain the same settings as the old .cc files. Also: * Move ping_settings out of platform_settings. It'll have one value for all platforms. * Allow for an empty platform_settings and fill in the default_file_type's values. This makes the config shorter. * Add UMA values for two missing extensions and one for when we can't load the list at all. The data were generated by joining kDownloadFileTypes (from download_extensions.cc) and kSafeBrowsingFileTypes (from download_protection_util.cc) in a one-off conversion. The join aimed to minimize the number of items in platform_settings by setting a default when those settings were used by the majority of platforms for that file type. I left "ping_setting" set in all types (rather than use a default of FULL_PING) since it wouldn't be obvious in that state what the default value was. The conversion was done with //depot/google3/experimental/users/nparker/extensions/dump_new_proto.cc BUG=596555 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/59a573bc29379c9b316b464e4fe838d2ef644a93 Cr-Commit-Position: refs/heads/master@{#393114}

Patch Set 1 #

Patch Set 2 : Add real types, move ping_settings out of platform_settings #

Patch Set 3 : Update docs, add is_archive #

Patch Set 4 : nits in comments #

Patch Set 5 : rebase #

Total comments: 2

Patch Set 6 : Add MacOS-specialized types, flip default to ALLOW_AUTO_OPEN, and add OS-specific tests #

Patch Set 7 : Fix uma_value for ad and run, add missing entries for pdf, apk, torrent. #

Patch Set 8 : fix gyp build #

Patch Set 9 : (rebase) #

Patch Set 10 : fix chromeos detection again #

Total comments: 6

Patch Set 11 : Fixes to comments and .gyp per thestig #

Patch Set 12 : Switch to grouped extensions and add comments #

Total comments: 10

Patch Set 13 : reorder ascipb per asanka's comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2838 lines, -91 lines) Patch
M chrome/browser/resources/safe_browsing/BUILD.gn View 1 2 chunks +19 lines, -0 lines 0 comments Download
M chrome/browser/resources/safe_browsing/README.md View 1 2 3 4 5 6 7 8 9 10 4 chunks +42 lines, -34 lines 0 comments Download
M chrome/browser/resources/safe_browsing/download_file_types.asciipb View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2487 lines, -17 lines 0 comments Download
M chrome/browser/resources/safe_browsing/gen_file_type_proto.py View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +130 lines, -9 lines 0 comments Download
M chrome/chrome_resources.gyp View 1 2 3 4 5 6 7 8 9 10 2 chunks +17 lines, -0 lines 0 comments Download
M chrome/common/safe_browsing/download_file_types.proto View 1 2 3 4 5 6 7 8 9 10 3 chunks +19 lines, -16 lines 0 comments Download
M chrome/common/safe_browsing/download_protection_util.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/common/safe_browsing/file_type_policies.cc View 1 2 3 4 5 6 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/common/safe_browsing/file_type_policies_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +109 lines, -10 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +8 lines, -3 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 34 (17 generated)
Nathan Parker
The data populated here should be functionally identical to the original files. If we want ...
4 years, 7 months ago (2016-05-02 23:35:09 UTC) #9
asanka
lgtm. I only spot checked a few entries and they looked legit. :) Since we ...
4 years, 7 months ago (2016-05-04 18:10:40 UTC) #11
Jialiu Lin
LGTM https://codereview.chromium.org/1929693003/diff/140001/chrome/browser/resources/safe_browsing/README.md File chrome/browser/resources/safe_browsing/README.md (right): https://codereview.chromium.org/1929693003/diff/140001/chrome/browser/resources/safe_browsing/README.md#newcode12 chrome/browser/resources/safe_browsing/README.md:12: * Edit `download_file_types.asciipb` and update `histograms.xml` Do we ...
4 years, 7 months ago (2016-05-04 20:01:57 UTC) #12
Nathan Parker
I added a test for each platform and discovered I was missing MacOS-specific types, so ...
4 years, 7 months ago (2016-05-05 00:27:07 UTC) #13
Nathan Parker
thakis@chromium.org: Please review changes in chrome/chrome_resources.gyp holte@chromiumorg: Please review histograms.xml Thanks!
4 years, 7 months ago (2016-05-05 00:39:11 UTC) #15
Steven Holte
lgtm
4 years, 7 months ago (2016-05-05 19:43:40 UTC) #16
Nathan Parker
Nico -- Can you take a look at the small change to chrome/chrome_resources.gyp? Thanks!
4 years, 7 months ago (2016-05-06 18:21:38 UTC) #17
Nathan Parker
+thestig@chromium.org: Can you look at chrome/chrome_resources.gyp? Thanks!
4 years, 7 months ago (2016-05-09 18:59:28 UTC) #19
Lei Zhang
https://codereview.chromium.org/1929693003/diff/240001/chrome/browser/resources/safe_browsing/README.md File chrome/browser/resources/safe_browsing/README.md (right): https://codereview.chromium.org/1929693003/diff/240001/chrome/browser/resources/safe_browsing/README.md#newcode19 chrome/browser/resources/safe_browsing/README.md:19: * `extension`: Value must be unique within the config. ...
4 years, 7 months ago (2016-05-09 23:28:01 UTC) #20
Nathan Parker
Thanks, thestig! PTAL https://codereview.chromium.org/1929693003/diff/240001/chrome/browser/resources/safe_browsing/README.md File chrome/browser/resources/safe_browsing/README.md (right): https://codereview.chromium.org/1929693003/diff/240001/chrome/browser/resources/safe_browsing/README.md#newcode19 chrome/browser/resources/safe_browsing/README.md:19: * `extension`: Value must be unique ...
4 years, 7 months ago (2016-05-10 17:33:51 UTC) #22
Lei Zhang
lgtm
4 years, 7 months ago (2016-05-10 19:02:34 UTC) #23
Nathan Parker
Thanks Lei. Asanka -- Per our discussion this morning, I switched the list from "sorted" ...
4 years, 7 months ago (2016-05-10 22:04:01 UTC) #24
asanka
lgtm % nits https://codereview.chromium.org/1929693003/diff/280001/chrome/browser/resources/safe_browsing/download_file_types.asciipb File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right): https://codereview.chromium.org/1929693003/diff/280001/chrome/browser/resources/safe_browsing/download_file_types.asciipb#newcode828 chrome/browser/resources/safe_browsing/download_file_types.asciipb:828: # Windows Access files Nit: Microsoft ...
4 years, 7 months ago (2016-05-11 14:23:03 UTC) #25
Nathan Parker
Thanks! https://codereview.chromium.org/1929693003/diff/280001/chrome/browser/resources/safe_browsing/download_file_types.asciipb File chrome/browser/resources/safe_browsing/download_file_types.asciipb (right): https://codereview.chromium.org/1929693003/diff/280001/chrome/browser/resources/safe_browsing/download_file_types.asciipb#newcode828 chrome/browser/resources/safe_browsing/download_file_types.asciipb:828: # Windows Access files On 2016/05/11 14:23:03, asanka ...
4 years, 7 months ago (2016-05-11 22:12:32 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1929693003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1929693003/300001
4 years, 7 months ago (2016-05-11 22:13:07 UTC) #30
commit-bot: I haz the power
Committed patchset #13 (id:300001)
4 years, 7 months ago (2016-05-12 00:16:13 UTC) #32
commit-bot: I haz the power
4 years, 7 months ago (2016-05-12 00:21:57 UTC) #34
Message was sent while issue was closed.
Patchset 13 (id:??) landed as
https://crrev.com/59a573bc29379c9b316b464e4fe838d2ef644a93
Cr-Commit-Position: refs/heads/master@{#393114}

Powered by Google App Engine
This is Rietveld 408576698